From 4beecd0151747045d135a749c56c879790c40878 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Sun, 8 Nov 2020 12:31:33 +0100 Subject: [PATCH] less ambitious changes --- .../Unstable_TrapFocus/Unstable_TrapFocus.js | 41 +++--- .../Unstable_TrapFocus.test.js | 133 +++++++----------- test/utils/user-event/index.js | 46 ++++-- test/utils/user-event/index.test.js | 48 ++++++- 4 files changed, 157 insertions(+), 111 deletions(-) diff --git a/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js index 7d8e70701f9d84..255ed4492a16eb 100644 --- a/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js +++ b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.js @@ -43,25 +43,31 @@ function getTabIndex(node) { return node.tabIndex; } -function isRadioTabble(node) { +function isNonTabbableRadio(node) { + if (node.tagName !== 'INPUT' || node.type !== 'radio') { + return false; + } + if (!node.name) { - return true; + return false; } - let input = node.ownerDocument.querySelector(`input[type="radio"][name="${node.name}"]:checked`); + const getRadio = (selector) => node.ownerDocument.querySelector(`input[type="radio"]${selector}`); + + let roving = getRadio(`[name="${node.name}"]:checked`); - if (!input) { - input = node.ownerDocument.querySelector(`input[type="radio"][name="${node.name}"]`); + if (!roving) { + roving = getRadio(`[name="${node.name}"]`); } - return input === node; + return roving !== node; } -function isFocusable(node) { +function isNodeMatchingSelectorFocusable(node) { if ( node.disabled || (node.tagName === 'INPUT' && node.type === 'hidden') || - (node.tagName === 'INPUT' && node.type === 'radio' && !isRadioTabble(node)) + isNonTabbableRadio(node) ) { return false; } @@ -75,7 +81,7 @@ export function defaultGetTabbable(root) { Array.from(root.querySelectorAll(candidatesSelector)).forEach((node, i) => { const nodeTabIndex = getTabIndex(node); - if (nodeTabIndex === -1 || !isFocusable(node)) { + if (nodeTabIndex === -1 || !isNodeMatchingSelectorFocusable(node)) { return; } @@ -174,12 +180,7 @@ function Unstable_TrapFocus(props) { } if (activated.current) { - const tabbable = getTabbable(rootRef.current); - if (tabbable.length > 0) { - tabbable[0].focus(); - } else { - rootRef.current.focus(); - } + rootRef.current.focus(); } } @@ -244,7 +245,13 @@ function Unstable_TrapFocus(props) { return; } - const tabbable = getTabbable(rootRef.current); + let tabbable = []; + if ( + doc.activeElement === sentinelStart.current || + doc.activeElement === sentinelEnd.current + ) { + tabbable = getTabbable(rootRef.current); + } if (tabbable.length > 0) { const isShiftTab = Boolean( @@ -303,7 +310,7 @@ function Unstable_TrapFocus(props) { doc.removeEventListener('focusin', contain); doc.removeEventListener('keydown', loopFocus, true); }; - }, [disableAutoFocus, disableEnforceFocus, disableRestoreFocus, isEnabled, open]); + }, [disableAutoFocus, disableEnforceFocus, disableRestoreFocus, isEnabled, open, getTabbable]); const onFocus = (event) => { if (!activated.current) { diff --git a/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.test.js b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.test.js index 2af9f726e14a84..3d95a36edf4dce 100644 --- a/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.test.js +++ b/packages/material-ui/src/Unstable_TrapFocus/Unstable_TrapFocus.test.js @@ -26,10 +26,10 @@ describe('', () => { document.body.removeChild(initialFocus); }); - it('should return focus to the children', () => { + it('should return focus to the root', () => { const { getByTestId } = render( -
+
, @@ -38,7 +38,7 @@ describe('', () => { expect(getByTestId('auto-focus')).toHaveFocus(); initialFocus.focus(); - expect(getByTestId('auto-focus')).toHaveFocus(); + expect(getByTestId('root')).toHaveFocus(); }); it('should not return focus to the children when disableEnforceFocus is true', () => { @@ -71,7 +71,7 @@ describe('', () => { expect(getByTestId('auto-focus')).toHaveFocus(); }); - it('should warn if the modal content is not focusable', () => { + it('should warn if the root content is not focusable', () => { const UnfocusableDialog = React.forwardRef((_, ref) =>
); expect(() => { @@ -96,7 +96,7 @@ describe('', () => { it('should loop the tab key', () => { render( -
+
Title
@@ -104,7 +104,9 @@ describe('', () => {
, ); + expect(screen.getByTestId('root')).toHaveFocus(); + userEvent.tab(); expect(screen.getByText('x')).toHaveFocus(); userEvent.tab(); expect(screen.getByText('cancel')).toHaveFocus(); @@ -114,7 +116,8 @@ describe('', () => { expect(screen.getByText('x')).toHaveFocus(); initialFocus.focus(); - expect(screen.getByText('x')).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); + screen.getByText('x').focus(); userEvent.tab({ shift: true }); expect(screen.getByText('ok')).toHaveFocus(); }); @@ -122,7 +125,7 @@ describe('', () => { it('should focus on first focus element after last has received a tab click', () => { render( -
+
Title
@@ -131,67 +134,23 @@ describe('', () => { , ); + userEvent.tab(); + expect(screen.getByText('x')).toHaveFocus(); userEvent.tab(); expect(screen.getByText('cancel')).toHaveFocus(); userEvent.tab(); expect(screen.getByText('ok')).toHaveFocus(); - userEvent.tab(); - expect(screen.getByText('x')).toHaveFocus(); }); it('should focus rootRef if no tabbable children are rendered', () => { render( -
+
Title
, ); - expect(screen.getByTestId('modal')).toHaveFocus(); - }); - - it('should focus checked radio button if present in radio group', () => { - const Test = () => { - const [value, setValue] = React.useState('two'); - const onChange = (e) => setValue(e.target.value); - return ( - -
-
Title
- - - -
- - ); - }; - render(); - userEvent.tab(); - expect(screen.getByLabelText('two')).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); }); it('does not steal focus from a portaled element if any prop but open changes', () => { @@ -271,7 +230,7 @@ describe('', () => { return (
eventLog.push('blur')}> document} isEnabled={() => true} open {...props}> -
+
@@ -283,7 +242,7 @@ describe('', () => { // same behavior, just referential equality changes setProps({ isEnabled: () => true }); - expect(screen.getByTestId('focus-input')).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); expect(eventLog).to.deep.equal([]); }); @@ -314,7 +273,7 @@ describe('', () => { disableRestoreFocus {...props} > -
+
@@ -325,7 +284,7 @@ describe('', () => { setProps({ open: false, disableRestoreFocus: false }); // undesired: should be expect(initialFocus).toHaveFocus(); - expect(screen.getByTestId('focus-input')).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); }); it('undesired: setting `disableRestoreFocus` to false before closing has no effect', () => { @@ -338,7 +297,7 @@ describe('', () => { disableRestoreFocus {...props} > -
+
@@ -350,7 +309,7 @@ describe('', () => { setProps({ open: false }); // undesired: should be expect(initialFocus).toHaveFocus(); - expect(screen.getByTestId('focus-input')).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); }); describe('interval', () => { @@ -368,23 +327,27 @@ describe('', () => { function WithRemovableElement({ hideButton = false }) { return ( -
- {!hideButton && } +
+ {!hideButton && ( + + )}
); } - const { getByRole, setProps } = render(); - const dialog = getByRole('dialog'); - const toggleButton = getByRole('button', { name: 'I am going to disappear' }); + const { setProps } = render(); - expect(toggleButton).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); + screen.getByTestId('hide-button').focus(); + expect(screen.getByTestId('hide-button')).toHaveFocus(); setProps({ hideButton: true }); - expect(dialog).not.toHaveFocus(); + expect(screen.getByTestId('root')).not.toHaveFocus(); clock.tick(500); // wait for the interval check to kick in. - expect(dialog).toHaveFocus(); + expect(screen.getByTestId('root')).toHaveFocus(); }); describe('prop: disableAutoFocus', () => { @@ -393,7 +356,7 @@ describe('', () => {
-
+
, ); @@ -406,12 +369,12 @@ describe('', () => { }); it('should trap once the focus moves inside', () => { - const { getByRole } = render( + render(
- + -
-
, @@ -419,16 +382,16 @@ describe('', () => { expect(initialFocus).toHaveFocus(); - userEvent.tab(); - expect(getByRole('textbox')).toHaveFocus(); + screen.getByTestId('outside-input').focus(); + expect(screen.getByTestId('outside-input')).toHaveFocus(); // the trap activates userEvent.tab(); expect(screen.getByTestId('focus-input')).toHaveFocus(); // the trap prevent to escape - getByRole('textbox').focus(); - expect(screen.getByTestId('focus-input')).toHaveFocus(); + screen.getByTestId('outside-input').focus(); + expect(screen.getByTestId('root')).toHaveFocus(); }); it('should restore the focus', () => { @@ -436,26 +399,26 @@ describe('', () => {
-
+
); - const { getByTestId, setProps } = render(); + const { setProps } = render(); // set the expected focus restore location - getByTestId('outside-input').focus(); - expect(getByTestId('outside-input')).toHaveFocus(); + screen.getByTestId('outside-input').focus(); + expect(screen.getByTestId('outside-input')).toHaveFocus(); // the trap activates - getByTestId('modal').focus(); - expect(getByTestId('modal')).toHaveFocus(); + screen.getByTestId('root').focus(); + expect(screen.getByTestId('root')).toHaveFocus(); // restore the focus to the first element before triggering the trap setProps({ open: false }); - expect(getByTestId('outside-input')).toHaveFocus(); + expect(screen.getByTestId('outside-input')).toHaveFocus(); }); }); }); diff --git a/test/utils/user-event/index.js b/test/utils/user-event/index.js index 3f8d1c63859435..5f546793d70f94 100644 --- a/test/utils/user-event/index.js +++ b/test/utils/user-event/index.js @@ -64,21 +64,51 @@ function blur(element) { }); } -function getNextElement(currentIndex, shift, elements, focusTrap) { +function getNextElement({ tabbable, shift, focusTrap, previousElement }) { + if (previousElement.getAttribute('tabindex') === '-1') { + let found; + + if (shift) { + for (let i = tabbable.length; i >= 0; i -= 1) { + if ( + // eslint-disable-next-line no-bitwise + tabbable[i].compareDocumentPosition(previousElement) & Node.DOCUMENT_POSITION_FOLLOWING + ) { + found = tabbable[i]; + break; + } + } + } else { + for (let i = 0; i < tabbable.length; i += 1) { + if ( + // eslint-disable-next-line no-bitwise + tabbable[i].compareDocumentPosition(previousElement) & Node.DOCUMENT_POSITION_PRECEDING + ) { + found = tabbable[i]; + break; + } + } + } + return found; + } + + const currentIndex = tabbable.findIndex((el) => el === focusTrap.activeElement); + if (focusTrap === document && currentIndex === 0 && shift) { return document.body; } - if (focusTrap === document && currentIndex === elements.length - 1 && !shift) { + + if (focusTrap === document && currentIndex === tabbable.length - 1 && !shift) { return document.body; } + const nextIndex = shift ? currentIndex - 1 : currentIndex + 1; - const defaultIndex = shift ? elements.length - 1 : 0; - return elements[nextIndex] || elements[defaultIndex]; + const defaultIndex = shift ? tabbable.length - 1 : 0; + + return tabbable[nextIndex] || tabbable[defaultIndex]; } function tab({ shift = false, focusTrap } = {}) { - const previousElement = getActiveElement(focusTrap?.ownerDocument ?? document); - if (!focusTrap) { focusTrap = document; } @@ -89,8 +119,8 @@ function tab({ shift = false, focusTrap } = {}) { return; } - const index = tabbable.findIndex((el) => el === el.ownerDocument.activeElement); - const nextElement = getNextElement(index, shift, tabbable, focusTrap); + const previousElement = getActiveElement(focusTrap?.ownerDocument ?? document); + const nextElement = getNextElement({ shift, tabbable, focusTrap, previousElement }); const shiftKeyInit = { key: 'Shift', diff --git a/test/utils/user-event/index.test.js b/test/utils/user-event/index.test.js index e858ad40a6380d..4db90daf6a76e9 100644 --- a/test/utils/user-event/index.test.js +++ b/test/utils/user-event/index.test.js @@ -1,6 +1,6 @@ import { expect } from 'chai'; import * as React from 'react'; -import { createClientRender, userEvent } from 'test/utils'; +import { createClientRender, screen, userEvent } from 'test/utils'; describe('userEvent', () => { const render = createClientRender(); @@ -22,5 +22,51 @@ describe('userEvent', () => { userEvent.tab({ shift: true }); expect(document.activeElement).to.equal(inputs[0]); }); + + it('should handle radio', () => { + const Test = () => { + const [value, setValue] = React.useState('two'); + const onChange = (e) => setValue(e.target.value); + return ( +
+ + + + +
+ ); + }; + render(); + screen.getByTestId('start').focus(); + expect(screen.getByTestId('start')).toHaveFocus(); + userEvent.tab(); + expect(screen.getByLabelText('two')).toHaveFocus(); + }); }); });