fix(makeStyles): handle comma separated selectors #20348

layershifter · 2021-10-26T12:33:38Z

Pull request checklist

~~Addresses an existing issue: Fixes #0000~~
Include a change request file using $ yarn change

Description of changes

This PR fixes a problem originally reported by @khmakoto 👇

makeStyles({
root: {

'@media (forced-colors: active)': {
  ':hover, :focus': {
   color: 'red'
  }
}
})

⬇⬇⬇

@media (forced-colors: active) {
  .fder123:hover,
    /* 🚨 this is completely wrong as ":focus" targets any focused element  */
  :focus {
    color: red;
  }
}

After debugging the problem I discovered that comma separated selectors are not handled properly. In the same time they supported in SASS:

.foo {
  &:hover,
  &:focus {
    color: red;
  }
}

⬇⬇⬇

.foo:hover,
.foo:focus {
  color: red;
}

They supported in mergeStyles() (styling solution from v8):

fluentui/packages/merge-styles/src/mergeStyles.test.ts

Lines 83 to 89 in b5b7401

    
           it('can merge comma delimitted selectors correctly', () => { 
        
             mergeStyles( 
        
               { selectors: { ':hover': { background: 'red' } } }, 
        
               { selectors: { ':hover, :active': { background: 'blue' } } }, 
        
             ); 
        
             expect(_stylesheet.getRules()).toEqual('.css-0:hover{background:blue;}.css-0:active{background:blue;}'); 
        
           });

And it the end they supported by Stylis (CSS preprocessor used in makeStyles()), our problem was that we were passing wrong input to Stylis:

/* ❌ was passed before */
.foo:hover,:focus { color: red }
/* 👆 it's valid CSS, there is nothing to expand */

/* ✅ should be passed */
.foo { &:hover,&:focus { red } }

This PR fixes the reported issue and add new unit tests.

fabricteam · 2021-10-26T12:47:15Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-input` Input	`31.332 kB` `11.311 kB`	`31.392 kB` `11.354 kB`	`60 B` `43 B`
`react-make-styles` makeStyles + mergeClasses (runtime)	`22.175 kB` `8.366 kB`	`22.235 kB` `8.408 kB`	`60 B` `42 B`
`react-slider` RangedSlider	`41.423 kB` `11.975 kB`	`41.432 kB` `11.976 kB`	`9 B` `1 B`
`react-slider` Slider	`34.801 kB` `10.861 kB`	`34.81 kB` `10.862 kB`	`9 B` `1 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-accordion` Accordion (including children components)	`57.707 kB` `18.166 kB`
`react-avatar` Avatar	`54.966 kB` `15.674 kB`
`react-badge` Badge	`23.195 kB` `6.992 kB`
`react-badge` CounterBadge	`25.655 kB` `7.688 kB`
`react-badge` PresenceBadge	`30.674 kB` `8.821 kB`
`react-button` Button	`25.514 kB` `7.744 kB`
`react-button` CompoundButton	`30.771 kB` `8.696 kB`
`react-button` MenuButton	`27.539 kB` `8.42 kB`
`react-button` SplitButton	`33.65 kB` `9.603 kB`
`react-button` ToggleButton	`34.74 kB` `8.396 kB`
`react-card` Card - All	`49.008 kB` `14.564 kB`
`react-card` Card	`44.495 kB` `13.343 kB`
`react-card` CardFooter	`8.141 kB` `3.43 kB`
`react-card` CardHeader	`9.461 kB` `3.883 kB`
`react-card` CardPreview	`8.434 kB` `3.604 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`169.348 kB` `48.062 kB`
`react-components` react-components: FluentProvider & webLightTheme	`32.201 kB` `10.665 kB`
`react-divider` Divider	`15.355 kB` `5.592 kB`
`react-image` Image	`9.784 kB` `3.982 kB`
`react-label` Label	`8.965 kB` `3.713 kB`
`react-link` Link	`11.659 kB` `4.704 kB`
`react-make-styles` makeStaticStyles (runtime)	`7.59 kB` `3.321 kB`
`react-make-styles` makeStyles + mergeClasses (build time)	`2.558 kB` `1.204 kB`
`react-menu` Menu (including children components)	`105.802 kB` `32.207 kB`
`react-menu` Menu (including selectable components)	`108.078 kB` `32.58 kB`
`react-popover` Popover	`101.166 kB` `30.376 kB`
`react-portal` Portal	`6.725 kB` `2.237 kB`
`react-provider` FluentProvider	`15.16 kB` `5.58 kB`
`react-switch` Switch	`26.615 kB` `8.562 kB`
`react-text` Text - Default	`11.351 kB` `4.424 kB`
`react-text` Text - Wrappers	`14.42 kB` `4.729 kB`
`react-tooltip` Tooltip	`45.556 kB` `15.549 kB`

_{🤖 This report was generated against b5b740100714724214e097fae2f7c1f51f294e35}

size-auditor · 2021-10-26T12:49:14Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: b5b740100714724214e097fae2f7c1f51f294e35 (build)

fabricteam · 2021-10-26T13:36:30Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1025	1047	5000
BaseButton	mount	1046	1070	5000
Breadcrumb	mount	2780	2804	1000
ButtonNext	mount	580	570	5000
Checkbox	mount	1759	1719	5000
CheckboxBase	mount	1468	1494	5000
ChoiceGroup	mount	5257	5344	5000
ComboBox	mount	1046	1079	1000
CommandBar	mount	10921	10832	1000
ContextualMenu	mount	6932	6843	1000
DefaultButton	mount	1293	1263	5000
DetailsRow	mount	4163	4136	5000
DetailsRowFast	mount	4154	4282	5000
DetailsRowNoStyles	mount	3953	3997	5000
Dialog	mount	2742	2749	1000
DocumentCardTitle	mount	179	189	1000
Dropdown	mount	3659	3659	5000
FluentProviderNext	mount	3442	3463	5000
FluentProviderWithTheme	mount	210	224	10
FluentProviderWithTheme	virtual-rerender	108	114	10
FluentProviderWithTheme	virtual-rerender-with-unmount	255	237	10
FocusTrapZone	mount	1981	1945	5000
FocusZone	mount	1939	1912	5000
IconButton	mount	1989	2037	5000
Label	mount	392	386	5000
Layer	mount	3305	3294	5000
Link	mount	532	523	5000
MakeStyles	mount	1933	1944	50000
MenuButton	mount	1679	1665	5000
MessageBar	mount	2212	2176	5000
Nav	mount	3682	3654	1000
OverflowSet	mount	1248	1248	5000
Panel	mount	2594	2651	1000
Persona	mount	927	929	1000
Pivot	mount	1602	1579	1000
PrimaryButton	mount	1437	1475	5000
Rating	mount	8771	8807	5000
SearchBox	mount	1487	1537	5000
Shimmer	mount	2861	2880	5000
Slider	mount	2194	2189	5000
SpinButton	mount	5578	5922	5000
Spinner	mount	453	469	5000
SplitButton	mount	3555	3495	5000
Stack	mount	581	588	5000
StackWithIntrinsicChildren	mount	1941	1942	5000
StackWithTextChildren	mount	5369	5444	5000
SwatchColorPicker	mount	11484	11596	5000
Tabs	mount	1646	1565	1000
TagPicker	mount	2887	2892	5000
TeachingBubble	mount	13897	13977	5000
Text	mount	474	489	5000
TextField	mount	1525	1561	5000
ThemeProvider	mount	1283	1327	5000
ThemeProvider	virtual-rerender	663	656	5000
ThemeProvider	virtual-rerender-with-unmount	2079	2089	5000
Toggle	mount	915	910	5000
buttonNative	mount	174	166	5000

Perf Analysis (`@fluentui/react-northstar`)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
CardMinimalPerf.default	677	622	1.09:1
ItemLayoutMinimalPerf.default	1476	1379	1.07:1
RefMinimalPerf.default	261	243	1.07:1
AttachmentMinimalPerf.default	190	179	1.06:1
PortalMinimalPerf.default	198	187	1.06:1
AnimationMinimalPerf.default	463	442	1.05:1
IconMinimalPerf.default	714	678	1.05:1
CarouselMinimalPerf.default	542	521	1.04:1
DropdownManyItemsPerf.default	797	764	1.04:1
HeaderMinimalPerf.default	423	408	1.04:1
ListCommonPerf.default	747	716	1.04:1
ListMinimalPerf.default	572	551	1.04:1
StatusMinimalPerf.default	770	741	1.04:1
ChatDuplicateMessagesPerf.default	343	333	1.03:1
ListWith60ListItems.default	742	719	1.03:1
PopupMinimalPerf.default	637	618	1.03:1
RadioGroupMinimalPerf.default	511	497	1.03:1
SegmentMinimalPerf.default	398	388	1.03:1
TooltipMinimalPerf.default	1165	1132	1.03:1
AvatarMinimalPerf.default	219	215	1.02:1
ChatMinimalPerf.default	737	721	1.02:1
ImageMinimalPerf.default	441	432	1.02:1
LabelMinimalPerf.default	458	448	1.02:1
TreeWith60ListItems.default	210	206	1.02:1
AccordionMinimalPerf.default	184	182	1.01:1
AttachmentSlotsPerf.default	1206	1192	1.01:1
ChatWithPopoverPerf.default	435	431	1.01:1
DialogMinimalPerf.default	831	826	1.01:1
FlexMinimalPerf.default	307	305	1.01:1
MenuMinimalPerf.default	940	927	1.01:1
ProviderMinimalPerf.default	1244	1228	1.01:1
SplitButtonMinimalPerf.default	4793	4733	1.01:1
TableManyItemsPerf.default	2149	2135	1.01:1
TextAreaMinimalPerf.default	599	591	1.01:1
ToolbarMinimalPerf.default	1058	1047	1.01:1
TreeMinimalPerf.default	885	872	1.01:1
DatepickerMinimalPerf.default	5955	5937	1:1
DividerMinimalPerf.default	403	402	1:1
EmbedMinimalPerf.default	4662	4669	1:1
LayoutMinimalPerf.default	409	408	1:1
ListNestedPerf.default	627	624	1:1
MenuButtonMinimalPerf.default	1816	1812	1:1
SkeletonMinimalPerf.default	406	405	1:1
TableMinimalPerf.default	467	465	1:1
CustomToolbarPrototype.default	4499	4483	1:1
ButtonOverridesMissPerf.default	1935	1945	0.99:1
CheckboxMinimalPerf.default	2954	2984	0.99:1
DropdownMinimalPerf.default	3385	3420	0.99:1
FormMinimalPerf.default	474	478	0.99:1
GridMinimalPerf.default	390	395	0.99:1
InputMinimalPerf.default	1429	1442	0.99:1
SliderMinimalPerf.default	1837	1857	0.99:1
BoxMinimalPerf.default	392	399	0.98:1
ButtonMinimalPerf.default	196	201	0.98:1
HeaderSlotsPerf.default	860	878	0.98:1
LoaderMinimalPerf.default	755	771	0.98:1
ProviderMergeThemesPerf.default	1808	1850	0.98:1
TextMinimalPerf.default	398	406	0.98:1
ButtonSlotsPerf.default	623	641	0.97:1
ReactionMinimalPerf.default	426	445	0.96:1
RosterPerf.default	1302	1366	0.95:1
AlertMinimalPerf.default	305	324	0.94:1
VideoMinimalPerf.default	690	731	0.94:1

codesandbox-ci · 2021-10-26T13:37:47Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 446c1d7:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

ling1726 · 2021-10-26T16:32:54Z

packages/make-styles/src/runtime/compileCSS.test.ts

+  });
+
+  it('handles spacing', () => {
+    expect(normalizePseudoSelector(' :hover')).toMatchInlineSnapshot(`"& :hover"`);


just curious, does the spacing have any impact on the final result ?

https://codesandbox.io/s/react-css-in-js-with-fela-forked-2xo8f?file=/src/components/C.css

Yes, check this one (hover text) 👆

ling1726 · 2021-10-26T16:33:23Z

packages/make-styles/src/runtime/compileCSS.test.ts

+    expect(normalizePseudoSelector(' :hover')).toMatchInlineSnapshot(`"& :hover"`);
+  });
+
+  it('handles multiple pseudos', () => {


This test has the same name as the one under it

Suggested change

it('handles multiple pseudos', () => {

it('handles comma separated pseudos', () => {

ling1726 · 2021-10-26T16:34:15Z

packages/make-styles/src/runtime/compileCSS.test.ts

+    expect(normalizePseudoSelector(':focus:hover')).toMatchInlineSnapshot(`"&:focus:hover"`);
+  });
+
+  it('handles multiple with comma', () => {


this test seems like just a combination of those above.... is it necessary ?

They are kinda same, but not the same 😅 Anyway I merged the into a single it 👍

* fix(makeStyles): handle comma separated selectors * Change files * fix fmt * add comments * update tests

layershifter added 3 commits October 26, 2021 14:28

fix(makeStyles): handle comma separated selectors

fd42c75

Change files

784ba91

fix fmt

dd092c3

msft-fluent-ui-bot assigned bsunderhus Oct 26, 2021

layershifter added this to In Progress in react-components beta via automation Oct 26, 2021

layershifter added Area: JS Styling Fluent UI react-components (v9) labels Oct 26, 2021

add comments

e6b8706

layershifter marked this pull request as ready for review October 26, 2021 13:01

layershifter requested a review from a team as a code owner October 26, 2021 13:01

khmakoto approved these changes Oct 26, 2021

View reviewed changes

ling1726 reviewed Oct 26, 2021

View reviewed changes

ling1726 approved these changes Oct 26, 2021

View reviewed changes

update tests

446c1d7

layershifter enabled auto-merge (squash) October 26, 2021 17:23

layershifter merged commit c6bc2b4 into master Oct 26, 2021

react-components beta automation moved this from In Progress to Done Oct 26, 2021

layershifter deleted the fix/mk-comma-selectors branch November 8, 2021 10:51

mlp73 pushed a commit to mlp73/fluentui that referenced this pull request Jan 17, 2022

fix(makeStyles): handle comma separated selectors (microsoft#20348)

99774c4

* fix(makeStyles): handle comma separated selectors * Change files * fix fmt * add comments * update tests

layershifter mentioned this pull request Apr 26, 2022

Multiple selectors are not properly scoped microsoft/griffel#105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(makeStyles): handle comma separated selectors #20348

fix(makeStyles): handle comma separated selectors #20348

layershifter commented Oct 26, 2021 •

edited

fabricteam commented Oct 26, 2021 •

edited

size-auditor bot commented Oct 26, 2021 •

edited

fabricteam commented Oct 26, 2021 •

edited

codesandbox-ci bot commented Oct 26, 2021 •

edited

ling1726 Oct 26, 2021

layershifter Oct 26, 2021

ling1726 Oct 26, 2021

ling1726 Oct 26, 2021

layershifter Oct 26, 2021

	it('can merge comma delimitted selectors correctly', () => {
	mergeStyles(
	{ selectors: { ':hover': { background: 'red' } } },
	{ selectors: { ':hover, :active': { background: 'blue' } } },
	);
	expect(_stylesheet.getRules()).toEqual('.css-0:hover{background:blue;}.css-0:active{background:blue;}');
	});

	it('handles multiple pseudos', () => {
	it('handles comma separated pseudos', () => {

fix(makeStyles): handle comma separated selectors #20348

fix(makeStyles): handle comma separated selectors #20348

Conversation

layershifter commented Oct 26, 2021 • edited

Pull request checklist

Description of changes

fabricteam commented Oct 26, 2021 • edited

📊 Bundle size report

size-auditor bot commented Oct 26, 2021 • edited

Asset size changes

fabricteam commented Oct 26, 2021 • edited

Perf Analysis (@fluentui/react)

Perf Analysis (@fluentui/react-northstar)

codesandbox-ci bot commented Oct 26, 2021 • edited

ling1726 Oct 26, 2021

Choose a reason for hiding this comment

layershifter Oct 26, 2021

Choose a reason for hiding this comment

ling1726 Oct 26, 2021

Choose a reason for hiding this comment

ling1726 Oct 26, 2021

Choose a reason for hiding this comment

layershifter Oct 26, 2021

Choose a reason for hiding this comment

layershifter commented Oct 26, 2021 •

edited

fabricteam commented Oct 26, 2021 •

edited

size-auditor bot commented Oct 26, 2021 •

edited

fabricteam commented Oct 26, 2021 •

edited

Perf Analysis (`@fluentui/react`)

Perf Analysis (`@fluentui/react-northstar`)

codesandbox-ci bot commented Oct 26, 2021 •

edited