From 2a930b7e3f7f58638e457e1f15b608c6f6c010e8 Mon Sep 17 00:00:00 2001 From: Nicolas Gallagher Date: Wed, 9 Jun 2021 17:30:25 -0700 Subject: [PATCH] [fix] Change logic of PressResponder for keyboards This patch removes the keyboard `onClick` shim from `unstable_createElement` and adds it to the `PressResponder`. Previously, the logic in `createElement` was calling `onClick` during `onKeyDown`. This could result in React state update warnings if the host element was no longer in the DOM during `keyup`. Fix #2051 --- .../__snapshots__/index-test.js.snap | 27 ++++- .../exports/Pressable/__tests__/index-test.js | 46 +++++++- .../createDOMProps/__tests__/index-test.js | 111 ------------------ .../src/modules/createDOMProps/index.js | 45 ------- .../modules/usePressEvents/PressResponder.js | 37 +++++- 5 files changed, 101 insertions(+), 165 deletions(-) diff --git a/packages/react-native-web/src/exports/Pressable/__tests__/__snapshots__/index-test.js.snap b/packages/react-native-web/src/exports/Pressable/__tests__/__snapshots__/index-test.js.snap index 8d2f4f20..036d49d0 100644 --- a/packages/react-native-web/src/exports/Pressable/__tests__/__snapshots__/index-test.js.snap +++ b/packages/react-native-web/src/exports/Pressable/__tests__/__snapshots__/index-test.js.snap @@ -62,14 +62,14 @@ exports[`components/Pressable hover interaction 3`] = ` /> `; -exports[`components/Pressable press interaction 1`] = ` +exports[`components/Pressable press interaction (keyboard) 1`] = `
`; -exports[`components/Pressable press interaction 2`] = ` +exports[`components/Pressable press interaction (keyboard) 2`] = `
`; -exports[`components/Pressable press interaction 3`] = ` +exports[`components/Pressable press interaction (keyboard) 3`] = `null`; + +exports[`components/Pressable press interaction (pointer) 1`] = ` +
+`; + +exports[`components/Pressable press interaction (pointer) 2`] = ` +
+
+
+`; + +exports[`components/Pressable press interaction (pointer) 3`] = `
{ expect(container.firstChild).toMatchSnapshot(); }); - test('press interaction', () => { + test('press interaction (pointer)', () => { let container; const onContextMenu = jest.fn(); const onPress = jest.fn(); @@ -162,6 +162,50 @@ describe('components/Pressable', () => { expect(onContextMenu).toBeCalled(); }); + test('press interaction (keyboard)', () => { + let container; + const onPress = jest.fn(); + const onPressIn = jest.fn(); + const onPressOut = jest.fn(); + const ref = React.createRef(); + + function TestCase() { + const [shown, setShown] = React.useState(true); + return shown ? ( + (pressed ?
: null)} + onPress={(e) => { + onPress(e); + setShown(false); + }} + onPressIn={onPressIn} + onPressOut={onPressOut} + ref={ref} + style={({ pressed }) => [pressed && { outline: 'press-ring' }]} + /> + ) : null; + } + + act(() => { + ({ container } = render()); + }); + const target = createEventTarget(ref.current); + expect(container.firstChild).toMatchSnapshot(); + act(() => { + target.keydown({ key: 'Enter' }); + jest.runAllTimers(); + }); + expect(onPressIn).toBeCalled(); + expect(container.firstChild).toMatchSnapshot(); + act(() => { + target.keyup({ key: 'Enter' }); + jest.runAllTimers(); + }); + expect(onPressOut).toBeCalled(); + expect(onPress).toBeCalled(); + expect(container.firstChild).toMatchSnapshot(); + }); + describe('prop "ref"', () => { test('value is set', () => { const ref = jest.fn(); diff --git a/packages/react-native-web/src/modules/createDOMProps/__tests__/index-test.js b/packages/react-native-web/src/modules/createDOMProps/__tests__/index-test.js index 3e35d718..0a208eda 100644 --- a/packages/react-native-web/src/modules/createDOMProps/__tests__/index-test.js +++ b/packages/react-native-web/src/modules/createDOMProps/__tests__/index-test.js @@ -95,117 +95,6 @@ describe('modules/createDOMProps', () => { }); }); - describe('prop "onClick"', () => { - const callsOnClick = (component, accessibilityRole, accessibilityDisabled = false) => { - const onClick = jest.fn(); - const event = { stopPropagation: jest.fn() }; - const finalProps = createDOMProps(component, { - accessibilityRole, - accessibilityDisabled, - onClick - }); - finalProps.onClick(event); - return onClick.mock.calls.length === 1; - }; - - test('is called for various roles', () => { - expect(callsOnClick('div', 'link')).toBe(true); - expect(callsOnClick('div', 'button')).toBe(true); - expect(callsOnClick('div', 'textbox')).toBe(true); - expect(callsOnClick('div', 'bogus')).toBe(true); - expect(callsOnClick('a')).toBe(true); - expect(callsOnClick('button')).toBe(true); - expect(callsOnClick('input')).toBe(true); - expect(callsOnClick('select')).toBe(true); - expect(callsOnClick('textarea')).toBe(true); - expect(callsOnClick('h1')).toBe(true); - }); - - test('is not called when disabled is true', () => { - expect(callsOnClick('div', 'link', true)).toBe(false); - expect(callsOnClick('div', 'button', true)).toBe(false); - expect(callsOnClick('a', undefined, true)).toBe(false); - expect(callsOnClick('button', undefined, true)).toBe(false); - expect(callsOnClick('input', undefined, true)).toBe(false); - expect(callsOnClick('select', undefined, true)).toBe(false); - expect(callsOnClick('textarea', undefined, true)).toBe(false); - - expect(callsOnClick('div', 'textbox', true)).toBe(true); - expect(callsOnClick('div', 'bogus', true)).toBe(true); - expect(callsOnClick('h1', undefined, true)).toBe(true); - }); - }); - - describe('prop "onKeyDown"', () => { - const callsOnClick = (key) => (component, accessibilityRole, accessibilityDisabled = false) => { - const onClick = jest.fn(); - const onKeyDown = jest.fn(); - const event = { key, preventDefault: jest.fn() }; - const finalProps = createDOMProps(component, { - accessibilityRole, - accessibilityDisabled, - onClick, - onKeyDown - }); - finalProps.onKeyDown(event); - // The original onKeyDown should always be called - expect(onKeyDown).toHaveBeenCalled(); - return onClick.mock.calls.length === 1; - }; - - const respondsToEnter = callsOnClick('Enter'); - const respondsToSpace = callsOnClick(' '); - - test('does not emulate "onClick" when disabled', () => { - expect(respondsToEnter('div', 'link', true)).toBe(false); - expect(respondsToEnter('div', 'button', true)).toBe(false); - expect(respondsToEnter('div', 'textbox', true)).toBe(false); - expect(respondsToEnter('div', 'bogus', true)).toBe(false); - }); - - test('does not emulate "onClick" for native elements', () => { - expect(respondsToEnter('a')).toBe(false); - expect(respondsToEnter('button')).toBe(false); - expect(respondsToEnter('input')).toBe(false); - expect(respondsToEnter('select')).toBe(false); - expect(respondsToEnter('textarea')).toBe(false); - expect(respondsToEnter('h1')).toBe(false); - expect(respondsToEnter('div', 'link')).toBe(false); - - expect(respondsToSpace('a')).toBe(false); - expect(respondsToSpace('button')).toBe(false); - expect(respondsToSpace('input')).toBe(false); - expect(respondsToSpace('select')).toBe(false); - expect(respondsToSpace('textarea')).toBe(false); - expect(respondsToSpace('h1')).toBe(false); - expect(respondsToSpace('div', 'link')).toBe(false); - }); - - test('emulates "onClick" for "Enter" for certain roles', () => { - expect(respondsToEnter('div', 'button')).toBe(true); - expect(respondsToEnter('div', 'textbox')).toBe(false); - expect(respondsToEnter('div', 'bogus')).toBe(false); - }); - - test('emulates "onClick" for "Enter" for items marked accessible', () => { - const onClick = jest.fn(); - const event = { key: 'Enter', preventDefault: jest.fn() }; - const finalProps = createDOMProps('div', { - accessibilityRole: 'article', - focusable: true, - onClick - }); - finalProps.onKeyDown(event); - expect(onClick).toHaveBeenCalled(); - }); - - test('emulates "onClick" for "Space" for certain roles', () => { - expect(respondsToSpace('div', 'button')).toBe(true); - expect(respondsToSpace('div', 'textbox')).toBe(false); - expect(respondsToSpace('div', 'bogus')).toBe(false); - }); - }); - test('prop "accessibilityLabel" becomes "aria-label"', () => { const accessibilityLabel = 'accessibilityLabel'; const props = createProps({ accessibilityLabel }); diff --git a/packages/react-native-web/src/modules/createDOMProps/index.js b/packages/react-native-web/src/modules/createDOMProps/index.js index d96636f4..3f7efb13 100644 --- a/packages/react-native-web/src/modules/createDOMProps/index.js +++ b/packages/react-native-web/src/modules/createDOMProps/index.js @@ -136,15 +136,6 @@ const createDOMProps = (elementType, props) => { const role = AccessibilityUtil.propsToAriaRole(props); - const isNativeInteractiveElement = - role === 'link' || - elementType === 'a' || - elementType === 'button' || - elementType === 'input' || - elementType === 'select' || - elementType === 'textarea' || - domProps.contentEditable != null; - // DEPRECATED if (accessibilityState != null) { for (const prop in accessibilityState) { @@ -417,42 +408,6 @@ const createDOMProps = (elementType, props) => { domProps['data-testid'] = testID; } - // Keyboard accessibility - // Button-like roles should trigger 'onClick' if SPACE key is pressed. - // Button-like roles should not trigger 'onClick' if they are disabled. - if (isNativeInteractiveElement || role === 'button' || (_focusable === true && !disabled)) { - const onClick = domProps.onClick; - if (onClick != null) { - if (disabled) { - // Prevent click propagating if the element is disabled. See #1757 - domProps.onClick = function (e) { - e.stopPropagation(); - }; - } else if (!isNativeInteractiveElement) { - // For native elements that are focusable but don't dispatch 'click' events - // for keyboards. - const onKeyDown = domProps.onKeyDown; - domProps.onKeyDown = function (e) { - const { key, repeat } = e; - const isSpacebarKey = key === ' ' || key === 'Spacebar'; - const isButtonRole = role === 'button'; - if (onKeyDown != null) { - onKeyDown(e); - } - if (!repeat && key === 'Enter') { - onClick(e); - } else if (isSpacebarKey && isButtonRole) { - if (!repeat) { - onClick(e); - } - // Prevent spacebar scrolling the window - e.preventDefault(); - } - }; - } - } - } - return domProps; }; diff --git a/packages/react-native-web/src/modules/usePressEvents/PressResponder.js b/packages/react-native-web/src/modules/usePressEvents/PressResponder.js index e4fcfc18..1de4323b 100644 --- a/packages/react-native-web/src/modules/usePressEvents/PressResponder.js +++ b/packages/react-native-web/src/modules/usePressEvents/PressResponder.js @@ -130,11 +130,11 @@ const isTerminalSignal = (signal) => signal === RESPONDER_TERMINATED || signal === RESPONDER_RELEASE; const isValidKeyPress = (event) => { - const key = event.key; - const target = event.currentTarget; + const { key, target } = event; const role = target.getAttribute('role'); const isSpacebar = key === ' ' || key === 'Spacebar'; - return !event.repeat && (key === 'Enter' || (isSpacebar && role === 'button')); + + return key === 'Enter' || (isSpacebar && role === 'button'); }; const DEFAULT_LONG_PRESS_DELAY_MS = 450; // 500 - 50 @@ -298,9 +298,27 @@ export default class PressResponder { }; const keyupHandler = (event: KeyboardEvent) => { - if (this._touchState !== NOT_RESPONDER) { + const { onPress } = this._config; + const { target } = event; + + if (this._touchState !== NOT_RESPONDER && isValidKeyPress(event)) { end(event); document.removeEventListener('keyup', keyupHandler); + + const role = target.getAttribute('role'); + const elementType = target.tagName.toLowerCase(); + + const isNativeInteractiveElement = + role === 'link' || + elementType === 'a' || + elementType === 'button' || + elementType === 'input' || + elementType === 'select' || + elementType === 'textarea'; + + if (onPress != null && !isNativeInteractiveElement) { + onPress(event); + } } }; @@ -317,13 +335,22 @@ export default class PressResponder { }, onKeyDown: (event) => { - if (isValidKeyPress(event)) { + const { disabled } = this._config; + const { key, target } = event; + if (!disabled && isValidKeyPress(event)) { if (this._touchState === NOT_RESPONDER) { start(event, false); // Listen to 'keyup' on document to account for situations where // focus is moved to another element during 'keydown'. document.addEventListener('keyup', keyupHandler); } + const role = target.getAttribute('role'); + const isSpacebarKey = key === ' ' || key === 'Spacebar'; + const isButtonRole = role === 'button' || role === 'menuitem'; + if (isSpacebarKey && isButtonRole) { + // Prevent spacebar scrolling the window + event.preventDefault(); + } event.stopPropagation(); } },