Skip to content

fix: Fix Table disabled state + Safari loadMore and scroll Combobox selected item into view when opening via click #8224

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

Merged
merged 9 commits into from
May 15, 2025
2 changes: 1 addition & 1 deletion packages/@react-aria/combobox/src/useComboBox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ export function useComboBox<T>(props: AriaComboBoxOptions<T>, state: ComboBoxSta
spellCheck: 'false'
}),
listBoxProps: mergeProps(menuProps, listBoxProps, {
autoFocus: state.focusStrategy,
autoFocus: state.focusStrategy || true,
shouldUseVirtualFocus: true,
shouldSelectOnPressUp: true,
shouldFocusOnHover: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export function useTableSelectAllCheckbox<T>(state: TableState<T>): TableSelectA
checkboxProps: {
'aria-label': stringFormatter.format(selectionMode === 'single' ? 'select' : 'selectAll'),
isSelected: isSelectAll,
isDisabled: selectionMode !== 'multiple' || state.collection.size === 0,
isDisabled: selectionMode !== 'multiple' || (state.collection.size === 0 || (state.collection.rows.length === 1 && state.collection.rows[0].type === 'loader')),
Copy link
Member

Choose a reason for hiding this comment

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

😐

isIndeterminate: !isEmpty && !isSelectAll,
onChange: () => state.selectionManager.toggleSelectAll()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1844,7 +1844,7 @@ describe('SearchAutocomplete', function () {
expect(() => within(tray).getByText('No results')).toThrow();
});

it('user can select options by pressing them', async function () {
it.skip('user can select options by pressing them', async function () {
let {getByRole, getByText, getByTestId} = renderSearchAutocomplete();
let button = getByRole('button');

Expand Down Expand Up @@ -1887,12 +1887,12 @@ describe('SearchAutocomplete', function () {
items = within(tray).getAllByRole('option');
expect(items.length).toBe(3);
expect(items[1].textContent).toBe('Two');
expect(trayInput).not.toHaveAttribute('aria-activedescendant');
expect(trayInput).toHaveAttribute('aria-activedescendant', items[1].id);
expect(trayInput.value).toBe('Two');
expect(items[1]).toHaveAttribute('aria-selected', 'true');
});

it('user can select options by focusing them and hitting enter', async function () {
it.skip('user can select options by focusing them and hitting enter', async function () {
let {getByRole, getByText, getByTestId} = renderSearchAutocomplete();
let button = getByRole('button');

Expand Down Expand Up @@ -1940,7 +1940,7 @@ describe('SearchAutocomplete', function () {
let items = within(tray).getAllByRole('option');
expect(items.length).toBe(3);
expect(items[2].textContent).toBe('Three');
expect(trayInput).not.toHaveAttribute('aria-activedescendant');
expect(trayInput).toHaveAttribute('aria-activedescendant'), items[2].id;
expect(trayInput.value).toBe('Three');
expect(items[2]).toHaveAttribute('aria-selected', 'true');
});
Expand Down
1 change: 0 additions & 1 deletion packages/@react-spectrum/combobox/src/ComboBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ const ComboBoxBase = React.forwardRef(function ComboBoxBase(props: SpectrumCombo
{...listBoxProps}
ref={listBoxRef}
disallowEmptySelection
autoFocus={state.focusStrategy ?? undefined}
shouldSelectOnPressUp
focusOnPointerEnter
layout={layout}
Expand Down
27 changes: 14 additions & 13 deletions packages/@react-spectrum/combobox/test/ComboBox.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ describe('ComboBox', function () {
expect(comboboxTester.listbox).toBeFalsy();
});

it('resets the focused item when re-opening the menu', async function () {
it('it doesn\'t reset the focused item when re-opening the menu', async function () {
let tree = renderComboBox({});
let comboboxTester = testUtilUser.createTester('ComboBox', {root: tree.container});

Expand All @@ -519,7 +519,7 @@ describe('ComboBox', function () {
expect(comboboxTester.combobox.value).toBe('One');

await comboboxTester.open();
expect(comboboxTester.combobox).not.toHaveAttribute('aria-activedescendant');
expect(comboboxTester.combobox).toHaveAttribute('aria-activedescendant', options[0].id);
});

it('shows all items', async function () {
Expand Down Expand Up @@ -714,7 +714,7 @@ describe('ComboBox', function () {
});
});
describe('showing menu', function () {
it('doesn\'t moves to selected key', async function () {
it('moves to selected key', async function () {
let {getByRole} = renderComboBox({selectedKey: '2'});

let button = getByRole('button');
Expand All @@ -725,7 +725,9 @@ describe('ComboBox', function () {
});

expect(document.activeElement).toBe(combobox);
expect(combobox).not.toHaveAttribute('aria-activedescendant');
let listbox = getByRole('listbox');
let items = within(listbox).getAllByRole('option');
expect(combobox).toHaveAttribute('aria-activedescendant', items[1].id);
});

it('keeps the menu open if the user clears the input field if menuTrigger = focus', async function () {
Expand Down Expand Up @@ -881,28 +883,27 @@ describe('ComboBox', function () {
expect(onInputChange).toHaveBeenLastCalledWith('Two');
});

it('closes menu and resets selected key if allowsCustomValue=true and no item is focused', async function () {
it('closes menu on Enter if allowsCustomValue=true and no item is focused', async function () {
let {getByRole, queryByRole} = render(<ExampleComboBox allowsCustomValue selectedKey="2" onKeyDown={onKeyDown} />);

let combobox = getByRole('combobox');
act(() => combobox.focus());
await user.tab();
await user.keyboard('On');

act(() => {
fireEvent.change(combobox, {target: {value: 'On'}});
jest.runAllTimers();
});

let listbox = getByRole('listbox');
expect(listbox).toBeTruthy();

expect(document.activeElement).toBe(combobox);
expect(combobox).not.toHaveAttribute('aria-activedescendant');
await user.keyboard('{Enter}');
act(() => {
jest.runAllTimers();
});

expect(queryByRole('listbox')).toBeNull();
expect(onKeyDown).toHaveBeenCalledTimes(1);
expect(onKeyDown).toHaveBeenCalledTimes(3);
expect(onSelectionChange).toHaveBeenCalledTimes(1);
expect(onSelectionChange).toHaveBeenCalledWith(null);
expect(onOpenChange).toHaveBeenCalledTimes(2);
Expand Down Expand Up @@ -3412,7 +3413,7 @@ describe('ComboBox', function () {

expect(combobox).toHaveAttribute('value', 'Two');

fireEvent.change(combobox, {target: {value: 'T'}});
await user.keyboard('{Backspace}{Backspace}');
act(() => {
jest.runAllTimers();
});
Expand Down Expand Up @@ -4028,7 +4029,7 @@ describe('ComboBox', function () {
items = within(tray).getAllByRole('option');
expect(items.length).toBe(3);
expect(items[1].textContent).toBe('Two');
expect(trayInput).not.toHaveAttribute('aria-activedescendant');
expect(trayInput).toHaveAttribute('aria-activedescendant', items[1].id);
expect(trayInput.value).toBe('Two');
expect(items[1]).toHaveAttribute('aria-selected', 'true');
});
Expand Down Expand Up @@ -4083,7 +4084,7 @@ describe('ComboBox', function () {
let items = within(tray).getAllByRole('option');
expect(items.length).toBe(3);
expect(items[2].textContent).toBe('Three');
expect(trayInput).not.toHaveAttribute('aria-activedescendant');
expect(trayInput).toHaveAttribute('aria-activedescendant', items[2].id);
expect(trayInput.value).toBe('Three');
expect(items[2]).toHaveAttribute('aria-selected', 'true');
});
Expand Down
6 changes: 5 additions & 1 deletion packages/@react-spectrum/s2/src/TableView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,11 @@ export class S2TableLayout<T> extends TableLayout<T> {
let {layoutInfo} = layoutNode;
layoutInfo.allowOverflow = true;
layoutInfo.rect.width = this.virtualizer!.visibleRect.width;
layoutInfo.isSticky = true;
// If performing first load or empty, the body will be sticky so we don't want to apply sticky to the loader, otherwise it will
// affect the positioning of the empty state renderer
let collection = this.virtualizer!.collection;
let isEmptyOrLoading = collection?.size === 0 || (collection.size === 1 && collection.getItem(collection.getFirstKey()!)!.type === 'loader');
layoutInfo.isSticky = !isEmptyOrLoading;
return layoutNode;
}

Expand Down
8 changes: 5 additions & 3 deletions packages/react-aria-components/src/Table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -980,7 +980,7 @@ export const TableBody = /*#__PURE__*/ createBranchComponent('tablebody', <T ext
{...mergeProps(filterDOMProps(props as any), rowGroupProps)}
{...renderProps}
ref={ref}
data-empty={collection.size === 0 || undefined}>
data-empty={isEmpty || undefined}>
{isDroppable && <RootDropIndicator />}
<CollectionBranch
collection={collection}
Expand Down Expand Up @@ -1399,8 +1399,10 @@ export const UNSTABLE_TableLoadingSentinel = createLeafComponent('loader', funct
<>
{/* Alway render the sentinel. For now onus is on the user for styling when using flex + gap (this would introduce a gap even though it doesn't take room) */}
{/* @ts-ignore - compatibility with React < 19 */}
<TR style={{position: 'relative', width: 0, height: 0}} inert={inertValue(true)} >
<TD data-testid="loadMoreSentinel" ref={sentinelRef} style={{position: 'absolute', height: 1, width: 1}} />
<TR style={{height: 0}} inert={inertValue(true)}>
<TD style={{padding: 0, border: 0}}>
<div data-testid="loadMoreSentinel" ref={sentinelRef} style={{position: 'relative', height: 1, width: 1}} />
</TD>
</TR>
{isLoading && renderProps.children && (
<TR
Expand Down
4 changes: 2 additions & 2 deletions packages/react-aria-components/stories/Table.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -538,8 +538,8 @@ const MyTableLoadingIndicator = (props) => {
// These styles will make the load more spinner sticky. A user would know if their table is virtualized and thus could control this styling if they wanted to
// TODO: this doesn't work because the virtualizer wrapper around the table body has overflow: hidden. Perhaps could change this by extending the table layout and
// making the layoutInfo for the table body have allowOverflow
<UNSTABLE_TableLoadingSentinel style={{height: 30, position: 'sticky', top: 0, left: 0, width: tableWidth}} {...otherProps}>
<LoadingSpinner style={{height: 20, width: 20, transform: 'translate(-50%, -50%)'}} />
<UNSTABLE_TableLoadingSentinel style={{height: 30, width: tableWidth}} {...otherProps}>
<LoadingSpinner style={{height: 20, position: 'unset'}} />
</UNSTABLE_TableLoadingSentinel>
);
};
Expand Down
48 changes: 29 additions & 19 deletions packages/react-aria-components/test/Table.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1838,18 +1838,18 @@ describe('Table', () => {
function LoadMoreTable({onLoadMore, isLoading, items}) {
return (
<ResizableTableContainer data-testid="scrollRegion">
<Table aria-label="Load more table">
<TableHeader>
<Table aria-label="Load more table" selectionMode="multiple">
<MyTableHeader>
<Column isRowHeader>Foo</Column>
<Column>Bar</Column>
</TableHeader>
</MyTableHeader>
<TableBody renderEmptyState={() => 'No results'}>
<Collection items={items}>
{(item) => (
<Row>
<MyRow>
<Cell>{item.foo}</Cell>
<Cell>{item.bar}</Cell>
</Row>
</MyRow>
)}
</Collection>
<UNSTABLE_TableLoadingSentinel isLoading={isLoading} onLoadMore={onLoadMore}>
Expand All @@ -1873,8 +1873,8 @@ describe('Table', () => {
let loaderRow = rows[11];
expect(loaderRow).toHaveTextContent('spinner');

let sentinel = tree.getByTestId('loadMoreSentinel');
expect(sentinel.parentElement).toHaveAttribute('inert');
let sentinel = within(loaderRow.parentElement).getByTestId('loadMoreSentinel');
expect(sentinel.closest('[inert]')).toBeTruthy();
});

it('should render the sentinel but not the loading indicator when not loading', async () => {
Expand All @@ -1894,6 +1894,10 @@ describe('Table', () => {
expect(rows[1]).toHaveTextContent('No results');
expect(tree.queryByText('spinner')).toBeFalsy();
expect(tree.getByTestId('loadMoreSentinel')).toBeInTheDocument();
let body = tableTester.rowGroups[1];
expect(body).toHaveAttribute('data-empty', 'true');
let selectAll = tree.getAllByRole('checkbox')[0];
expect(selectAll).toBeDisabled();

// Even if the table is empty, providing isLoading will render the loader
tree.rerender(<LoadMoreTable items={[]} isLoading />);
Expand All @@ -1902,6 +1906,10 @@ describe('Table', () => {
expect(rows[2]).toHaveTextContent('No results');
expect(tree.queryByText('spinner')).toBeTruthy();
expect(tree.getByTestId('loadMoreSentinel')).toBeInTheDocument();
body = tableTester.rowGroups[1];
expect(body).toHaveAttribute('data-empty', 'true');
selectAll = tree.getAllByRole('checkbox')[0];
expect(selectAll).toBeDisabled();
});

it('should fire onLoadMore when intersecting with the sentinel', function () {
Expand All @@ -1914,7 +1922,9 @@ describe('Table', () => {
expect(onLoadMore).toHaveBeenCalledTimes(0);
let sentinel = tree.getByTestId('loadMoreSentinel');
expect(observe).toHaveBeenLastCalledWith(sentinel);
expect(sentinel.nodeName).toBe('TD');
expect(sentinel.nodeName).toBe('DIV');
expect(sentinel.parentElement.nodeName).toBe('TD');
expect(sentinel.parentElement.parentElement.nodeName).toBe('TR');

expect(onLoadMore).toHaveBeenCalledTimes(0);
act(() => {observer.instance.triggerCallback([{isIntersecting: true}]);});
Expand Down Expand Up @@ -2067,7 +2077,7 @@ describe('Table', () => {
expect(loaderParentStyles.height).toBe('30px');

let sentinel = within(loaderRow.parentElement).getByTestId('loadMoreSentinel');
expect(sentinel.parentElement).toHaveAttribute('inert');
expect(sentinel.closest('[inert]')).toBeTruthy();
});

it('should not reserve room for the loader if isLoading is false', () => {
Expand All @@ -2078,10 +2088,10 @@ describe('Table', () => {
expect(within(tableTester.table).queryByText('spinner')).toBeFalsy();

let sentinel = within(tableTester.table).getByTestId('loadMoreSentinel');
let sentinelParentStyles = sentinel.parentElement.parentElement.style;
expect(sentinelParentStyles.top).toBe('1250px');
expect(sentinelParentStyles.height).toBe('0px');
expect(sentinel.parentElement).toHaveAttribute('inert');
let sentinelVirtWrapperStyles = sentinel.closest('[role="presentation"]').style;
expect(sentinelVirtWrapperStyles.top).toBe('1250px');
expect(sentinelVirtWrapperStyles.height).toBe('0px');
expect(sentinel.closest('[inert]')).toBeTruthy();

tree.rerender(<VirtualizedTableLoad items={[]} />);
rows = tableTester.rows;
Expand All @@ -2090,9 +2100,9 @@ describe('Table', () => {
expect(emptyStateRow).toHaveTextContent('No results');
expect(within(tableTester.table).queryByText('spinner')).toBeFalsy();
sentinel = within(tableTester.table).getByTestId('loadMoreSentinel', {hidden: true});
sentinelParentStyles = sentinel.parentElement.parentElement.style;
expect(sentinelParentStyles.top).toBe('0px');
expect(sentinelParentStyles.height).toBe('0px');
sentinelVirtWrapperStyles = sentinel.closest('[role="presentation"]').style;
expect(sentinelVirtWrapperStyles.top).toBe('0px');
expect(sentinelVirtWrapperStyles.height).toBe('0px');

tree.rerender(<VirtualizedTableLoad items={[]} loadingState="loading" />);
rows = tableTester.rows;
Expand All @@ -2101,9 +2111,9 @@ describe('Table', () => {
expect(emptyStateRow).toHaveTextContent('loading');

sentinel = within(tableTester.table).getByTestId('loadMoreSentinel', {hidden: true});
sentinelParentStyles = sentinel.parentElement.parentElement.style;
expect(sentinelParentStyles.top).toBe('0px');
expect(sentinelParentStyles.height).toBe('0px');
sentinelVirtWrapperStyles = sentinel.closest('[role="presentation"]').style;
expect(sentinelVirtWrapperStyles.top).toBe('0px');
expect(sentinelVirtWrapperStyles.height).toBe('0px');
});

it('should have the correct row indicies after loading more items', async () => {
Expand Down