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.

Sign up for GitHub

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: use browser :focus-visible if supported #24195

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

feat: use browser :focus-visible if supported #24195

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e851bd1
feat: use browser `:focus-visible` if supported
ling1726 Aug 3, 2022
56acea2
changefile
ling1726 Aug 3, 2022
eb138db
Add SSR checks
ling1726 Aug 3, 2022
a46af33
mark as internal
ling1726 Aug 3, 2022
9f8338b
update md
ling1726 Aug 3, 2022
1d9a1eb
Update packages/react-components/react-tabster/src/hooks/useFocusVisi…
ling1726 Aug 3, 2022
aae3965
Update packages/react-components/react-tabster/src/hooks/useFocusVisi…
ling1726 Aug 3, 2022
7764376
Update packages/react-components/react-tabster/src/hooks/useFocusWith…
ling1726 Aug 3, 2022
bc2f8cb
Update packages/react-components/react-tabster/src/hooks/useFocusWith…
ling1726 Aug 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@fluentui-react-tabster-8d4667d2-30a7-4c68-bbcc-7b6331016905.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "feat: use browser `:focus-visible` if supported",
"packageName": "@fluentui/react-tabster",
"email": "[email protected]",
"dependentChangeType": "patch"
}
8 changes: 6 additions & 2 deletions packages/react-components/react-tabster/etc/react-tabster.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,14 @@ export const useFocusFinders: () => {
findPrevFocusable: (currentElement: HTMLElement, options?: Pick<Types.FindNextProps, 'container'>) => HTMLElement | null | undefined;
};

// @public (undocumented)
// Warning: (ae-internal-missing-underscore) The name "useFocusVisible" should be prefixed with an underscore because the declaration is marked as @internal
//
// @internal (undocumented)
export function useFocusVisible<TElement extends HTMLElement = HTMLElement>(): React_2.RefObject<TElement>;

// @public
// Warning: (ae-internal-missing-underscore) The name "useFocusWithin" should be prefixed with an underscore because the declaration is marked as @internal
//
// @internal
export function useFocusWithin<TElement extends HTMLElement = HTMLElement>(): React_2.RefObject<TElement>;

// @public
Expand Down
2 changes: 2 additions & 0 deletions packages/react-components/react-tabster/src/focus/createCustomFocusIndicatorStyle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export const createCustomFocusIndicatorStyle = (
[`:global(.fui-FluentProvider)`]: {
[`& .${FOCUS_VISIBLE_CLASS}`]: style,
},
Copy link
Contributor

@jspurlin jspurlin Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be wrapped in a @ supports not selector(:focus-visible) block so that it's easier for the browser to know they don't have to try and match this style if :focus-visible is supported?


':focus-visible': style,
}),
...(selector === 'focus-within' && {
[`:global(.fui-FluentProvider)`]: {
Expand Down
7 changes: 5 additions & 2 deletions packages/react-components/react-tabster/src/focus/focusVisiblePolyfill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ type HTMLElementWithFocusVisibleScope = {
} & HTMLElement;

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

Expand Down Expand Up @@ -103,3 +102,7 @@ function alreadyInScope(el: HTMLElement | null | undefined): boolean {

return alreadyInScope(el?.parentElement);
}

function browserSupportsFocusVisible() {
return CSS.supports('selector(:focus-visible)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to hear from @mshoho that we can use focus-visible in supported browsers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miroslavstastny focus-visible isn't fully sufficient. It cannot be triggered programmatically. And keyborg has things like detecting if the focus is moved by screen readers (and consequently enabling the keyboard navigation mode).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is focusVisible option: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus
but: https://caniuse.com/?search=focusVisible

}
7 changes: 6 additions & 1 deletion packages/react-components/react-tabster/src/hooks/useFocusVisible.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import * as React from 'react';
import { canUseDOM } from '@fluentui/react-utilities';
import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts';
import { applyFocusVisiblePolyfill } from '../focus/focusVisiblePolyfill';

/**
* @internal
* @returns ref to the container element whose children have `:focus-visible` styles
*/
export function useFocusVisible<TElement extends HTMLElement = HTMLElement>() {
const { targetDocument } = useFluent();
const scopeRef = React.useRef<TElement>(null);

React.useEffect(() => {
if (targetDocument?.defaultView && scopeRef.current) {
if (targetDocument?.defaultView && scopeRef.current && canUseDOM()) {
return applyFocusVisiblePolyfill(scopeRef.current, targetDocument.defaultView);
}
}, [scopeRef, targetDocument]);
Expand Down
4 changes: 3 additions & 1 deletion packages/react-components/react-tabster/src/hooks/useFocusWithin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import * as React from 'react';
import { canUseDOM } from '@fluentui/react-utilities';
import { useFluent_unstable as useFluent } from '@fluentui/react-shared-contexts';
import { applyFocusWithinPolyfill } from '../focus/focusWithinPolyfill';

/**
* @internal
* A ponyfill that allows `:focus-within` to support visibility based on keyboard/mouse navigation
* like `:focus-visible` https://github.com/WICG/focus-visible/issues/151
* @returns ref to the element that uses `:focus-within` styles
Expand All @@ -12,7 +14,7 @@ export function useFocusWithin<TElement extends HTMLElement = HTMLElement>() {
const elementRef = React.useRef<TElement>(null);

React.useEffect(() => {
if (targetDocument?.defaultView && elementRef.current) {
if (targetDocument?.defaultView && elementRef.current && canUseDOM()) {
return applyFocusWithinPolyfill(elementRef.current, targetDocument.defaultView);
}
}, [elementRef, targetDocument]);
Expand Down