-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 4 commits
4669b74
687d781
782e150
49f0e7b
82d998c
a4f12d8
d7be6fa
cbae0a8
3d64f8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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}> | ||
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems brittle, can we just use sentinel.closest('[inert]') is truthy? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () => { | ||
|
@@ -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 />); | ||
|
@@ -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 () { | ||
|
@@ -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}]);}); | ||
|
@@ -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', () => { | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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 () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😐