Multiple selectors are not properly scoped

[spmonahan]

When writing multiple selectors separated by commas only the first selector is properly scoped with a class name.

Here's a link to an annotated repro case in CodeSandbox.

Current Behavior

A Griffel styles like

makeStyles({
  root: {
    ":active,:focus-within": {
      ...shorthands.borderColor("red")
    }
  }
});

Produces a selector like this in DevTools:

.f1mk9nzz:active, :focus-within {
    border-right-color: red;
}

Currently only the :active selector is scoped with a generated class name.

Expected Behavior

Both the :active and :focus-within selectors should be scoped with the same generated class name. So we should see something like this in DevTools:

.f1mk9nzz:active, .f1mk9nzz:focus-within {
    border-right-color: red;
}

Alternatively, if this syntax is not supported I should get an error in VSCode explaining to me that I cannot write styles in this way.

Notes

In Codesandbox I get an error (red squiggle under the selectors) when writing ":active,:focus-within" indicating to me that perhaps this way of writing styles is not supported by Griffel. However, I don't get this error when writing styles like this in VS Code so I'm not certain. I'm using griffel@1.0.3 in both Codesandbox and VSCode.

[layershifter]

Hey, this issue was fixed in makeStyles sometime ago, microsoft/fluentui#20348. But read below ⬇️

It breaks with rules that flip in RTL, the simplest example is:

makeStyles({
  root: {
    ":active,:focus-within": {
      paddingLeft: "10px"
    }
  }
});

⬇️⬇️⬇️

.f14f5aie:active,
.f14f5aie:focus-within {
  padding-left: 10px;
}

.f1sheuf0:active,
:focus-within {
  padding-right: 10px;
}

---

P.S. You can dump all rules with:

console.log(
  [...document.querySelectorAll("[data-make-styles-bucket]")]
    .flatMap(el => [...el.sheet.cssRules].map(rule => rule.cssText))
    .join("\n")
);

P.P.S. We have tryout page to check generated CSS.

P.P.P.S. Thanks for reporting 🙏

[layershifter]

The fix is needed in compileCSS.ts:

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

Lines 102 to 105 in 8a6d180

	cssRule = `${classNameSelector}{${normalizedPseudo} ${cssDeclaration}};`;
	if (rtlProperty) {
	cssRule = `${cssRule}; ${rtlClassNameSelector}${normalizedPseudo} ${rtlCSSDeclaration};`;

It's not visible with template literals, but cssRule on 101 & 104 are different - there are missing curly brackets 🤦‍♂️

-cssRule = `${cssRule}; ${rtlClassNameSelector}${normalizedPseudo} ${rtlCSSDeclaration};`;
+cssRule = `${cssRule}; ${rtlClassNameSelector}{${normalizedPseudo} ${rtlCSSDeclaration}};`;

We also need to cover this with a unit test:

    it('handles complex nested selectors', () => {
      expect(resolveStyleRules({ '& > :first-child': { '& svg': { color: 'red' } } })).toMatchInlineSnapshot(`
        .fxfx2ih > :first-child svg {
          color: red;
        }
      `);
    });

+    it('handles comma separated selectors', () => {
+      expect(
+        resolveStyleRules({
+          ':active,:focus-within': {
+            paddingLeft: '10px',
+          },
+        }),
+      ).toMatchInlineSnapshot(`
+        .f14f5aie:active,
+        .f14f5aie:focus-within {
+          padding-left: 10px;
+        }
+        .f1sheuf0:active,
+        .f1sheuf0:focus-within {
+          padding-right: 10px;
+        }
+      `);
+    });

P.S. @spmonahan are you interested in contributing a fix? 😉

[spmonahan]

Didn't see an issue for this in the issue log but glad it was (mostly) fixed already!

Thanks for the debugging tips -- they will be very helpful! I've been following the work on Griffel DevTools and while looking into this issue was thinking, "can't wait for DevTools!".

Happy to contribute a fix. Should have time to work on it today.