From ef41409ceb7f704def9842d92687e2be27c5eccb Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 13 Mar 2020 15:01:28 -0700 Subject: [PATCH 1/6] update active scope to newly rendered dialogs --- packages/@react-aria/focus/src/FocusScope.tsx | 8 +++- .../@react-spectrum/dialog/src/Dialog.tsx | 2 +- .../dialog/test/DialogTrigger.test.js | 41 +++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx index 9107ea35be5..c7d11244746 100644 --- a/packages/@react-aria/focus/src/FocusScope.tsx +++ b/packages/@react-aria/focus/src/FocusScope.tsx @@ -19,7 +19,8 @@ interface FocusScopeProps { children: ReactNode, contain?: boolean, restoreFocus?: boolean, - autoFocus?: boolean + autoFocus?: boolean, + setActiveScope?: boolean } interface FocusManagerOptions { @@ -42,7 +43,7 @@ let activeScope: RefObject = null; // For now, it relies on the DOM tree order rather than the React tree order, and is probably // less optimized for performance. export function FocusScope(props: FocusScopeProps) { - let {children, contain, restoreFocus, autoFocus} = props; + let {children, contain, restoreFocus, autoFocus, setActiveScope} = props; let startRef = useRef(); let endRef = useRef(); let scopeRef = useRef([]); @@ -57,6 +58,9 @@ export function FocusScope(props: FocusScopeProps) { } scopeRef.current = nodes; + if (setActiveScope) { + activeScope = scopeRef; + } }, [children]); useFocusContainment(scopeRef, contain); diff --git a/packages/@react-spectrum/dialog/src/Dialog.tsx b/packages/@react-spectrum/dialog/src/Dialog.tsx index e58e53a36d4..591780f7ade 100644 --- a/packages/@react-spectrum/dialog/src/Dialog.tsx +++ b/packages/@react-spectrum/dialog/src/Dialog.tsx @@ -95,7 +95,7 @@ function BaseDialog({children, slots, size, role, ...otherProps}: SpectrumBaseDi } return ( - +
+ + Trigger1 + + contents1 + + Trigger2 + + contents2 + + + + + + ); + } + + let tree = render(); + let trigger1 = tree.getByText('Trigger1'); + triggerPress(trigger1); + await waitForDomChange(); // wait for animation + + let dialog1 = tree.getByTestId('dialog1'); + + expect(dialog1).toBeVisible(); + expect(document.activeElement).toEqual(dialog1); + + let trigger2 = tree.getByText('Trigger2'); + // trigger2.focus(); + // fireEvent.focusIn(trigger2); + triggerPress(trigger2); + + await waitForDomChange(); // wait for animation + let dialog2 = tree.getByTestId('dialog2'); + expect(dialog2).toBeVisible(); + expect(document.activeElement).toEqual(dialog2); + }); }); From 051cf500c652bb7c64857a80b6d4a8529d899065 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 16 Mar 2020 11:00:22 -0700 Subject: [PATCH 2/6] Adding test for isActiveScope --- packages/@react-aria/focus/src/FocusScope.tsx | 7 ++-- .../@react-aria/focus/test/FocusScope.test.js | 34 +++++++++++++++ .../@react-spectrum/dialog/src/Dialog.tsx | 2 +- .../dialog/test/DialogTrigger.test.js | 41 ------------------- 4 files changed, 39 insertions(+), 45 deletions(-) diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx index c7d11244746..de46deb215b 100644 --- a/packages/@react-aria/focus/src/FocusScope.tsx +++ b/packages/@react-aria/focus/src/FocusScope.tsx @@ -20,7 +20,7 @@ interface FocusScopeProps { contain?: boolean, restoreFocus?: boolean, autoFocus?: boolean, - setActiveScope?: boolean + isActiveScope?: boolean } interface FocusManagerOptions { @@ -43,7 +43,7 @@ let activeScope: RefObject = null; // For now, it relies on the DOM tree order rather than the React tree order, and is probably // less optimized for performance. export function FocusScope(props: FocusScopeProps) { - let {children, contain, restoreFocus, autoFocus, setActiveScope} = props; + let {children, contain, restoreFocus, autoFocus, isActiveScope} = props; let startRef = useRef(); let endRef = useRef(); let scopeRef = useRef([]); @@ -58,7 +58,7 @@ export function FocusScope(props: FocusScopeProps) { } scopeRef.current = nodes; - if (setActiveScope) { + if (isActiveScope) { activeScope = scopeRef; } }, [children]); @@ -196,6 +196,7 @@ function useFocusContainment(scopeRef: RefObject, contain: boolea // 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))) { activeScope = scopeRef; } diff --git a/packages/@react-aria/focus/test/FocusScope.test.js b/packages/@react-aria/focus/test/FocusScope.test.js index fc6ad2b0494..1895c5bf481 100644 --- a/packages/@react-aria/focus/test/FocusScope.test.js +++ b/packages/@react-aria/focus/test/FocusScope.test.js @@ -285,6 +285,40 @@ describe('FocusScope', function () { expect(document.activeElement).toBe(outside); }); + + it('should not restore focus back to the previous node if a new FocusScope becomes the active scope', function () { + function Test({show}) { + return ( +
+ + + + + {show && + + + + } +
+ ); + } + + let {getByTestId, rerender} = render(); + // 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 with a new active FocusScope + rerender(); + + expect(document.activeElement).toBe(input1); + let input3 = getByTestId('input3'); + input3.focus(); + fireEvent.focusIn(input3); + expect(document.activeElement).toBe(input3); + }); }); describe('auto focus', function () { diff --git a/packages/@react-spectrum/dialog/src/Dialog.tsx b/packages/@react-spectrum/dialog/src/Dialog.tsx index 591780f7ade..0d372a34a05 100644 --- a/packages/@react-spectrum/dialog/src/Dialog.tsx +++ b/packages/@react-spectrum/dialog/src/Dialog.tsx @@ -95,7 +95,7 @@ function BaseDialog({children, slots, size, role, ...otherProps}: SpectrumBaseDi } return ( - +
- - Trigger1 - - contents1 - - Trigger2 - - contents2 - - - - - - ); - } - - let tree = render(); - let trigger1 = tree.getByText('Trigger1'); - triggerPress(trigger1); - await waitForDomChange(); // wait for animation - - let dialog1 = tree.getByTestId('dialog1'); - - expect(dialog1).toBeVisible(); - expect(document.activeElement).toEqual(dialog1); - - let trigger2 = tree.getByText('Trigger2'); - // trigger2.focus(); - // fireEvent.focusIn(trigger2); - triggerPress(trigger2); - - await waitForDomChange(); // wait for animation - let dialog2 = tree.getByTestId('dialog2'); - expect(dialog2).toBeVisible(); - expect(document.activeElement).toEqual(dialog2); - }); }); From 419c72f4aa51607ec19158291c0ae1345a1c0b70 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 16 Mar 2020 11:03:06 -0700 Subject: [PATCH 3/6] removing extra newline --- packages/@react-aria/focus/src/FocusScope.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx index de46deb215b..cb3085ddeda 100644 --- a/packages/@react-aria/focus/src/FocusScope.tsx +++ b/packages/@react-aria/focus/src/FocusScope.tsx @@ -196,7 +196,6 @@ function useFocusContainment(scopeRef: RefObject, contain: boolea // 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))) { activeScope = scopeRef; } From e4c16fa7b7383cd155cc3f9153ac568a6aa4c477 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 16 Mar 2020 14:55:48 -0700 Subject: [PATCH 4/6] Making child focus scopes active over parent focus scopes --- packages/@react-aria/focus/src/FocusScope.tsx | 13 ++-- .../@react-aria/focus/test/FocusScope.test.js | 76 ++++++++++--------- .../@react-spectrum/dialog/src/Dialog.tsx | 2 +- 3 files changed, 49 insertions(+), 42 deletions(-) diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx index cb3085ddeda..44449b38b13 100644 --- a/packages/@react-aria/focus/src/FocusScope.tsx +++ b/packages/@react-aria/focus/src/FocusScope.tsx @@ -19,8 +19,7 @@ interface FocusScopeProps { children: ReactNode, contain?: boolean, restoreFocus?: boolean, - autoFocus?: boolean, - isActiveScope?: boolean + autoFocus?: boolean } interface FocusManagerOptions { @@ -43,10 +42,11 @@ let activeScope: RefObject = null; // For now, it relies on the DOM tree order rather than the React tree order, and is probably // less optimized for performance. export function FocusScope(props: FocusScopeProps) { - let {children, contain, restoreFocus, autoFocus, isActiveScope} = props; + let {children, contain, restoreFocus, autoFocus} = props; let startRef = useRef(); let endRef = useRef(); let scopeRef = useRef([]); + let prevContext = useFocusManager(); useEffect(() => { // Find all rendered nodes between the sentinels and add them to the scope. @@ -58,11 +58,12 @@ export function FocusScope(props: FocusScopeProps) { } scopeRef.current = nodes; - if (isActiveScope) { - activeScope = scopeRef; - } }, [children]); + if (prevContext) { + activeScope = scopeRef; + } + useFocusContainment(scopeRef, contain); useRestoreFocus(restoreFocus); useAutoFocus(scopeRef, autoFocus); diff --git a/packages/@react-aria/focus/test/FocusScope.test.js b/packages/@react-aria/focus/test/FocusScope.test.js index 1895c5bf481..beecde28d86 100644 --- a/packages/@react-aria/focus/test/FocusScope.test.js +++ b/packages/@react-aria/focus/test/FocusScope.test.js @@ -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); @@ -284,41 +285,7 @@ describe('FocusScope', function () { rerender(); expect(document.activeElement).toBe(outside); - }); - - it('should not restore focus back to the previous node if a new FocusScope becomes the active scope', function () { - function Test({show}) { - return ( -
- - - - - {show && - - - - } -
- ); - } - - let {getByTestId, rerender} = render(); - // 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 with a new active FocusScope - rerender(); - - expect(document.activeElement).toBe(input1); - let input3 = getByTestId('input3'); - input3.focus(); - fireEvent.focusIn(input3); - expect(document.activeElement).toBe(input3); - }); + }); }); describe('auto focus', function () { @@ -560,4 +527,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); + } + + function Test({show}) { + return ( +
+ + + + {show && + + + + + + } + +
+ ); + } + + let {getByTestId, rerender} = render(); + // 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(); + expect(document.activeElement).toBe(input1); + let input3 = getByTestId('input3'); + input3.focus(); + fireEvent.focusIn(input3); + expect(document.activeElement).toBe(input3); + }); + }); }); diff --git a/packages/@react-spectrum/dialog/src/Dialog.tsx b/packages/@react-spectrum/dialog/src/Dialog.tsx index 0d372a34a05..e58e53a36d4 100644 --- a/packages/@react-spectrum/dialog/src/Dialog.tsx +++ b/packages/@react-spectrum/dialog/src/Dialog.tsx @@ -95,7 +95,7 @@ function BaseDialog({children, slots, size, role, ...otherProps}: SpectrumBaseDi } return ( - +
Date: Wed, 18 Mar 2020 14:40:28 -0700 Subject: [PATCH 5/6] updating FocusScope onFocus and onBlur so they are attached to scope --- packages/@react-aria/focus/src/FocusScope.tsx | 63 ++++++++++--------- .../@react-aria/focus/test/FocusScope.test.js | 28 +++++++++ 2 files changed, 63 insertions(+), 28 deletions(-) diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx index 44449b38b13..93eaa5af897 100644 --- a/packages/@react-aria/focus/src/FocusScope.tsx +++ b/packages/@react-aria/focus/src/FocusScope.tsx @@ -36,6 +36,8 @@ interface FocusManager { const FocusContext = React.createContext(null); let activeScope: RefObject = null; +let counter = 0; +let scopesMap: Map> = new Map(); // This is a hacky DOM-based implementation of a FocusScope until this RFC lands in React: // https://github.com/reactjs/rfcs/pull/109 @@ -46,7 +48,7 @@ export function FocusScope(props: FocusScopeProps) { let startRef = useRef(); let endRef = useRef(); let scopeRef = useRef([]); - let prevContext = useFocusManager(); + let key = counter++; useEffect(() => { // Find all rendered nodes between the sentinels and add them to the scope. @@ -58,11 +60,11 @@ export function FocusScope(props: FocusScopeProps) { } scopeRef.current = nodes; - }, [children]); - - if (prevContext) { - activeScope = scopeRef; - } + scopesMap.set(key, scopeRef); + return () => { + scopesMap.delete(key); + }; + }, [children, key]); useFocusContainment(scopeRef, contain); useRestoreFocus(restoreFocus); @@ -153,6 +155,7 @@ function useFocusContainment(scopeRef: RefObject, contain: boolea let focusedNode = useRef(); useEffect(() => { + let scope = scopeRef.current; if (!contain) { return; } @@ -164,11 +167,11 @@ function useFocusContainment(scopeRef: RefObject, 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; @@ -194,38 +197,42 @@ function useFocusContainment(scopeRef: RefObject, 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))) { - activeScope = scopeRef; - } + e.stopPropagation(); + activeScope = scopeRef; + focusedNode.current = e.target; + }; - // Save the currently focused node in this scope - if (isInScope) { - focusedNode.current = e.target; - } + let onBlur = (e) => { + e.stopPropagation(); + let isInAnyScope = isElementInAnyScope(e.relatedTarget, scopesMap); - // 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); - } + if (!isInAnyScope) { + activeScope = scopeRef; + focusedNode.current = e.target; + 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)); + scope.forEach(element => element.removeEventListener('focusout', onBlur, false)); }; }, [scopeRef, contain]); } +function isElementInAnyScope(element: Element, scopes: Map>) { + 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)); } diff --git a/packages/@react-aria/focus/test/FocusScope.test.js b/packages/@react-aria/focus/test/FocusScope.test.js index beecde28d86..ae01fce4b92 100644 --- a/packages/@react-aria/focus/test/FocusScope.test.js +++ b/packages/@react-aria/focus/test/FocusScope.test.js @@ -251,6 +251,34 @@ describe('FocusScope', function () { outside.focus(); fireEvent.focusIn(outside); + fireEvent.focusOut(input2); + expect(document.activeElement).toBe(input2); + }); + + it('should restore focus to the last focused element in the scope on focus out', function () { + let {getByTestId} = render( +
+ + + + +
+ ); + + 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); }); }); From d27b0442c101b0bda0c06dae24b2d15a7d66c8ba Mon Sep 17 00:00:00 2001 From: Daniel Date: Thu, 19 Mar 2020 10:01:46 -0700 Subject: [PATCH 6/6] Adding focusin listener on document to catch tabbing from browser bar --- packages/@react-aria/focus/src/FocusScope.tsx | 33 ++++++++++++------- .../@react-aria/focus/test/FocusScope.test.js | 1 - 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx index 93eaa5af897..9eed0140a4e 100644 --- a/packages/@react-aria/focus/src/FocusScope.tsx +++ b/packages/@react-aria/focus/src/FocusScope.tsx @@ -36,8 +36,7 @@ interface FocusManager { const FocusContext = React.createContext(null); let activeScope: RefObject = null; -let counter = 0; -let scopesMap: Map> = new Map(); +let scopes: Set> = 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 @@ -48,7 +47,6 @@ export function FocusScope(props: FocusScopeProps) { let startRef = useRef(); let endRef = useRef(); let scopeRef = useRef([]); - let key = counter++; useEffect(() => { // Find all rendered nodes between the sentinels and add them to the scope. @@ -60,11 +58,11 @@ export function FocusScope(props: FocusScopeProps) { } scopeRef.current = nodes; - scopesMap.set(key, scopeRef); + scopes.add(scopeRef); return () => { - scopesMap.delete(key); + scopes.delete(scopeRef); }; - }, [children, key]); + }, [children]); useFocusContainment(scopeRef, contain); useRestoreFocus(restoreFocus); @@ -197,14 +195,25 @@ function useFocusContainment(scopeRef: RefObject, contain: boolea }; let onFocus = (e) => { - e.stopPropagation(); - activeScope = scopeRef; - 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 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; + focusedNode.current = e.target; + } }; let onBlur = (e) => { e.stopPropagation(); - let isInAnyScope = isElementInAnyScope(e.relatedTarget, scopesMap); + let isInAnyScope = isElementInAnyScope(e.relatedTarget, scopes); if (!isInAnyScope) { activeScope = scopeRef; @@ -214,17 +223,19 @@ function useFocusContainment(scopeRef: RefObject, contain: boolea }; 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)); scope.forEach(element => element.removeEventListener('focusout', onBlur, false)); }; }, [scopeRef, contain]); } -function isElementInAnyScope(element: Element, scopes: Map>) { +function isElementInAnyScope(element: Element, scopes: Set>) { for (let scope of scopes.values()) { if (isElementInScope(element, scope.current)) { return true; diff --git a/packages/@react-aria/focus/test/FocusScope.test.js b/packages/@react-aria/focus/test/FocusScope.test.js index ae01fce4b92..2852642e30f 100644 --- a/packages/@react-aria/focus/test/FocusScope.test.js +++ b/packages/@react-aria/focus/test/FocusScope.test.js @@ -251,7 +251,6 @@ describe('FocusScope', function () { outside.focus(); fireEvent.focusIn(outside); - fireEvent.focusOut(input2); expect(document.activeElement).toBe(input2); });