-
Notifications
You must be signed in to change notification settings - Fork 1.3k
(RSP-1596 and RSP-1592) Update onFocus and onBlur behavior of FocusScope containment #281
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
Changes from all commits
ef41409
051cf50
419c72f
ad0110d
e4c16fa
71e3785
e8e7ef3
d27b044
1adff4a
28c7c6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ interface FocusManager { | |
| const FocusContext = React.createContext<FocusManager>(null); | ||
|
|
||
| let activeScope: RefObject<HTMLElement[]> = null; | ||
| let scopes: Set<RefObject<HTMLElement[]>> = new Set(); | ||
|
|
||
| // This is a hacky DOM-based implementation of a FocusScope until this RFC lands in React: | ||
| // https://github.com/reactjs/rfcs/pull/109 | ||
|
|
@@ -57,6 +58,10 @@ export function FocusScope(props: FocusScopeProps) { | |
| } | ||
|
|
||
| scopeRef.current = nodes; | ||
| scopes.add(scopeRef); | ||
| return () => { | ||
| scopes.delete(scopeRef); | ||
| }; | ||
| }, [children]); | ||
|
|
||
| useFocusContainment(scopeRef, contain); | ||
|
|
@@ -148,6 +153,7 @@ function useFocusContainment(scopeRef: RefObject<HTMLElement[]>, contain: boolea | |
| let focusedNode = useRef<HTMLElement>(); | ||
|
|
||
| useEffect(() => { | ||
| let scope = scopeRef.current; | ||
| if (!contain) { | ||
| return; | ||
| } | ||
|
|
@@ -159,11 +165,11 @@ function useFocusContainment(scopeRef: RefObject<HTMLElement[]>, contain: boolea | |
| } | ||
|
|
||
| let focusedElement = document.activeElement as HTMLElement; | ||
| if (!isElementInScope(focusedElement, scopeRef.current)) { | ||
| if (!isElementInScope(focusedElement, scope)) { | ||
| return; | ||
| } | ||
|
|
||
| let elements = getFocusableElementsInScope(scopeRef.current, {tabbable: true}); | ||
| let elements = getFocusableElementsInScope(scope, {tabbable: true}); | ||
| let position = elements.indexOf(focusedElement); | ||
| let lastPosition = elements.length - 1; | ||
| let nextElement = null; | ||
|
|
@@ -189,38 +195,55 @@ function useFocusContainment(scopeRef: RefObject<HTMLElement[]>, contain: boolea | |
| }; | ||
|
|
||
| let onFocus = (e) => { | ||
| // If the focused element is in the current scope, and not in the active scope, | ||
| // update the active scope to point to this scope. | ||
| let isInScope = isElementInScope(e.target, scopeRef.current); | ||
| if (isInScope && (!activeScope || !isElementInScope(e.target, activeScope.current))) { | ||
| // If a focus event occurs outside the active scope (e.g. user tabs from browser location bar), | ||
| // restore focus to the previously focused node or the first tabbable element in the active scope. | ||
| let isInAnyScope = isElementInAnyScope(e.target, scopes); | ||
| if (!isInAnyScope) { | ||
| if (focusedNode.current) { | ||
| focusedNode.current.focus(); | ||
| } else if (activeScope) { | ||
| focusFirstInScope(activeScope.current); | ||
| } | ||
| } else { | ||
| e.stopPropagation(); | ||
| activeScope = scopeRef; | ||
| } | ||
|
|
||
| // Save the currently focused node in this scope | ||
| if (isInScope) { | ||
| focusedNode.current = e.target; | ||
| } | ||
| }; | ||
|
|
||
| // If a focus event occurs outside the active scope (e.g. user tabs from browser location bar), | ||
| // restore focus to the previously focused node or the first tabbable element if none. | ||
| if (activeScope === scopeRef && !isInScope) { | ||
| if (focusedNode.current) { | ||
| focusedNode.current.focus(); | ||
| } else { | ||
| focusFirstInScope(scopeRef.current); | ||
| } | ||
| let onBlur = (e) => { | ||
| e.stopPropagation(); | ||
| let isInAnyScope = isElementInAnyScope(e.relatedTarget, scopes); | ||
|
|
||
| if (!isInAnyScope) { | ||
| activeScope = scopeRef; | ||
| focusedNode.current = e.target; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like you could get rid of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using it now for the document level focusin events, thanks for the catch |
||
| focusedNode.current.focus(); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', onKeyDown, false); | ||
| document.addEventListener('focusin', onFocus, false); | ||
| scope.forEach(element => element.addEventListener('focusin', onFocus, false)); | ||
| scope.forEach(element => element.addEventListener('focusout', onBlur, false)); | ||
| return () => { | ||
| document.removeEventListener('keydown', onKeyDown, false); | ||
| document.removeEventListener('focusin', onFocus, false); | ||
| scope.forEach(element => element.removeEventListener('focusin', onFocus, false)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to make sure, but the element's we are looping over, they can't change between effects right? I ask because scope is from a ref, which can change without causing
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lint error previously mentioned this possibility when I had
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. o, yep, that'll do the trick.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think we are ok, tested by adding a div and a button that removes said div inside the Dialog's FocusScope. Removing the div and then closing the dialog didn't trigger any console errors, |
||
| scope.forEach(element => element.removeEventListener('focusout', onBlur, false)); | ||
| }; | ||
| }, [scopeRef, contain]); | ||
| } | ||
|
|
||
| function isElementInAnyScope(element: Element, scopes: Set<RefObject<HTMLElement[]>>) { | ||
| for (let scope of scopes.values()) { | ||
| if (isElementInScope(element, scope.current)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isElementInScope(element: Element, scope: HTMLElement[]) { | ||
| return scope.some(node => node.contains(element)); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import {cleanup, fireEvent, render} from '@testing-library/react'; | ||
| import {FocusScope, useFocusManager} from '../'; | ||
| import React from 'react'; | ||
| import ReactDOM from 'react-dom'; | ||
|
|
||
| describe('FocusScope', function () { | ||
| afterEach(cleanup); | ||
|
|
@@ -252,6 +253,33 @@ describe('FocusScope', function () { | |
| fireEvent.focusIn(outside); | ||
| expect(document.activeElement).toBe(input2); | ||
| }); | ||
|
|
||
| it('should restore focus to the last focused element in the scope on focus out', function () { | ||
| let {getByTestId} = render( | ||
| <div> | ||
| <FocusScope contain> | ||
| <input data-testid="input1" /> | ||
| <input data-testid="input2" /> | ||
| </FocusScope> | ||
| </div> | ||
| ); | ||
|
|
||
| let input1 = getByTestId('input1'); | ||
| let input2 = getByTestId('input2'); | ||
|
|
||
| input1.focus(); | ||
| fireEvent.focusIn(input1); // jsdom doesn't fire this automatically | ||
| expect(document.activeElement).toBe(input1); | ||
|
|
||
| fireEvent.keyDown(document.activeElement, {key: 'Tab'}); | ||
| fireEvent.focusIn(input2); | ||
| expect(document.activeElement).toBe(input2); | ||
|
|
||
| input2.blur(); | ||
| expect(document.activeElement).toBe(document.body); | ||
| fireEvent.focusOut(input2); | ||
| expect(document.activeElement).toBe(input2); | ||
| }); | ||
| }); | ||
|
|
||
| describe('focus restoration', function () { | ||
|
|
@@ -593,4 +621,43 @@ describe('FocusScope', function () { | |
| expect(document.activeElement).toBe(item1); | ||
| }); | ||
| }); | ||
| describe('nested focus scopes', function () { | ||
| it('should make child FocusScopes the active scope regardless of DOM structure', function () { | ||
| function ChildComponent(props) { | ||
| return ReactDOM.createPortal(props.children, document.body); | ||
| } | ||
|
Comment on lines
+626
to
+628
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rendered the child FocusScope in a portal to test the case where the child is not contained within the parent FocusScope in the DOM but is still a child of the parent FocusScope (nested Dialog test case basically) |
||
|
|
||
| function Test({show}) { | ||
| return ( | ||
| <div> | ||
| <input data-testid="outside" /> | ||
| <FocusScope restoreFocus contain> | ||
| <input data-testid="input1" /> | ||
| {show && | ||
| <ChildComponent> | ||
| <FocusScope restoreFocus contain> | ||
| <input data-testid="input3" /> | ||
| </FocusScope> | ||
| </ChildComponent> | ||
| } | ||
| </FocusScope> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| let {getByTestId, rerender} = render(<Test />); | ||
| // Set a focused node and make first FocusScope the active scope | ||
| let input1 = getByTestId('input1'); | ||
| input1.focus(); | ||
| fireEvent.focusIn(input1); | ||
| expect(document.activeElement).toBe(input1); | ||
|
|
||
| rerender(<Test show />); | ||
| expect(document.activeElement).toBe(input1); | ||
| let input3 = getByTestId('input3'); | ||
| input3.focus(); | ||
| fireEvent.focusIn(input3); | ||
| expect(document.activeElement).toBe(input3); | ||
| }); | ||
| }); | ||
| }); | ||
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.
Did this cuz lint warning regarding hooks