fix: handle nesting in comma separated selectors by layershifter · Pull Request #207 · microsoft/griffel

layershifter · 2022-09-06T15:21:53Z

Fixes #204.
Fixes #206.

Changes in selectors handling

In resolveStyleRules() & compileCSS() I renamed pseudo to selectors and changed their type to be string[]:

To reflect what it contains as selectors can contain not only pseudo selectors.
To handle selectors properly as it was a problem without a solution:
```
makeStyles({
  ":hover,:focus-within": {
    "::before": {
      color: "orange"
    }
  }
});
```
Before we were concatenating inputs with selectors and getting ":hover,:focus-within::before" that will be compiled by Stylis to:
```
.class:hover,.class:focus-within::before {}
```
And without proper wrapping with curly braces we cannot handle it. With selectors as an array with can send as input :hover,:focus-within { ::before { } } that will be handled properly.

Changes in `:global()` handling

Because before we didn't know scopes of selectors we were doing .trim() and were losing " " in rules (#206):

griffel/packages/core/src/runtime/compileCSS.ts

Lines 92 to 93 in 0e358cb

    
           // should be normalized to handle ":global(SELECTOR) &" 
        
           const normalizedPseudo = normalizeNestedProperty(restPseudo.trim());

Currently I kept existing hack with :global() in place to avoid more changes in this PR, but in a follow up - I will replace it with a Stylis plugin.

github-actions · 2022-09-06T15:42:46Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`core` makeStyles + mergeClasses (runtime)	`20.339 kB` `7.598 kB`	`20.454 kB` `7.616 kB`	`115 B` `18 B`
`react` makeStyles + mergeClasses (runtime)	`22.099 kB` `8.317 kB`	`22.214 kB` `8.324 kB`	`115 B` `7 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`core` makeStyles + mergeClasses (build time)	`1.8 kB` `861 B`
`react` __css + mergeClasses (build time)	`1.837 kB` `857 B`
`react` __styles + mergeClasses (build time)	`3.535 kB` `1.595 kB`
`react` makeStaticStyles (runtime)	`8.982 kB` `3.908 kB`

_{🤖 This report was generated against 0e358cbda0f663246ed7c1cc1ad7db0bdc86c22c}

layershifter · 2022-09-06T16:04:19Z

      expect(
        compileCSS({
          ...defaultOptions,
-          pseudo: ':global(body) &',


The original selector is invalid, replaced it with a proper test case.

layershifter · 2022-09-06T16:05:01Z

+      // Issues are reported:
+      // https://github.com/thysultan/stylis.js/issues/253
+      // https://github.com/thysultan/stylis.js/issues/252
+      if (selector.indexOf(':global(') === 0) {
+        // 👇 :global(GROUP_1)GROUP_2
+        const GLOBAL_PSEUDO_REGEX = /global\((.+)\)(.+)?/;
+        const result = GLOBAL_PSEUDO_REGEX.exec(selector)!;

-  let rtlClassNameSelector: string | null = null;
-  let rtlCSSDeclaration: string | null = null;
+        globalSelector = result[1] + ' ';
+        const restPseudo = result[2] || '';

-  if (rtlProperty && rtlClassName) {
-    rtlClassNameSelector = `.${rtlClassName}`;
-    rtlCSSDeclaration = Array.isArray(rtlValue)
-      ? `{ ${rtlValue.map(v => `${hyphenateProperty(rtlProperty)}: ${v}`).join(';')}; }`
-      : `{ ${hyphenateProperty(rtlProperty)}: ${rtlValue}; }`;
-  }
+        // should be normalized to handle ":global(SELECTOR) &"
+        const normalizedPseudoSelector = normalizePseudoSelector(restPseudo);

-  let cssRule = '';
+        if (normalizedPseudoSelector === '') {
+          return acc;
+        }

-  // Should be handled by namespace plugin of Stylis, is buggy now
-  // Issues are reported:
-  // https://github.com/thysultan/stylis.js/issues/253
-  // https://github.com/thysultan/stylis.js/issues/252
-  if (pseudo.indexOf(':global(') === 0) {
-    // 👇 :global(GROUP_1)GROUP_2
-    const GLOBAL_PSEUDO_REGEX = /global\((.+)\)(.+)?/;
-    const [, globalSelector, restPseudo = ''] = GLOBAL_PSEUDO_REGEX.exec(pseudo)!;
+        return `${normalizedPseudoSelector} { ${acc} }`;
+      }


I moved the hack with small changes, will replace it with a Stylis plugin in a follow up PR.

layershifter · 2022-09-06T16:05:37Z

+    expect(getStyleBucketName([], '', '', '')).toBe('d');
+    expect(getStyleBucketName(['.foo'], '', '', '')).toBe('d');


New test cases.

layershifter · 2022-09-06T16:06:14Z

+  if (selectors.length > 0) {
+    const normalizedPseudo = selectors[0].trim();


We take into account only the first pseudo selector, so it's find to use the first element.

layershifter · 2022-09-06T16:06:38Z

+      expect(
+        resolveStyleRules({
+          ':hover,:focus-within': {
+            '::before': {
+              color: 'orange',
+            },
+          },
+        }),
+      ).toMatchInlineSnapshot(`
+        .fij4gri:hover::before,
+        .fij4gri:focus-within::before {
+          color: orange;
+        }
+      `);


Test case from #204.

layershifter · 2022-09-06T16:06:54Z

-    it('handles :global selector', () => {
-      expect(
-        resolveStyleRules({
-          ':global(body) &': { color: 'green' },
-        }),
-      ).toMatchInlineSnapshot(`
-        body .fm1e7ra {
-          color: green;
-        }
-      `);
-    });


Again a test that does not have any sense.

layershifter · 2022-09-06T16:07:11Z

      `);
      expect(
        resolveStyleRules({
+          ':global(body):focus': { color: 'pink' },


Now spaces are handled (#206).

layershifter · 2022-09-06T16:07:55Z

+    it('supports :global as a nested selector', () => {
+      expect(
+        resolveStyleRules({
+          ':focus': { ':global(body)': { color: 'green' } },
+        }),
+      ).toMatchInlineSnapshot(`
+        body .fz7er5p:focus {
+          color: green;
+        }
+      `);
+    });


As now we have scopes this test surprisingly works 🐱

ling1726 · 2022-09-07T12:27:59Z

+      // https://github.com/thysultan/stylis.js/issues/252
+      if (selector.indexOf(':global(') === 0) {
+        // 👇 :global(GROUP_1)GROUP_2
+        const GLOBAL_PSEUDO_REGEX = /global\((.+)\)(.+)?/;


maybe hoist the regex so it doesn't get created on every iteration ?

Makes sense, but in a followup I will get it of this part anyway (I will replace this part with a stylis plugin).

ling1726 · 2022-09-07T15:29:11Z

  className: string;

-  pseudo: string;
+  selectors: string[];


would the length of selectors ever exceed 2 ? It seems that the only case we are handling here is pseudo selector + element. It shouldn't be possible to go further since it would be invalid css ?

Can you please provide an example?

layershifter force-pushed the fix/comma-nested branch 2 times, most recently from 6799698 to 058744f Compare September 6, 2022 15:40

layershifter force-pushed the fix/comma-nested branch from 058744f to 27c7266 Compare September 6, 2022 15:56

layershifter commented Sep 6, 2022

View reviewed changes

layershifter force-pushed the fix/comma-nested branch from 27c7266 to 737a2a6 Compare September 6, 2022 16:09

fix: handle nesting in comma separated selectors

280d7fb

layershifter force-pushed the fix/comma-nested branch from 737a2a6 to 280d7fb Compare September 6, 2022 16:09

layershifter marked this pull request as ready for review September 6, 2022 16:18

layershifter requested a review from a team as a code owner September 6, 2022 16:18

ling1726 reviewed Sep 7, 2022

View reviewed changes

ling1726 approved these changes Sep 8, 2022

View reviewed changes

miroslavstastny approved these changes Sep 14, 2022

View reviewed changes

layershifter merged commit 926fe73 into microsoft:main Sep 14, 2022

layershifter deleted the fix/comma-nested branch September 14, 2022 12:14

This was referenced Oct 4, 2022

chore: Update Griffel to latest version microsoft/fluentui#25075

Merged

fix: handle empty strings in mergeClasses() #248

Merged

	// should be normalized to handle ":global(SELECTOR) &"
	const normalizedPseudo = normalizeNestedProperty(restPseudo.trim());

		expect(getStyleBucketName([], '', '', '')).toBe('d');
		expect(getStyleBucketName(['.foo'], '', '', '')).toBe('d');

		if (selectors.length > 0) {
		const normalizedPseudo = selectors[0].trim();

Uh oh!

Conversation

layershifter commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes in selectors handling

Changes in :global() handling

Uh oh!

github-actions Bot commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

layershifter commented Sep 6, 2022 •

edited

Loading

Changes in `:global()` handling

github-actions Bot commented Sep 6, 2022 •

edited

Loading