Fix onPress when used with onKeyUp on a button (#5554)

* Fix button stuck in pressed state

Author: snowystinger

packages/@react-aria/interactions/src/usePress.ts

@@ -15,9 +15,9 @@
 // NOTICE file in the root directory of this source tree.
 // See https://github.com/facebook/react/tree/cc7c1aece46a6b69b41958d731e0fd27c94bfc6c/packages/react-interactions
 
+import {chain, focusWithoutScrolling, getOwnerDocument, getOwnerWindow, isMac, isVirtualClick, isVirtualPointerEvent, mergeProps, openLink, useEffectEvent, useGlobalListeners, useSyncRef} from '@react-aria/utils';
 import {disableTextSelection, restoreTextSelection} from './textSelection';
 import {DOMAttributes, FocusableElement, PressEvent as IPressEvent, PointerType, PressEvents} from '@react-types/shared';
-import {focusWithoutScrolling, getOwnerDocument, getOwnerWindow, isMac, isVirtualClick, isVirtualPointerEvent, mergeProps, openLink, useEffectEvent, useGlobalListeners, useSyncRef} from '@react-aria/utils';
 import {PressResponderContext} from './context';
 import {RefObject, useContext, useEffect, useMemo, useRef, useState} from 'react';
 
@@ -270,8 +270,16 @@ export function usePress(props: PressHookProps): PressResult {
             shouldStopPropagation = triggerPressStart(e, 'keyboard');
 
             // Focus may move before the key up event, so register the event on the document
-            // instead of the same element where the key down event occurred.
-            addGlobalListener(getOwnerDocument(e.currentTarget), 'keyup', onKeyUp, false);
+            // instead of the same element where the key down event occurred. Make it capturing so that it will trigger
+            // before stopPropagation from useKeyboard on a child element may happen and thus we can still call triggerPress for the parent element.
+            let originalTarget = e.currentTarget;
+            let pressUp = (e) => {
+              if (isValidKeyboardEvent(e, originalTarget) && !e.repeat && originalTarget.contains(e.target as Element) && state.target) {
+                triggerPressUp(createEvent(state.target, e), 'keyboard');
+              }
+            };
+
+            addGlobalListener(getOwnerDocument(e.currentTarget), 'keyup', chain(pressUp, onKeyUp), true);
           }
 
           if (shouldStopPropagation) {
@@ -292,11 +300,6 @@ export function usePress(props: PressHookProps): PressResult {
           state.metaKeyEvents = new Map();
         }
       },
-      onKeyUp(e) {
-        if (isValidKeyboardEvent(e.nativeEvent, e.currentTarget) && !e.repeat && e.currentTarget.contains(e.target as Element) && state.target) {
-          triggerPressUp(createEvent(state.target, e), 'keyboard');
-        }
-      },
       onClick(e) {
         if (e && !e.currentTarget.contains(e.target as Element)) {
           return;
@@ -338,13 +341,9 @@ export function usePress(props: PressHookProps): PressResult {
         }
 
         let target = e.target as Element;
-        let shouldStopPropagation = triggerPressEnd(createEvent(state.target, e), 'keyboard', state.target.contains(target));
+        triggerPressEnd(createEvent(state.target, e), 'keyboard', state.target.contains(target));
         removeAllGlobalListeners();
 
-        if (shouldStopPropagation) {
-          e.stopPropagation();
-        }
-
         // If a link was triggered with a key other than Enter, open the URL ourselves.
         // This means the link has a role override, and the default browser behavior
         // only applies when using the Enter key.

packages/@react-aria/interactions/test/usePress.test.js

@@ -1789,7 +1789,6 @@ describe('usePress', function () {
       let el = getByText('test');
       fireEvent.keyDown(el, {key: ' '});
       fireEvent.keyUp(el, {key: ' '});
-
       expect(events).toEqual([
         {
           type: 'pressstart',

packages/@react-spectrum/button/stories/Button.stories.tsx

@@ -31,9 +31,11 @@ export default {
     onPress: action('press'),
     onPressStart: action('pressstart'),
     onPressEnd: action('pressend'),
+    onPressChange: action('presschange'),
     onPressUp: action('pressup'),
     onFocus: action('focus'),
-    onBlur: action('blur')
+    onBlur: action('blur'),
+    onKeyUp: action('keyup')
   },
   argTypes: {
     onPress: {

packages/@react-spectrum/button/test/Button.test.js

@@ -78,6 +78,34 @@ describe('Button', function () {
     expect(text).not.toBeNull();
   });
 
+  it.each`
+    Name              | Component      | props
+    ${'ActionButton'} | ${ActionButton}| ${{onPress: onPressSpy, onPressStart: onPressStartSpy, onPressEnd: onPressEndSpy, onPressUp: onPressUpSpy, onPressChange: onPressChangeSpy}}
+    ${'Button'}       | ${Button}      | ${{onPress: onPressSpy, onPressStart: onPressStartSpy, onPressEnd: onPressEndSpy, onPressUp: onPressUpSpy, onPressChange: onPressChangeSpy}}
+    ${'LogicButton'}  | ${LogicButton} | ${{onPress: onPressSpy, onPressStart: onPressStartSpy, onPressEnd: onPressEndSpy, onPressUp: onPressUpSpy, onPressChange: onPressChangeSpy}}
+  `('$Name keyboard press and key events', async function ({Component, props}) {
+    let onKeyDownSpy = jest.fn();
+    let onKeyUpSpy = jest.fn();
+    let {getByRole} = render(<Component {...props} onKeyDown={onKeyDownSpy} onKeyUp={onKeyUpSpy}>Click Me</Component>);
+
+    let button = getByRole('button');
+    await user.tab();
+    expect(onKeyUpSpy).toHaveBeenCalledTimes(1);
+    expect(document.activeElement).toBe(button);
+    await user.keyboard('{Enter}');
+    expect(onPressStartSpy).toHaveBeenCalledTimes(1);
+    expect(onPressSpy).toHaveBeenCalledTimes(1);
+    expect(onPressEndSpy).toHaveBeenCalledTimes(1);
+    expect(onPressUpSpy).toHaveBeenCalledTimes(1);
+    expect(onPressChangeSpy).toHaveBeenCalledTimes(2);
+    expect(onKeyDownSpy).toHaveBeenCalledTimes(1);
+    expect(onKeyUpSpy).toHaveBeenCalledTimes(2);
+
+    await user.keyboard('A');
+    expect(onKeyDownSpy).toHaveBeenCalledTimes(2);
+    expect(onKeyUpSpy).toHaveBeenCalledTimes(3);
+  });
+
   it.each`
     Name              | Component
     ${'ActionButton'} | ${ActionButton}

packages/react-aria-components/stories/ToggleButton.stories.tsx

@@ -10,6 +10,7 @@
  * governing permissions and limitations under the License.
  */
 
+import {action} from '@storybook/addon-actions';
 import {classNames} from '@react-spectrum/utils';
 import React from 'react';
 import styles from '../example/index.css';
@@ -21,6 +22,6 @@ export default {
 
 export const ToggleButtonExample = () => {
   return (
-    <ToggleButton className={classNames(styles, 'toggleButtonExample')} data-testid="toggle-button-example">Toggle</ToggleButton>
+    <ToggleButton className={classNames(styles, 'toggleButtonExample')} data-testid="toggle-button-example" onKeyUp={action('keyup')} onPress={action('press')}>Toggle</ToggleButton>
   );
 };