Button: Adding various tests and updating component to include componentRef

[dzearing]

Setting up componentRef focus method, adding tests.

[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 04bce6d:

Sandbox	Source
Fluent UI Button	Configuration

[size-auditor]

Asset size changes

Project	Bundle	Baseline Size	New Size	Difference
office-ui-fabric-react	fluentui-react-next-Button	20.301 kB	20.794 kB	493 bytes
office-ui-fabric-react	fluentui-react-next-ToggleButton	21.512 kB	22.002 kB	490 bytes

 Over Tolerance (1024 B)  Over Baseline  Below Baseline  New   Removed 1 kB = 1000 B

Baseline commit: 7932ae5eef247af9b914e8f05efdaac0b64914f8 (build)

[msft-github-bot]

Perf Analysis

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations	Status
BaseButton	mount	824	778	5000
ButtonNext	mount	506	549	5000
Checkbox	mount	1421	1431	5000
CheckboxBase	mount	1187	1158	5000
CheckboxNext	mount	1454	1469	5000
ChoiceGroup	mount	4587	4538	5000
ComboBox	mount	820	834	1000
CommandBar	mount	7010	7023	1000
ContextualMenu	mount	12329	12244	1000
DefaultButton	mount	1011	1002	5000
DetailsRow	mount	3238	3268	5000
DetailsRowFast	mount	3240	3259	5000
DetailsRowNoStyles	mount	3086	3063	5000
Dialog	mount	1400	1419	1000
DocumentCardTitle	mount	1727	1710	1000
Dropdown	mount	2328	2339	5000
FocusZone	mount	1631	1638	5000
IconButton	mount	1598	1538	5000
Label	mount	317	317	5000
Link	mount	441	417	5000
LinkNext	mount	423	440	5000
MenuButton	mount	1342	1349	5000
Nav	mount	2935	2916	1000
Panel	mount	1351	1357	1000
Persona	mount	782	789	1000
Pivot	mount	1306	1319	1000
PivotNext	mount	1258	1284	1000
PrimaryButton	mount	1124	1138	5000
SearchBox	mount	1169	1160	5000
SearchBoxNext	mount	1212	1213	5000
Slider	mount	1379	1414	5000
SliderNext	mount	1825	1826	5000
Spinner	mount	378	385	5000
SplitButton	mount	2863	2854	5000
Stack	mount	466	500	5000
StackWithIntrinsicChildren	mount	1781	1804	5000
StackWithTextChildren	mount	4554	4552	5000
TagPicker	mount	2587	2521	5000
Text	mount	374	399	5000
TextField	mount	1306	1332	5000
ThemeProvider	mount	2776	2764	5000
ThemeProvider	virtual-rerender	481	490	5000
Toggle	mount	812	758	5000
ToggleNext	mount	784	754	5000
button	mount	102	100	5000

Perf Analysis (Fluent)

Perf comparison

Status	Scenario	Fluent TPI	Fabric TPI	Ratio	Iterations	Ticks
🎯	Avatar.Fluent	0.39	0.43	0.91:1	2000	789
🦄	Button.Fluent	0.09	0.17	0.53:1	5000	467
🔧	Checkbox.Fluent	0.56	0.31	1.81:1	1000	564
🦄	Dialog.Fluent	0.13	0.2	0.65:1	5000	661
🔧	Dropdown.Fluent	3.02	0.43	7.02:1	1000	3020
🔧	Icon.Fluent	0.13	0.04	3.25:1	5000	628
🎯	Image.Fluent	0.07	0.1	0.7:1	5000	336
🔧	Slider.Fluent	1.47	0.33	4.45:1	1000	1467
🔧	Text.Fluent	0.06	0.02	3:1	5000	295
🦄	Tooltip.Fluent	0.09	15.93	0.01:1	5000	453

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
GridMinimalPerf.default	694	569	1.22:1
PortalMinimalPerf.default	113	104	1.09:1
LayoutMinimalPerf.default	342	320	1.07:1
SegmentMinimalPerf.default	302	282	1.07:1
Image.Fluent	336	316	1.06:1
AnimationMinimalPerf.default	353	337	1.05:1
ChatMinimalPerf.default	544	517	1.05:1
TextAreaMinimalPerf.default	419	400	1.05:1
TreeWith60ListItems.default	193	183	1.05:1
AlertMinimalPerf.default	256	247	1.04:1
ButtonSlotsPerf.default	555	536	1.04:1
LabelMinimalPerf.default	346	332	1.04:1
RefMinimalPerf.default	186	179	1.04:1
TreeMinimalPerf.default	802	769	1.04:1
Icon.Fluent	628	603	1.04:1
HeaderSlotsPerf.default	699	677	1.03:1
SplitButtonMinimalPerf.default	3364	3274	1.03:1
AttachmentMinimalPerf.default	134	132	1.02:1
CarouselMinimalPerf.default	408	400	1.02:1
DropdownManyItemsPerf.default	1270	1245	1.02:1
FlexMinimalPerf.default	248	244	1.02:1
InputMinimalPerf.default	956	936	1.02:1
ListCommonPerf.default	847	827	1.02:1
IconMinimalPerf.default	592	578	1.02:1
Text.Fluent	295	289	1.02:1
Tooltip.Fluent	453	443	1.02:1
ChatDuplicateMessagesPerf.default	375	372	1.01:1
DividerMinimalPerf.default	311	308	1.01:1
ItemLayoutMinimalPerf.default	1098	1085	1.01:1
SliderMinimalPerf.default	1495	1486	1.01:1
Avatar.Fluent	789	778	1.01:1
Slider.Fluent	1467	1453	1.01:1
ButtonMinimalPerf.default	150	150	1:1
CheckboxMinimalPerf.default	2514	2503	1:1
DropdownMinimalPerf.default	3062	3067	1:1
LoaderMinimalPerf.default	649	648	1:1
CustomToolbarPrototype.default	3352	3349	1:1
ToolbarMinimalPerf.default	828	828	1:1
Dropdown.Fluent	3020	3032	1:1
AccordionMinimalPerf.default	122	123	0.99:1
AvatarMinimalPerf.default	403	409	0.99:1
HierarchicalTreeMinimalPerf.default	376	379	0.99:1
ListWith60ListItems.default	970	980	0.99:1
MenuButtonMinimalPerf.default	1367	1384	0.99:1
ProviderMergeThemesPerf.default	1748	1758	0.99:1
ProviderMinimalPerf.default	788	798	0.99:1
TableManyItemsPerf.default	1907	1919	0.99:1
CardMinimalPerf.default	471	479	0.98:1
ListMinimalPerf.default	402	411	0.98:1
ReactionMinimalPerf.default	340	348	0.98:1
StatusMinimalPerf.default	569	583	0.98:1
TooltipMinimalPerf.default	673	690	0.98:1
VideoMinimalPerf.default	555	565	0.98:1
Button.Fluent	467	475	0.98:1
EmbedMinimalPerf.default	1649	1696	0.97:1
FormMinimalPerf.default	344	353	0.97:1
ImageMinimalPerf.default	304	315	0.97:1
MenuMinimalPerf.default	731	753	0.97:1
PopupMinimalPerf.default	578	595	0.97:1
RadioGroupMinimalPerf.default	353	365	0.97:1
Dialog.Fluent	661	680	0.97:1
AttachmentSlotsPerf.default	985	1028	0.96:1
BoxMinimalPerf.default	281	293	0.96:1
ListNestedPerf.default	779	809	0.96:1
TextMinimalPerf.default	290	301	0.96:1
Checkbox.Fluent	564	596	0.95:1
ChatWithPopoverPerf.default	409	435	0.94:1
TableMinimalPerf.default	320	346	0.92:1
DialogMinimalPerf.default	662	734	0.9:1
HeaderMinimalPerf.default	315	403	0.78:1

[layershifter]

Hey folks!

Can you please clarify why you have decided to go this approach? As I don't see any benefits from it.

Issue 1: Additional API

It adds an additional API about what customer should know and learn. In the same time there is an existing API with refs and it works. It's also a standard thing even for CSS-in-JS libraries, https://styled-components.com/docs/advanced#refs

At the same time there is no additional value from it as points to the same element:

fluentui/packages/react-button/src/components/Button/ButtonBase.tsx

Line 13 in 4746eb1

<slots.root ref={useMergedRefs(ref, state.buttonRef)} {...slotProps.root}>

Issue 2: Endless story

As customer I would like to have access to other DOM methods like .blur(), .select() (Semantic-Org/Semantic-UI-React#2928) and even .querySelector(). What I should use and why? Are we going to add all DOM methods or guide user to use componentRef & ref?

function App() {
  const ref = React.useRef() // Should I use `.focus()` there?
  const componentRef = React.useRef() // Or there?

  return <Button componentRef={componentRef} ref={ref} />
}

Issue 3: Internal usage

There are cases where we will need refs to call some DOM methods like .focus(). We can't use .findDOMNode() as it will throw a warning in Strict Mode and will make components incompatible with Concurrent Mode of React.

So what we should prefer internally componentRef or ref? If we will prefer a componentRef how we are going to detect that it is supported?

const isComponentRefSupported = !!slots.button.type.IS_FLUENT_COMPONENT
// ??? But we know that this will not work because of HoCs at least

[msft-github-bot]

🎉@fluentui/react-button@v0.6.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Official NPM Release

[dzearing]

Ah communication gap, so I'll give some thorough explanation here, hopefully this will shed more light on the thinking. If by the end you still feel like this is crazy-sauce, feel free to suggest alternatives on how to provide imperative apis when needed without requiring customers reaching into DOM structure and doing things we may want to change later.

At the same time there is no additional value from it as points to the same element:

<slots.root ref={useMergedRefs(ref, state.buttonRef)} {...slotProps.root}>

I don't think you're following it quite right. buttonRef is not the imperative API handle. It is simply a ref, owned by state management, and used in the imperative api surface. ref is the optional ref, provided by the user and forwarded by the forwardRef call, possibly. We can not re-use the ref to gain access to the root element from the state hook as it's optional and we don't own it. So we make a buttonRef to enable us to identify it and use it.

The reasoning for componentRef pattern is this:

• ref should return the root element predictably. This gives you an escape hatch that predictably gains you access to the DOM, so that we can avoid findDOMNode due to deprecation like you called out.
• componentRef should return the abstracted, limited imperative API surface. Typically this is limited to focus, but there are a few other things which are awkward to do declaratively.
• devs should avoid reaching into private DOM element structure to execute an imperative thing. Either it should be a declarative prop, a declarative callback, or if imperative API surface is the right solution, expose a limited api surface for that.

By having componentRef we can provide a predictable way to do things like set focus. That way things like SplitButton or FocusZone, which have special behavior to determine the right focus target, still show up to customers in a consistent, predictable format: provide a ref to componentRef, which is the imperative api handle.

As customer I would like to have access to other DOM methods like .blur(), .select() (Semantic-Org/Semantic-UI-React#2928) and even .querySelector(). What I should use and why? Are we going to add all DOM methods or guide user to use componentRef & ref?

No, we should not expose every possible API as an abstraction on the DOM. We should prioritize declarative ways to expose functionality. If that doesn't make sense (e.g. an Input's setSelectedRange method may be a good candidate for abstracting), we add it to the imperative handle.

We've found limiting and abstracting common imperative APIs is a good thing:

• It creates a consistent, predictable API for accessing common imperative things without reaching into the DOM guts.
• It protects consumers from shifting DOM (either from code changes or from slot/as reconfiguration)
• It gives a window of opportunity to refine behavior (e.g. if focus is called and there are multiple options, we can refine where to go. Or if we want to asyncFocus to avoid layout events, we can do that.)

So what we should prefer internally componentRef or ref? If we will prefer a componentRef how we are going to detect that it is supported?

ref should return the root element, always. This creates symmetry with native element expectations. But using it is an escape hatch to the DOM. Once you're there, it's free for all, no guarantees.

componentRef should return the imperative limited api. Every component should decide if it is required; not everything has an imperative API. I don't think there is a need to detect that it is supported, other than from conformance tests.

And yes, now there are multiple ways to do the same thing, which in principle we want to avoid. You could set focus to a Button by calling componentRef.current.focus() which internally will abstract calling focus on the correct element, or you could go directly to the DOM like ref.current.focus() which breaks as soon as the DOM changes from your original expectations, but also triggers a layout event, and may not be applicable in all states. And, as you move to other components like SplitButton where the DOM changes, instead of focusing via ref.current.firstChild.focus() which is structurally sensitive and error prone, you predictably access the imperative API and call focus() and it will set focus to the right thing. Same with FocusZone, where setting focus must be abstracted because focus should move to the active element (a stateful value), rather than the first focusable child or the root.

componentRef provides a pattern for when imperative APIs are necessary. focus tends to be one that is common, necessary, and should be abstracted for predictability in API surface.

Again, open to alternatives if this doesn't make sense. But I don't think "reach into the DOM" is the right answer for every imperative operation and we should try and avoid that either by having declarative ways to do things, or a recommended and supported and reliable way of imperatively doing things.

[layershifter]

ref should return the root element, always. This creates symmetry with native element expectations. But using it is an escape hatch to the DOM. Once you're there, it's free for all, no guarantees.

componentRef should return the imperative limited api. Every component should decide if it is required; not everything has an imperative API. I don't think there is a need to detect that it is supported, other than from conformance tests.

What we are going to use internally? You mentioned SplitButton as an example, if SplitButton is used as a slot are we going to use ref or componentRef to execute .focus()?

[layershifter]

What is a purpose to have this API for Button? Only to have unified approach for all components?

[levithomason]

Let's have a sync on this one. I think we moved a little too fast both in merging this and in objecting to it. Let's take a look at the use cases we have, refresh ourselves on the latest React docs dealing with refs and the DOM (especially exposing DOM refs to parents and forwardRef), and hop on a call to discuss. Who's in?

Reminders

👉 We discussed this to a decent degree in our last off site as well, notes aren't too telling but here they are:

componentRef currently supports 2 cases unpredictably - will continue to support both cases but with 2 difft APIs: have a way to always return imperative API but it shouldn’t be abused, separately have something that always gets DOM node.

• return native DOM element in Stardust vs rn in Fabric, it’s returning the React instance or be line with hooks - imperative api?
• current Ref in Fluent = findDOMNode

-- https://hackmd.io/8WievoYRQpiAXbP_rBAecA?both

👉 We also have slots, so you can already get a ref to any part of the component, including the root:

const MyComponent = () => {
  const rootRef = React.useRef();
  const iconRef = React.useRef();
  const headerRef = React.useRef();
  const descriptionRef = React.useRef();
  const actionRef = React.useRef();
  const progressRef = React.useRef();

  return (
    <Attachment
      ref={{ ref: rootRef }}
      icon={<MyIcon ref={iconRef} />}
      header={{ ref: headerRef }}
      description={{ ref: descriptionRef }}
      action={{ ref: actionRef }}
      progress={{ ref: progressRef }}
    />
  );
};