-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: improve focus selector perf with focus-visible and focus-within polyfill #24154
feat: improve focus selector perf with focus-visible and focus-within polyfill #24154
Conversation
Adds a polyfill for `:focus-visible` that is built on top of Keyborg that serves as a replacement for `useKeyboardNavAttribute`
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: f11b365e857e7f01879f0a597c2496821a844ada (build) |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 1288 | 1274 | 5000 | |
Button | mount | 923 | 920 | 5000 | |
FluentProvider | mount | 1494 | 1485 | 5000 | |
FluentProviderWithTheme | mount | 564 | 576 | 10 | |
FluentProviderWithTheme | virtual-rerender | 537 | 539 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 570 | 571 | 10 | |
MakeStyles | mount | 1978 | 1983 | 50000 | |
SpinButton | mount | 2385 | 2425 | 5000 |
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 d5aae20:
|
packages/react-components/react-provider/src/components/FluentProvider/useFluentProvider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any special |
packages/react-components/react-portal/src/components/Portal/usePortalMountNode.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-provider/src/components/FluentProvider/useFluentProvider.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-tabster/src/focus/createCustomFocusIndicatorStyle.ts
Show resolved
Hide resolved
packages/react-components/react-tabster/src/focus/createCustomFocusIndicatorStyle.ts
Show resolved
Hide resolved
@@ -69,7 +70,10 @@ export const useCheckbox_unstable = (props: CheckboxProps, ref: React.Ref<HTMLIn | |||
}, | |||
root: resolveShorthand(props.root, { | |||
required: true, | |||
defaultProps: nativeProps.root, | |||
defaultProps: { | |||
ref: useFocusWithin<HTMLSpanElement>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The story with createCustomFocusIndicatorStyle()
& useFocusWithin()
looks a bit dangerous for me as the dependency on useFocusWithin()
is implicit. Do you have something on your mind to make it explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I am not happy at all that this focus-within
requires an extra hook, but globally there is no way to know what parent of the focused element should have this class.
The 'safety' mechanism currently is that createCustomFocusIndicatorStyles
needs explicit focus-within
selector to be used. The styles make sure that there is no any focus styles are applied unless useFocusWithin
is called.
If anyone uses :focus-within
directly (I think inputs do it) then they should know that there is no keyboard/mouse heuristic there.
If you have any proposals, I'd be happy to hear them, I'm not 100% happy with the current situation either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good proposal, the one that I have will be a regression to the previous behavior in terms of performance...
change/@fluentui-react-switch-504a98f0-bdbd-4377-959c-1dcb2275040a.json
Outdated
Show resolved
Hide resolved
<Boring part> Currently it's more ponyfill rather than a real polyfill. </Boring part> I think that it's a step forward as it improves performance due less complicated selectors, but it's not making us closer to native I suggest to explore how we could use native |
packages/react-components/react-tabster/src/focus/focusVisiblePolyfill.ts
Outdated
Show resolved
Hide resolved
I plan to introduce native support in the next PR since we can't do that unless we first have the fallback ponyfill 🏇 My idea is to bake |
Adds a polyfill for
:focus-visible
that is built on top of Keyborgthat serves as a replacement for
useKeyboardNavAttribute
It should be possible to improve even further but I can't find a way to do chained class selectors in Griffel microsoft/griffel#178
Below are results of the
SelectorStats
trace fromedge://tracing
which measures the time spent per-selector during the matching phase. The comparison was done by comparing selector stats for.fui-focus-visible
(polyfill) vs the current[data-keyboard-nav]
selector.(Lower numbers are better)
[data-keyboard-nav]
on provider root:focus-visible
polyfillSteps to repro:
@fluentui/ssr-tests-v9
:focus-within
polyfillSteps to repro:
@fluentui/ssr-tests-v9
:focus-visible
Improvement in %:focus-within
Improvement in %Addresses #24183