Skip to content
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

Merged
merged 15 commits into from
Aug 3, 2022
12 changes: 4 additions & 8 deletions apps/vr-tests-react-components/src/stories/Link.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@ storiesOf('Link Converged - Rendered as anchor', module)
.hover('.fui-Link')
.snapshot('hover', { cropTo: '.testWrapper' })
// This needs to be added so that the focus outline is shown correctly
.executeScript(
"document.getElementsByClassName('fui-FluentProvider')[0].setAttribute('data-keyboard-nav', true)",
)
.executeScript("document.getElementsByClassName('fui-Link')[0].classList.add('fui-focus-visible')")
.focus('.fui-Link')
.snapshot('focused', { cropTo: '.testWrapper' })
.executeScript("document.getElementsByClassName('fui-FluentProvider')[0].removeAttribute('data-keyboard-nav')")
.executeScript("document.getElementsByClassName('fui-Link')[0].classList.remove('fui-focus-visible')")
.mouseDown('.fui-Link')
.snapshot('pressed', { cropTo: '.testWrapper' })
.mouseUp('.fui-Link')
Expand Down Expand Up @@ -96,12 +94,10 @@ storiesOf('Link Converged - Rendered as button', module)
.hover('.fui-Link')
.snapshot('hover', { cropTo: '.testWrapper' })
// This needs to be added so that the focus outline is shown correctly
.executeScript(
"document.getElementsByClassName('fui-FluentProvider')[0].setAttribute('data-keyboard-nav', true)",
)
.executeScript("document.getElementsByClassName('fui-Link')[0].classList.add('fui-focus-visible')")
.focus('.fui-Link')
.snapshot('focused', { cropTo: '.testWrapper' })
.executeScript("document.getElementsByClassName('fui-FluentProvider')[0].removeAttribute('data-keyboard-nav')")
.executeScript("document.getElementsByClassName('fui-Link')[0].classList.remove('fui-focus-visible')")
.mouseDown('.fui-Link')
.snapshot('pressed', { cropTo: '.testWrapper' })
.mouseUp('.fui-Link')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "refactor: use `useFocusWithin` hook for :focus-within styles",
"packageName": "@fluentui/react-checkbox",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix: use `useFocusVisible` hook for :focus-visible styles",
"packageName": "@fluentui/react-portal",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix: use `useFocusVisible` hook for :focus-visible styles",
"packageName": "@fluentui/react-provider",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "refactor: use `useFocusWithin` hook for :focus-within styles",
"packageName": "@fluentui/react-radio",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "refactor: use `useFocusWithin` hook for :focus-within styles",
"packageName": "@fluentui/react-slider",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "refactor: use `useFocusWithin` hook for :focus-within styles",
"packageName": "@fluentui/react-switch",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "feat: create `:focus-visible` and `:focus-within` polyfills",
"packageName": "@fluentui/react-tabster",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
CircleFilled,
} from '@fluentui/react-icons';
import { Label } from '@fluentui/react-label';
import { useFocusWithin } from '@fluentui/react-tabster';

/**
* Create the state required to render Checkbox.
Expand Down Expand Up @@ -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>(),
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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...

...nativeProps.root,
},
}),
input: resolveShorthand(props.input, {
required: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import {
useThemeClassName_unstable as useThemeClassName,
useFluent_unstable as useFluent,
} from '@fluentui/react-shared-contexts';
import { useKeyboardNavAttribute } from '@fluentui/react-tabster';
import { makeStyles, mergeClasses } from '@griffel/react';
import { useFocusVisible } from '@fluentui/react-tabster';

export type UsePortalMountNodeOptions = {
/**
Expand All @@ -26,6 +26,7 @@ const useStyles = makeStyles({
*/
export const usePortalMountNode = (options: UsePortalMountNodeOptions): HTMLElement | null => {
const { targetDocument, dir } = useFluent();
const focusVisibleRef = useFocusVisible<HTMLDivElement>() as React.MutableRefObject<HTMLElement | null>;

const classes = useStyles();
const themeClassName = useThemeClassName();
Expand All @@ -49,15 +50,14 @@ export const usePortalMountNode = (options: UsePortalMountNodeOptions): HTMLElem

element.classList.add(...classesToApply);
element.setAttribute('dir', dir);
focusVisibleRef.current = element;

return () => {
element.classList.remove(...classesToApply);
element.removeAttribute('dir');
};
}
}, [element, className, dir]);

(useKeyboardNavAttribute() as React.MutableRefObject<HTMLElement>).current = element!;
}, [className, dir, element, focusVisibleRef]);

React.useEffect(() => {
return () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useKeyboardNavAttribute } from '@fluentui/react-tabster';
import { useFocusVisible } from '@fluentui/react-tabster';
import {
ThemeContext_unstable as ThemeContext,
useFluent_unstable as useFluent,
Expand Down Expand Up @@ -58,7 +58,7 @@ export const useFluentProvider_unstable = (
root: getNativeElementProps('div', {
...props,
dir,
ref: useMergedRefs(ref, useKeyboardNavAttribute()),
ref: useMergedRefs(ref, useFocusVisible<HTMLDivElement>()),
}),
};
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Label } from '@fluentui/react-label';
import { getPartitionedNativeProps, resolveShorthand, useId, useMergedEventCallbacks } from '@fluentui/react-utilities';
import { RadioGroupContext } from '../../contexts/RadioGroupContext';
import { useContextSelector } from '@fluentui/react-context-selector';
import { useFocusWithin } from '@fluentui/react-tabster';
import type { RadioProps, RadioState } from './Radio.types';

/**
Expand Down Expand Up @@ -41,7 +42,10 @@ export const useRadio_unstable = (props: RadioProps, ref: React.Ref<HTMLInputEle

const root = resolveShorthand(props.root, {
required: true,
defaultProps: nativeProps.root,
defaultProps: {
ref: useFocusWithin<HTMLSpanElement>(),
...nativeProps.root,
},
});

const input = resolveShorthand(props.input, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react';
import { getPartitionedNativeProps, resolveShorthand, useId } from '@fluentui/react-utilities';
import { useSliderState_unstable } from './useSliderState';
import { SliderProps, SliderState } from './Slider.types';
import { useFocusWithin } from '@fluentui/react-tabster';

export const useSlider_unstable = (props: SliderProps, ref: React.Ref<HTMLInputElement>): SliderState => {
const nativeProps = getPartitionedNativeProps({
Expand Down Expand Up @@ -34,6 +35,7 @@ export const useSlider_unstable = (props: SliderProps, ref: React.Ref<HTMLInputE
root: resolveShorthand(root, {
required: true,
defaultProps: {
ref: useFocusWithin<HTMLDivElement>(),
...nativeProps.root,
},
}),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import { CircleFilled } from '@fluentui/react-icons';
import { Label } from '@fluentui/react-label';
import { useFocusWithin } from '@fluentui/react-tabster';
import { getPartitionedNativeProps, resolveShorthand, useId, useMergedEventCallbacks } from '@fluentui/react-utilities';
import type { SwitchProps, SwitchState } from './Switch.types';

Expand All @@ -25,7 +26,7 @@ export const useSwitch_unstable = (props: SwitchProps, ref: React.Ref<HTMLInputE
const id = useId('switch-', nativeProps.primary.id);

const root = resolveShorthand(props.root, {
defaultProps: nativeProps.root,
defaultProps: { ref: useFocusWithin<HTMLDivElement>(), ...nativeProps.root },
required: true,
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
```ts

import type { GriffelStyle } from '@griffel/react';
import * as React_2 from 'react';
import type { RefObject } from 'react';
import { Types } from 'tabster';

Expand Down Expand Up @@ -66,6 +67,12 @@ export const useFocusFinders: () => {
findPrevFocusable: (currentElement: HTMLElement, options?: Pick<Types.FindNextProps, 'container'>) => HTMLElement | null | undefined;
};

// @public (undocumented)
export function useFocusVisible<TElement extends HTMLElement = HTMLElement>(): React_2.RefObject<TElement>;

// @public
export function useFocusWithin<TElement extends HTMLElement = HTMLElement>(): React_2.RefObject<TElement>;

// @public
export function useKeyboardNavAttribute<E extends HTMLElement>(): RefObject<E>;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
export const KEYBOARD_NAV_ATTRIBUTE = 'data-keyboard-nav' as const;
export const KEYBOARD_NAV_SELECTOR = `:global([${KEYBOARD_NAV_ATTRIBUTE}])` as const;
export const FOCUS_VISIBLE_CLASS = 'fui-focus-visible';
export const FOCUS_WITHIN_CLASS = 'fui-focus-within';
export const defaultOptions = {
style: {},
selector: 'focus',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { KEYBOARD_NAV_SELECTOR, defaultOptions } from './constants';
import { FOCUS_VISIBLE_CLASS, defaultOptions, FOCUS_WITHIN_CLASS } from './constants';
import type { GriffelStyle } from '@griffel/react';

export interface CreateCustomFocusIndicatorStyleOptions {
Expand All @@ -19,5 +19,19 @@ export const createCustomFocusIndicatorStyle = (
':focus': {
outlineStyle: 'none',
},
[`${KEYBOARD_NAV_SELECTOR} :${selector}`]: style,
':focus-visible': {
outlineStyle: 'none',
},
// Remove the `.fui-FluentProvider` global selector once Griffel supports chained global styles
// https://github.com/microsoft/griffel/issues/178
...(selector === 'focus' && {
[`:global(.fui-FluentProvider)`]: {
ling1726 marked this conversation as resolved.
Show resolved Hide resolved
[`& .${FOCUS_VISIBLE_CLASS}`]: style,
},
}),
...(selector === 'focus-within' && {
[`:global(.fui-FluentProvider)`]: {
[`& .${FOCUS_WITHIN_CLASS}:${selector}`]: style,
},
}),
ling1726 marked this conversation as resolved.
Show resolved Hide resolved
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import { KEYBORG_FOCUSIN, KeyborgFocusInEvent, createKeyborg, disposeKeyborg } from 'keyborg';
import { FOCUS_VISIBLE_CLASS } from './constants';

/**
* Because `addEventListener` type override falls back to 2nd definition (evt name is unknown string literal)
* evt is being typed as a base class of MouseEvent -> `Event`.
* This type is used to override `listener` calls to make TS happy
*/
type ListenerOverride = (evt: Event) => void;

type FocusVisibleState = {
/**
* Current element with focus visible in state
*/
current: HTMLElement | undefined;
};

type HTMLElementWithFocusVisibleScope = {
focusVisible: boolean | undefined;
} & HTMLElement;

export function applyFocusVisiblePolyfill(scope: HTMLElement, win: Window): () => void {
if (alreadyInScope(scope)) {
// Focus visible polyfill already applied at this scope
return () => undefined;
}

const state: FocusVisibleState = {
current: undefined,
};

const keyborg = createKeyborg(win);

// When navigation mode changes remove the focus-visible selector
keyborg.subscribe(isNavigatingWithKeyboard => {
if (!isNavigatingWithKeyboard && state.current) {
removeFocusVisibleClass(state.current);
state.current = undefined;
}
});

// Keyborg's focusin event is delegated so it's only registered once on the window
// and contains metadata about the focus event
const keyborgListener = (e: KeyborgFocusInEvent) => {
if (state.current) {
removeFocusVisibleClass(state.current);
state.current = undefined;
}

if (keyborg.isNavigatingWithKeyboard() && isHTMLElement(e.target) && e.target) {
// Griffel can't create chained global styles so use the parent element for now
state.current = e.target;
applyFocusVisibleClass(state.current);
}
};

// Make sure that when focus leaves the scope, the focus visible class is removed
const blurListener = (e: FocusEvent) => {
if (!e.relatedTarget || (isHTMLElement(e.relatedTarget) && !scope.contains(e.relatedTarget))) {
if (state.current) {
removeFocusVisibleClass(state.current);
state.current = undefined;
}
}
};

scope.addEventListener(KEYBORG_FOCUSIN, keyborgListener as ListenerOverride);
scope.addEventListener('focusout', blurListener);
(scope as HTMLElementWithFocusVisibleScope).focusVisible = true;

// Return disposer
return () => {
scope.removeEventListener(KEYBORG_FOCUSIN, keyborgListener as ListenerOverride);
scope.removeEventListener('focusout', blurListener);
delete (scope as HTMLElementWithFocusVisibleScope).focusVisible;
disposeKeyborg(keyborg);
};
}

function applyFocusVisibleClass(el: HTMLElement) {
el.classList.add(FOCUS_VISIBLE_CLASS);
}

function removeFocusVisibleClass(el: HTMLElement) {
el.classList.remove(FOCUS_VISIBLE_CLASS);
}

function isHTMLElement(target: EventTarget | null): target is HTMLElement {
if (!target) {
return false;
}
return Boolean(target && typeof target === 'object' && 'classList' in target && 'contains' in target);
}

function alreadyInScope(el: HTMLElement | null | undefined): boolean {
if (!el) {
return false;
}

if ((el as HTMLElementWithFocusVisibleScope).focusVisible) {
return true;
}

return alreadyInScope(el?.parentElement);
}
Loading