isDisabled does not work as expected (#2479)

* first commit

* second commit

* preventing useMenuTrigger and useComboBox press handlers from toggling menu when isDisabled

* adding tests

* returning isDisabled from buttonProps in useComboBox

Co-authored-by: Daniel Lu <[email protected]>
Co-authored-by: Daniel Lu <[email protected]>

Author: Onyinye91-ctrl

packages/@react-aria/combobox/src/useComboBox.ts

@@ -77,7 +77,8 @@ export function useComboBox<T>(props: AriaComboBoxProps<T>, state: ComboBoxState
   let formatMessage = useMessageFormatter(intlMessages);
   let {menuTriggerProps, menuProps} = useMenuTrigger(
     {
-      type: 'listbox'
+      type: 'listbox',
+      isDisabled: isDisabled || isReadOnly
     },
     state,
     buttonRef
@@ -302,7 +303,8 @@ export function useComboBox<T>(props: AriaComboBoxProps<T>, state: ComboBoxState
       ...triggerLabelProps,
       excludeFromTabOrder: true,
       onPress,
-      onPressStart
+      onPressStart,
+      isDisabled: isDisabled || isReadOnly
     },
     inputProps: mergeProps(inputProps, {
       role: 'combobox',

packages/@react-aria/combobox/stories/useComboBox.stories.tsx

@@ -25,11 +25,14 @@ for (let i = 0; i < 50; i++) {
   lotsOfItems.push({name: 'Item ' + i});
 }
 
-const Template = () => () => (
-  <ComboBox label="Example" defaultItems={lotsOfItems}>
+const Template = () => (args) => (
+  <ComboBox {...args} label="Example" defaultItems={lotsOfItems}>
     {(item: any) => <Item key={item.name}>{item.name}</Item>}
   </ComboBox>
 );
 
 export const ScrollTesting = Template().bind({});
 ScrollTesting.args = {};
+
+export const Disabled = Template().bind({});
+Disabled.args = {isDisabled: true};

packages/@react-aria/combobox/test/useComboBox.test.js

@@ -21,6 +21,8 @@ import {useSingleSelectListState} from '@react-stately/list';
 describe('useComboBox', function () {
   let preventDefault = jest.fn();
   let stopPropagation = jest.fn();
+  let openSpy = jest.fn();
+  let toggleSpy = jest.fn();
   let event = (e) => ({
     ...e,
     preventDefault,
@@ -34,20 +36,31 @@ describe('useComboBox', function () {
   });
   mockLayout.collection = result.current.collection;
 
+  let props = {
+    label: 'test label',
+    popoverRef: {
+      current: {}
+    },
+    buttonRef: {
+      current: {}
+    },
+    inputRef: {
+      current: {
+        contains: jest.fn(),
+        focus: jest.fn()
+      }
+    },
+    listBoxRef: {
+      current: {}
+    },
+    layout: mockLayout
+  };
+
   afterEach(() => {
     jest.clearAllMocks();
   });
 
   it('should return default props for all the button group elements', function () {
-    let props = {
-      label: 'test label',
-      popoverRef: React.createRef(),
-      buttonRef: React.createRef(),
-      inputRef: React.createRef(),
-      listBoxRef: React.createRef(),
-      layout: mockLayout
-    };
-
     let {result} = renderHook(() => useComboBox(props, useSingleSelectListState(defaultProps)));
     let {buttonProps, inputProps, listBoxProps, labelProps} = result.current;
 
@@ -72,20 +85,6 @@ describe('useComboBox', function () {
   });
 
   it('should prevent default on Enter if isOpen', function () {
-    let props = {
-      label: 'test label',
-      popoverRef: React.createRef(),
-      buttonRef: React.createRef(),
-      inputRef: {
-        current: {
-          contains: jest.fn(),
-          focus: jest.fn()
-        }
-      },
-      listBoxRef: React.createRef(),
-      layout: mockLayout
-    };
-
     let {result: state} = renderHook((props) => useComboBoxState(props), {initialProps: {...props, allowsEmptyCollection: true}});
     act(() => {
       // set combobox state to open
@@ -113,22 +112,6 @@ describe('useComboBox', function () {
   });
 
   it('calls open and toggle with the expected parameters when arrow down/up/trigger button is pressed', function () {
-    let openSpy = jest.fn();
-    let toggleSpy = jest.fn();
-    let props = {
-      label: 'test label',
-      popoverRef: React.createRef(),
-      buttonRef: React.createRef(),
-      inputRef: {
-        current: {
-          contains: jest.fn(),
-          focus: jest.fn()
-        }
-      },
-      listBoxRef: React.createRef(),
-      layout: mockLayout
-    };
-
     let {result: state} = renderHook((props) => useComboBoxState(props), {initialProps: props});
     state.current.open = openSpy;
     state.current.toggle = toggleSpy;
@@ -152,4 +135,29 @@ describe('useComboBox', function () {
     expect(toggleSpy).toHaveBeenCalledTimes(2);
     expect(toggleSpy).toHaveBeenLastCalledWith(null, 'manual');
   });
+
+  it.each`
+    Name          | componentProps
+    ${'disabled'} | ${{isDisabled: true}}
+    ${'readonly'} | ${{isReadOnly: true}}
+  `('press and keyboard events on the button doesn\'t toggle the menu if $Name', function ({componentProps}) {
+    let additionalProps = {
+      ...props,
+      ...componentProps
+    };
+
+    let {result: state} = renderHook((props) => useComboBoxState(props), {initialProps: additionalProps});
+    state.current.open = openSpy;
+    state.current.toggle = toggleSpy;
+
+    let {result} = renderHook((props) => useComboBox(props, state.current), {initialProps: additionalProps});
+    let {buttonProps} = result.current;
+    buttonProps.onKeyDown(event({key: 'ArrowDown'}));
+    expect(openSpy).toHaveBeenCalledTimes(0);
+    expect(toggleSpy).toHaveBeenCalledTimes(0);
+    buttonProps.onKeyDown(event({key: 'ArrowUp'}));
+    expect(openSpy).toHaveBeenCalledTimes(0);
+    expect(toggleSpy).toHaveBeenCalledTimes(0);
+    expect(buttonProps.isDisabled).toBeTruthy();
+  });
 });

packages/@react-aria/menu/src/useMenuTrigger.ts

@@ -79,14 +79,14 @@ export function useMenuTrigger(props: MenuTriggerAriaProps, state: MenuTriggerSt
       id: menuTriggerId,
       onPressStart(e) {
         // For consistency with native, open the menu on mouse/key down, but touch up.
-        if (e.pointerType !== 'touch' && e.pointerType !== 'keyboard') {
+        if (e.pointerType !== 'touch' && e.pointerType !== 'keyboard' && !isDisabled) {
           // If opened with a screen reader, auto focus the first item.
           // Otherwise, the menu itself will be focused.
           state.toggle(e.pointerType === 'virtual' ? 'first' : null);
         }
       },
       onPress(e) {
-        if (e.pointerType === 'touch') {
+        if (e.pointerType === 'touch' && !isDisabled) {
           state.toggle();
         }
       },

packages/@react-aria/menu/test/useMenuTrigger.test.js

@@ -83,4 +83,16 @@ describe('useMenuTrigger', function () {
     expect(setFocusStrategy).toHaveBeenCalledTimes(1);
     expect(setFocusStrategy).toHaveBeenCalledWith(null);
   });
+
+  it('doesn\'t toggle the menu if isDisabled', function () {
+    let {menuTriggerProps} = renderMenuTriggerHook({isDisabled: true}, state);
+    expect(typeof menuTriggerProps.onPressStart).toBe('function');
+    menuTriggerProps.onPressStart({pointerType: 'mouse'});
+    expect(setOpen).toHaveBeenCalledTimes(0);
+    expect(setFocusStrategy).toHaveBeenCalledTimes(0);
+
+    menuTriggerProps.onPress({pointerType: 'touch'});
+    expect(setOpen).toHaveBeenCalledTimes(0);
+    expect(setFocusStrategy).toHaveBeenCalledTimes(0);
+  });
 });

packages/@react-spectrum/combobox/src/ComboBox.tsx

@@ -217,7 +217,6 @@ const ComboBoxInput = React.forwardRef(function ComboBoxInput(props: ComboBoxInp
   let {
     isQuiet,
     isDisabled,
-    isReadOnly,
     validationState,
     inputProps,
     inputRef,
@@ -340,7 +339,6 @@ const ComboBoxInput = React.forwardRef(function ComboBoxInput(props: ComboBoxInp
                 'spectrum-FieldButton'
               )
             }
-            isDisabled={isDisabled || isReadOnly}
             isQuiet={isQuiet}
             validationState={validationState}>
             <ChevronDownMedium UNSAFE_className={classNames(styles, 'spectrum-Dropdown-chevron')} />