Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#4080): ComboBox: aXe throws an error when the virtualized listbox is scrollable #4081

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
9 changes: 3 additions & 6 deletions packages/@react-aria/selection/src/useSelectableCollection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import {DOMAttributes, FocusableElement, FocusStrategy, KeyboardDelegate} from '@react-types/shared';
import {FocusEvent, Key, KeyboardEvent, RefObject, useEffect, useRef} from 'react';
import {focusSafely, getFocusableTreeWalker} from '@react-aria/focus';
import {focusWithoutScrolling, mergeProps, scrollIntoView, scrollIntoViewport, useEvent} from '@react-aria/utils';
import {focusWithoutScrolling, isAndroid, isIOS, mergeProps, scrollIntoView, scrollIntoViewport, useEvent} from '@react-aria/utils';
import {getInteractionModality} from '@react-aria/interactions';
import {isCtrlKeyPressed, isNonContiguousSelectionModifier} from './utils';
import {MultipleSelectionManager} from '@react-stately/selection';
Expand Down Expand Up @@ -409,12 +409,9 @@ export function useSelectableCollection(options: AriaSelectableCollectionOptions
handlers = mergeProps(typeSelectProps, handlers);
}

// If nothing is focused within the collection, make the collection itself tabbable.
// This will be marshalled to either the first or last item depending on where focus came from.
// If using virtual focus, don't set a tabIndex at all so that VoiceOver on iOS 14 doesn't try
// to move real DOM focus to the element anyway.
let tabIndex: number;
if (!shouldUseVirtualFocus) {
let isMobile = isAndroid() || isIOS();
if (!isMobile) {
tabIndex = manager.focusedKey == null ? 0 : -1;
}

Expand Down
21 changes: 14 additions & 7 deletions packages/@react-aria/selection/src/useSelectableItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@
*/

import {DOMAttributes, FocusableElement, LongPressEvent, PressEvent} from '@react-types/shared';
import {FocusEvent, FocusEventHandler, Key, RefObject, useEffect, useRef} from 'react';
import {focusSafely} from '@react-aria/focus';
import {isAndroid, isIOS, mergeProps} from '@react-aria/utils';
import {isCtrlKeyPressed, isNonContiguousSelectionModifier} from './utils';
import {Key, RefObject, useEffect, useRef} from 'react';
import {mergeProps} from '@react-aria/utils';
import {MultipleSelectionManager} from '@react-stately/selection';
import {PressProps, useLongPress, usePress} from '@react-aria/interactions';

Expand Down Expand Up @@ -153,14 +153,21 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte
// item is tabbable. If using virtual focus, don't set a tabIndex at all so that VoiceOver
// on iOS 14 doesn't try to move real DOM focus to the item anyway.
let itemProps: SelectableItemAria['itemProps'] = {};
if (!shouldUseVirtualFocus && !isDisabled) {
itemProps = {
tabIndex: key === manager.focusedKey ? 0 : -1,
onFocus(e) {
if (!isDisabled) {
let tabIndex: number;
let onFocus: FocusEventHandler<FocusableElement | Element>;
let isMobile = isAndroid() || isIOS();
if (!isMobile) {
tabIndex = key === manager.focusedKey ? 0 : -1;
onFocus = (e:FocusEvent<FocusableElement | Element>) => {
if (e.target === ref.current) {
manager.setFocusedKey(key);
}
}
};
}
itemProps = {
tabIndex,
onFocus
};
} else if (isDisabled) {
itemProps.onMouseDown = (e) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,13 @@ function testSearchAutocompleteOpen(searchAutocomplete, listbox, focusedItemInde
expect(items[1]).toHaveTextContent('Two');
expect(items[2]).toHaveTextContent('Three');

expect(listbox).not.toHaveAttribute('tabIndex');
expect(listbox).toHaveAttribute('tabIndex', typeof focusedItemIndex === 'undefined' ? '0' : '-1');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we expect focusedItemIndex to be undefined at this point? or is this a React 16/17/18 difference?

for (let item of items) {
expect(item).not.toHaveAttribute('tabIndex');
expect(item)
.toHaveAttribute(
'tabIndex',
typeof focusedItemIndex !== 'undefined' && item === items[focusedItemIndex] ? '0' : '-1'
);
}

expect(document.activeElement).toBe(searchAutocomplete);
Expand Down
8 changes: 6 additions & 2 deletions packages/@react-spectrum/combobox/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,13 @@ function testComboBoxOpen(combobox, button, listbox, focusedItemIndex) {
expect(items[1]).toHaveTextContent('Two');
expect(items[2]).toHaveTextContent('Three');

expect(listbox).not.toHaveAttribute('tabIndex');
expect(listbox).toHaveAttribute('tabIndex', typeof focusedItemIndex === 'undefined' ? '0' : '-1');
for (let item of items) {
expect(item).not.toHaveAttribute('tabIndex');
expect(item)
.toHaveAttribute(
'tabIndex',
typeof focusedItemIndex !== 'undefined' && item === items[focusedItemIndex] ? '0' : '-1'
);
}

expect(document.activeElement).toBe(combobox);
Expand Down