RFC: Customizing application of native element props and ref

[ecraig12345]

Alternate version of #18804 with a slightly different approach, and broadening the focus beyond input components in some aspects. This came after a discussion in the Redmond components crew sync and incorporates various people's feedback from the previous PR.

EDIT: removed description because it was very outdated--see the final text and the discussion for details

[codesandbox-ci]

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 511c32a:

Sandbox	Source
Fluent UI React Starter	Configuration

[size-auditor]

Asset size changes

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

Baseline commit: 1e5b888607626cae4ff69b5b9055bf8b9537c45b (build)

[fabricteam]

📊 Bundle size report

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
react-accordion Accordion (including children components)	55.248 kB 17.403 kB
react-avatar Avatar	56.558 kB 15.66 kB
react-badge Badge	24.343 kB 7.165 kB
react-badge CounterBadge	27.156 kB 7.851 kB
react-badge PresenseBadge	237 B 177 B
react-button Button	22.932 kB 6.984 kB
react-button CompoundButton	28.215 kB 7.834 kB
react-button MenuButton	24.733 kB 7.546 kB
react-button ToggleButton	32.527 kB 7.601 kB
react-components react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	163.935 kB 46.761 kB
react-components react-components: FluentProvider & webLightTheme	35.75 kB 11.392 kB
react-divider Divider	15.889 kB 5.747 kB
react-image Image	9.882 kB 3.975 kB
react-input Input	31.636 kB 11.312 kB
react-label Label	9.397 kB 3.839 kB
react-link Link	12.609 kB 4.948 kB
react-make-styles makeStaticStyles (runtime)	7.59 kB 3.321 kB
react-make-styles makeStyles + mergeClasses (runtime)	22.135 kB 8.356 kB
react-make-styles makeStyles + mergeClasses (build time)	2.557 kB 1.202 kB
react-menu Menu (including children components)	102.963 kB 31.313 kB
react-menu Menu (including selectable components)	105.239 kB 31.665 kB
react-popover Popover	100.411 kB 30.075 kB
react-portal Portal	6.725 kB 2.237 kB
react-positioning usePopper	23.145 kB 7.942 kB
react-provider FluentProvider	15.748 kB 5.773 kB
react-slider Slider	30.621 kB 9.711 kB
react-switch Switch	18.067 kB 6.181 kB
react-text Text - Default	11.798 kB 4.452 kB
react-text Text - Wrappers	15.414 kB 4.734 kB
react-theme Teams: all themes	32.941 kB 6.674 kB
react-theme Teams: Light theme	20.247 kB 5.662 kB
react-tooltip Tooltip	46.029 kB 15.655 kB
react-utilities SSRProvider	213 B 170 B

_{🤖 This report was generated against 1e5b888607626cae4ff69b5b9055bf8b9537c45b}

[fabricteam]

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
Avatar	mount	1034	1029	5000
BaseButton	mount	1038	1043	5000
Breadcrumb	mount	2917	2856	1000
ButtonNext	mount	512	516	5000
Checkbox	mount	1741	1697	5000
CheckboxBase	mount	1520	1504	5000
ChoiceGroup	mount	5544	5520	5000
ComboBox	mount	1063	1115	1000
CommandBar	mount	11382	11289	1000
ContextualMenu	mount	7263	7249	1000
DefaultButton	mount	1328	1330	5000
DetailsRow	mount	4311	4313	5000
DetailsRowFast	mount	4329	4324	5000
DetailsRowNoStyles	mount	4141	4078	5000
Dialog	mount	2699	2739	1000
DocumentCardTitle	mount	174	147	1000
Dropdown	mount	3717	3686	5000
FluentProviderNext	mount	7478	7670	5000
FluentProviderWithTheme	mount	383	392	10
FluentProviderWithTheme	virtual-rerender	95	109	10
FluentProviderWithTheme	virtual-rerender-with-unmount	510	542	10
FocusTrapZone	mount	2149	2088	5000
FocusZone	mount	2037	2083	5000
IconButton	mount	2079	2065	5000
Label	mount	378	398	5000
Layer	mount	3457	3504	5000
Link	mount	530	583	5000
MakeStyles	mount	2093	2096	50000
MenuButton	mount	1725	1764	5000
MessageBar	mount	2296	2326	5000
Nav	mount	3778	3785	1000
OverflowSet	mount	1235	1196	5000
Panel	mount	2578	2557	1000
Persona	mount	937	957	1000
Pivot	mount	1602	1577	1000
PrimaryButton	mount	1430	1485	5000
Rating	mount	8845	9048	5000
SearchBox	mount	1494	1550	5000
Shimmer	mount	2938	2933	5000
Slider	mount	2278	2209	5000
SpinButton	mount	5464	5572	5000
Spinner	mount	455	468	5000
SplitButton	mount	3914	3530	5000
Stack	mount	574	604	5000
StackWithIntrinsicChildren	mount	1859	1837	5000
StackWithTextChildren	mount	5300	5324	5000
SwatchColorPicker	mount	11253	11309	5000
Tabs	mount	1555	1565	1000
TagPicker	mount	2960	2985	5000
TeachingBubble	mount	14346	14423	5000
Text	mount	470	499	5000
TextField	mount	1521	1591	5000
ThemeProvider	mount	1301	1285	5000
ThemeProvider	virtual-rerender	641	619	5000
ThemeProvider	virtual-rerender-with-unmount	2087	2068	5000
Toggle	mount	928	889	5000
buttonNative	mount	120	124	5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
ChatDuplicateMessagesPerf.default	360	325	1.11:1
AttachmentMinimalPerf.default	190	172	1.1:1
BoxMinimalPerf.default	412	374	1.1:1
ButtonMinimalPerf.default	199	181	1.1:1
ListWith60ListItems.default	790	716	1.1:1
CarouselMinimalPerf.default	547	503	1.09:1
ChatWithPopoverPerf.default	463	425	1.09:1
FormMinimalPerf.default	516	480	1.08:1
AnimationMinimalPerf.default	489	456	1.07:1
ImageMinimalPerf.default	458	427	1.07:1
ListMinimalPerf.default	631	587	1.07:1
HeaderMinimalPerf.default	427	402	1.06:1
TableMinimalPerf.default	471	443	1.06:1
TreeWith60ListItems.default	210	198	1.06:1
InputMinimalPerf.default	1448	1381	1.05:1
ListNestedPerf.default	663	632	1.05:1
MenuButtonMinimalPerf.default	1947	1852	1.05:1
TextMinimalPerf.default	412	392	1.05:1
LayoutMinimalPerf.default	422	407	1.04:1
ListCommonPerf.default	761	734	1.04:1
LoaderMinimalPerf.default	825	797	1.04:1
RefMinimalPerf.default	264	254	1.04:1
TableManyItemsPerf.default	2273	2180	1.04:1
ChatMinimalPerf.default	770	751	1.03:1
CheckboxMinimalPerf.default	3121	3034	1.03:1
DatepickerMinimalPerf.default	6216	6028	1.03:1
DividerMinimalPerf.default	431	420	1.03:1
PortalMinimalPerf.default	193	188	1.03:1
ReactionMinimalPerf.default	446	431	1.03:1
SliderMinimalPerf.default	1852	1791	1.03:1
DialogMinimalPerf.default	856	838	1.02:1
MenuMinimalPerf.default	988	968	1.02:1
PopupMinimalPerf.default	674	658	1.02:1
StatusMinimalPerf.default	773	760	1.02:1
TextAreaMinimalPerf.default	601	588	1.02:1
AlertMinimalPerf.default	308	304	1.01:1
DropdownManyItemsPerf.default	785	775	1.01:1
CustomToolbarPrototype.default	4304	4248	1.01:1
TooltipMinimalPerf.default	1183	1172	1.01:1
TreeMinimalPerf.default	947	934	1.01:1
AvatarMinimalPerf.default	227	227	1:1
ButtonOverridesMissPerf.default	1889	1896	1:1
LabelMinimalPerf.default	458	459	1:1
ProviderMergeThemesPerf.default	1818	1812	1:1
RadioGroupMinimalPerf.default	537	538	1:1
SplitButtonMinimalPerf.default	4746	4757	1:1
DropdownMinimalPerf.default	3332	3364	0.99:1
FlexMinimalPerf.default	316	320	0.99:1
GridMinimalPerf.default	390	393	0.99:1
SkeletonMinimalPerf.default	414	418	0.99:1
IconMinimalPerf.default	716	720	0.99:1
ToolbarMinimalPerf.default	1093	1104	0.99:1
EmbedMinimalPerf.default	4619	4704	0.98:1
ProviderMinimalPerf.default	1126	1146	0.98:1
ButtonSlotsPerf.default	623	645	0.97:1
HeaderSlotsPerf.default	870	901	0.97:1
ItemLayoutMinimalPerf.default	1423	1460	0.97:1
RosterPerf.default	1350	1392	0.97:1
AttachmentSlotsPerf.default	1196	1244	0.96:1
SegmentMinimalPerf.default	387	405	0.96:1
AccordionMinimalPerf.default	174	183	0.95:1
CardMinimalPerf.default	640	671	0.95:1
VideoMinimalPerf.default	742	790	0.94:1

[bsunderhus]

IMO this is not ideal and also not intuitive, and I don't see which big impact would it make in layout if this wasn't adopted.

[ecraig12345]

I think the scenario for this was if someone wanted to apply styling (for example flex properties) to fields within a form. Then they would probably expect the top-level className to go to the top-level element.

[bsunderhus]

makeMergeProps will be deprecated after as prop simplification RFC

[ecraig12345]

Ah okay, I was wondering about that after trying to copy some of the Accordion code in Input and noticing makeMergeProps was gone. Where would this go instead?

[bsunderhus]

Right now this is done manually on state construction as you can verify on Accordion code. Biggest change from as prop simplification RFC is the usage of a more explicit approach for state declaration, it becomes dev responsibility to pass ref and other props to the proper slot.

[bsunderhus]

IMO this RFC makes root and slots even more similar, which is compatible with the proposal from RFC #19068, the 2 biggest changes being:

1. making root available on props
2. creating the notion of primary slot which allows to decide which slot is the one that has it's properties flattened on components properties

I believe that this changes are valid and makes slots implementation even more generic and explicit.

[layershifter]

@ecraig12345 is this ready for review or do you plan to update it?

[ecraig12345]

@layershifter It's not ready for review. I need to update it but haven't gotten to do it yet.

[miroslavstastny]

Should we special-case className and style to always go on the root slot?

No.
From my experience there is no correct way how to do props splitting correctly - you will always be able to find an edge case showing the current split is not 100% - see #18983 (comment) from @layershifter.
Instead of having magic which works in 95% of the cases but fails in the remaining 5% it is better to require developer to choose explicitly.

Northstar v0 put className on the outermost element

Unfortunately, Northstar is not that consistent, MenuItem renders li > a and className goes to the a

the assumption that users would expect styling to be applied to the entire control

There are different use cases - for positioning the control inside a flex, you need to style the outermost element.
But another use case is you need to change border-radius of the input - that needs to happed on the inner element.

I took a look at a few libraries and made a table of their behaviors here

This is the right approach how we should make decisions, thank you very much for that 👍

My main concern is not about where Input props should be applied to. It is about the impact of this decision on all other components. Let's say we have 50 components, the problem with the wrapper applies to Input, Checkbox, possibly Image, Popup?
But for the remaining 46 components, we are adding the problematic props vs root slot schizophrenia.

For Input let's do whatever we believe is the best - apply the props to Input or the outermost element (or do the split), add root or wrapper or input slot.
But please, for single element components let's keep just one way how to do things:

<Button className="only-here" />

[behowell]

@layershifter:

I took a look at a few libraries and made a table of their behaviors here: #18804 (comment)

Honestly, there is not enough data to make a decision based on it: once we will remove Fluent UI flavors from the table, it's 50/50.

I agree that table wasn't really enough to get a full picture. So I did a larger survey of components in a number of react-based libraries. I found that a large majority follow the rule "id/name goes to primary slot, and class goes to root slot".
The results are here: https://hackmd.io/r3ii_hFuSBGQAROL5HWGaw?view

Also for what it's worth, we may want to weight FluentUI flavors more heavily, since that's what many of our users will be upgrading from. Option C does make the upgrade scenario more difficult, as it changes the behavior in a way that can't be caught by build breaks or warnings. We could likely write a codemod to help with that though.

[ling1726]

I propose an Option C:

Option C works and gets my vote 👍, and gives instant compatibility to form validation libraries. I just see no point in calling it root since we already use this terminology consistently across v9 as the root DOM element. IMO any kind of special knowledge for root in a specific scenario a step in the wrong direction. We can obviously change that without a breaking change, but it effectively introduces mental effort to figure out again how to make inputs work 😟

Let's just call this wrapper (😅) so that we can consistently ship a pattern for this kind of problem which is still unsolved for every react framework that uses inputs. This will be aligned in the common approach of 'do something special for inputs because we need to be compatible with form validation libs'

[ecraig12345]

Thanks everyone for the input! I'll give a more detailed response later but quick FYI about @behowell's comparison table: note that similar to what @miroslavstastny said about @fluentui/react-northstar, @fluentui/react is also wildly inconsistent about whether DOM root or "primary" element gets the top-level native props such as className--maybe roughly 50/50 split. 😬 And then there's an awkward rootProps or inputProps to handle the other one, and lots of related github issues.

[behowell]

I'm proposing Option D based on a meeting including @ecraig12345, @miroslavstastny, @levithomason and others:

Start with Option C, with the following changes:

• className and style are always put on the root slot, even if another slot is a primary slot.
  • For example: <Checkbox id="myId" className="myClass"> -> <div class="myClass"><input id="myId" /></div>
• There is a prop for the primary slot, only when root is not the primary slot.
  • For example, Checkbox would have an input prop for its primary slot, and a root prop for its root slot.
  • Button would not have root or button props, since root is its primary slot.
• If the same prop is specified on both the component and the primary slot prop, then the slot prop wins. (This is somewhat arbitrary, and we do not recommend that users rely on this, but we need to pick one and document it.)
  • For example: <Checkbox id="foo" input={{ id: 'bar' }}> -> "bar" wins: <div><input id="bar" /></div>

Some background on the reasoning:

• All of the options have some sort of inconsistency, but putting className on the root is likely what most people would expect.
  • The vast majority of component libraries I could find put className on the root of a component, notably including FluentUI v8 and Northstar v0. See my comment above for more info.
  • Follows the principle of least surprise.
• We need a prop for the primary slot when it is not the root slot because we need a way to add className/style to the primary slot. But, we don't want to overcomplicate simple components: we don't want to expose a root prop in cases where root is the primary slot because it is entirely redundant and unnecessary.

[ling1726]

I'm proposing Option D based on a meeting including @ecraig12345, @miroslavstastny, @levithomason and others:

Start with Option C, with the following changes:

• className and style are always put on the root slot, even if another slot is a primary slot.

  • For example: <Checkbox id="myId" className="myClass"> -> <div class="myClass"><input id="myId" /></div>

• There is a prop for the primary slot, only when root is not the primary slot.

  • For example, Checkbox would have an input prop for its primary slot, and a root prop for its root slot.
  • Button would not have root or button props, since root is its primary slot.

• If the same prop is specified on both the component and the primary slot prop, then the slot prop wins. (This is somewhat arbitrary, and we do not recommend that users rely on this, but we need to pick one and document it.)

  • For example: <Checkbox id="foo" input={{ id: 'bar' }}> -> "bar" wins: <div><input id="bar" /></div>

Some background on the reasoning:

• All of the options have some sort of inconsistency, but putting className on the root is likely what most people would expect.

  • The vast majority of component libraries I could find put className on the root of a component, notably including FluentUI v8 and Northstar v0. See my comment above for more info.
  • Follows the principle of least surprise.

• We need a prop for the primary slot when it is not the root slot because we need a way to add className/style to the primary slot. But, we don't want to overcomplicate simple components: we don't want to expose a root prop in cases where root is the primary slot because it is entirely redundant and unnecessary.

I think we are kind of bound by our slot approach to be as consistent as possible. Other libraries do not expose a way to attempt to style or apply attributes to almost ever DOM element slot of a component. The circumstances really change for Fluent IMO because we are actually selling our slot architecture in this way.

Let's look at evergreen's checkbox:

<label @class @style @data>
  <input type="checkbox" @id @name />
  <div />
  <span />
</label>

It is just a mix and match of attributes, and styling the spanor input requires some weird mix of css selectors. Adding any aria-* to the checkbox results in adding it to the <label> tag.

I think we need consistency becuase we are actively pushing slots as a part of our general architecture. Therefore my vote is still on option C

[layershifter]

My vote is still on C.

---

IMO we should be consistent and props should go to the same slot:

// "id" & "className" should go to the same slot
<Input id="bar" className="foo" />

• no need to add input slot
• no need to explain why something goes there and something else goes there

[dzearing]

My 2c;

Agree with @behowell above; className and style of a component should always affect the root. Predictability is key. When you use the components, you need to position them in a flexbox or target a grid cell name and you will intuitively create a class and stick it on the component. Same goes for style. In v8 and below, we mostly followed this rule... with the exception of a few cases where className ended up going to an element inside the root, and it completely confused people. Really want to avoid repeating that mistake.

Beyond those 2 props, ideally 99% of the other top level props "do the right thing" and you only need to break into slot props in a very very rare occasion. I think Option C (with the exceptions) should lead to this. You can style the <Input/> like you expect, but also throwing a name or id on it for form scenarios will also do the right thing.

[GeoffCoxMSFT]

One way that I've used Fluent components in several applications is along with StyledComponents and Emotion. I extended or modified the style of a Fluent component using the style extension mechanism.

const StyledLink = styled(Link)`
  color: palevioletred;
  font-weight: bold;
`;

My expectation is that the styles apply to the root element of the component. In the checkbox example, If I applied a margin of 4px and this was applied to the input - moving the bookends away from the input part - I would be very surprised.

When using StyledComponents with Fluent, I often examine the output DOM of a component and then apply selectors to update the style of child parts. Sometimes this doesn't work because the default part styles override. I then either need to create a styles object to style that part directly, or apply important to my style.

Whichever solution is chosen, I think it should support the expected behavior of developers working with other styling libraries.

[miroslavstastny]

@dzearing's argument re styling is definitely valid.

But also

Predictability is key.

Once we will do prop splitting we need to add slots for both root and input to be able to override the split - and this "three names for the two places" and "which one wins?" is what I see as breaking the predictability.

So styles+className go always to the outermost element versus never do automatic prop splitting?

I still prefer the latter but if quorum prefers the styles, I am ok with D:
#18983 (comment)

[ecraig12345]

@miroslavstastny @ling1726 @bsunderhus Thanks for the approvals! Just to be 100% clear, which option are you approving?

[behowell]

I've updated the RFC to reflect the Option D proposal from above.
@miroslavstastny @ling1726 @bsunderhus - could you take a look at the new Proposed Design and make sure it still aligns with what you approved? Thanks!

[bsunderhus]

As @miroslavstastny exposed, we're still more inclined to the never do prop splitting approach (Option C).

We have the feeling that this discussion is kind of stuck and none of the the parties are coming to an agreement, so if majority prefers Option D, let's go with Option D and simply get it done 💪🏼, we can always come back to this discussion latter

[miroslavstastny]

Approving the current proposed design (aka Option D)