Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix/stylex-content-quotes #782

Merged
merged 7 commits into from
Nov 27, 2024
Merged

fix/stylex-content-quotes #782

merged 7 commits into from
Nov 27, 2024

Conversation

p0nch000
Copy link
Contributor

@p0nch000 p0nch000 commented Nov 25, 2024

Fix StyleX content property quotes handling

What changed / motivation

Updated transformValue function to correctly handle CSS content property values:

  • Prevents wrapping CSS functions (counter, attr, url) in quotes
  • Preserves CSS keywords (normal, none, open-quote)
  • Supports mixed content values
  • Only adds quotes for plain text strings

Previously, StyleX incorrectly wrapped CSS functions like counters() in quotes, breaking their functionality.

Linked PR/Issues

Fixes #771 - Don't wrap content: counters(...) in quotes

Additional Context

Test Coverage

describe('transformValue content property tests', () => {
  test('preserves CSS functions without quotes', () => {
    const functions = [
      ['counters(div, ".")', 'counters(div, ".")'],
      ['counter(chapter)', 'counter(chapter)'],
      ['attr(href)', 'attr(href)'],
      ['url(image.jpg)', 'url(image.jpg)']
    ];
    functions.forEach(([input, expected]) => {
      expect(transformValue('content', input, {})).toBe(expected);
    });
  });

  test('preserves CSS keywords without quotes', () => {
    const keywords = ['normal', 'none', 'open-quote'];
    keywords.forEach(keyword => {
      expect(transformValue('content', keyword, {})).toBe(keyword);
    });
  });

  test('handles mixed content values', () => {
    expect(transformValue('content', 'open-quote counter(chapter)', {}))
      .toBe('open-quote counter(chapter)');
  });
});

Implementation Notes

  • Pattern matches CSS functions and keywords
  • Errs on side of not adding quotes when uncertain
  • Returns unchanged value if parsing unclear
  • No breaking changes - maintains backward compatibility

Pre-flight checklist

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 25, 2024
@p0nch000
Copy link
Contributor Author

@nmn this was my first approach for the content issue, lmk if need to do some fixes :)

Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fully spec-compliant implementation would be quite, so I think it makes sense to change the implementation to be more forgiving instead.

Let's change the hasCssFunction to use var.includes('attributes(') etc. and similarly for keywords.

Additionally, let's check if the string contains " twice or ' twice instead of checking the first and last value.

If any of these conditions hold we can make a safe guess that the value doesn't need to be wrapped in quotes.


Your tests look great already. Let's add some more mixed case examples and examples where there is no space separating values.

packages/shared/src/transform-value.js Outdated Show resolved Hide resolved
packages/shared/src/transform-value.js Outdated Show resolved Hide resolved
@nmn
Copy link
Contributor

nmn commented Nov 26, 2024

@p0nch000 Thanks for the great work. Turns out values for content don't need to be separated by spaces, so we might need to adjust the code a bit.

@p0nch000
Copy link
Contributor Author

Heyy took your advice i think is better now, lmk if it needs extra work

Copy link
Contributor

@nmn nmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks!

@nmn nmn merged commit 42f70e4 into facebook:main Nov 27, 2024
7 of 8 checks passed
@p0nch000
Copy link
Contributor Author

thanks for the fast review and the great feedback again! I'll look for my next issue to work on @nmn

aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
* Correctly parse functions like counter, url, etc, added a unit test
* Unit test for transform value features
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
* Correctly parse functions like counter, url, etc, added a unit test
* Unit test for transform value features
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't wrap content: counters(...) in quotes.
3 participants