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
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
12 changes: 6 additions & 6 deletions packages/@react-spectrum/combobox/test/ComboBox.test.js
Copy link
Member Author

@LFDanLu LFDanLu May 13, 2025

Choose a reason for hiding this comment

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

For now I'm skipping these tests until we do more testing and confirm if we want to focus + scroll the ComboBox selected item into view when opening the drop down via click. These tests originate from #1014 but I don't remember/can't find any reason why we don't want this behavior other than a214902. The NVDA problem shouldn't apply since we still clear focus when the input field is modified, but will test

Copy link
Member Author

Choose a reason for hiding this comment

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

One potential weird point may be if you have a selected key and allowsCustomValue, where then if you backspace once the dropdown will open and an item will be focused, preventing a subsequent ENTER keypress from submitting your custom value

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.skip('resets the focused item when re-opening the menu', async function () {
let tree = renderComboBox({});
let comboboxTester = testUtilUser.createTester('ComboBox', {root: tree.container});

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.skip('doesn\'t moves to selected key', async function () {
let {getByRole} = renderComboBox({selectedKey: '2'});

let button = getByRole('button');
Expand Down Expand Up @@ -881,7 +881,7 @@ 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.skip('closes menu and resets selected key if allowsCustomValue=true and no item is focused', async function () {
let {getByRole, queryByRole} = render(<ExampleComboBox allowsCustomValue selectedKey="2" onKeyDown={onKeyDown} />);

let combobox = getByRole('combobox');
Expand Down Expand Up @@ -3361,7 +3361,7 @@ describe('ComboBox', function () {
}
}
`('$Name', ({Name, Component, action}) => {
it('should reset the input value and close the menu when pressing escape', async function () {
it.skip('should reset the input value and close the menu when pressing escape', async function () {
let {getByRole, queryByRole} = render(Component);
let button = getByRole('button');
let combobox = getByRole('combobox');
Expand Down Expand Up @@ -4028,7 +4028,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 +4083,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
46 changes: 28 additions & 18 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 @@ -1874,7 +1874,7 @@ describe('Table', () => {
expect(loaderRow).toHaveTextContent('spinner');

let sentinel = tree.getByTestId('loadMoreSentinel');
expect(sentinel.parentElement).toHaveAttribute('inert');
expect(sentinel.parentElement.parentElement).toHaveAttribute('inert');
Copy link
Member

Choose a reason for hiding this comment

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

seems brittle, can we just use sentinel.closest('[inert]') is truthy?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, was mainly concerned about that possibly matching a higher level inert element and still passing, but I'll see about limiting it to the body

});

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.parentElement.parentElement).toHaveAttribute('inert');
});

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.parentElement.parentElement).toHaveAttribute('inert');

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