-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Dataviews] Table layout: Ensure focus is not lost on interaction #57340
Conversation
When tables are sorted or paginated, focus can be lost if the new data takes time to come back. This patch keeps the table header in place, whether data exists or not, and persists stale data until new data is returned, ensuring focus is not lost. Additionally, when columns are hidden from the column header menu, focus is also lost as the header menu no longer exists. This patch adds an `onHide` callback to each header menu, which when called will put the focus on a fallback header menu.
Very nice, do you think it's worth rebasing to check this against the latest work around dropdown menus? |
Yes. I thought I was up to date when I cut the branch, but evidently not! Rebased now, things still seem to work without issue. Interaction.with.latest.menu.implementation.mov |
Size Change: +1.31 kB (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
Flaky tests detected in 38fc7e4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7301259607
|
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.
Great improvement!
The focus behavior in particular seems like a great thing to have automated tests for. There doesn't seem to be any tests for this component yet so I'm not sure if we're waiting for things to stabilize more, but in the long run it'd be great to have test coverage since this will be a very complicated component.
packages/dataviews/src/view-table.js
Outdated
const usefulData = data.length ? data : shownData; | ||
const deferredData = deferredRendering ? shownData : usefulData; | ||
const staleData = shownData.length ? shownData : previousData.current; | ||
const usedData = | ||
isLoading && ! deferredData.length ? staleData : deferredData; |
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.
This section was a bit hard to follow for me. I had some suggestions to make it clearer, based on what I understood, but I might be misunderstanding some things.
shownData
does not seem like an accurate name, as it may or may not be the data shown. For naming consistency within this section of logic, we should probably stick to the adjective "deferred".- The
usefulData
variable name wasn't sufficiently informative for me, but the logic was actually easier to understand without that intermediate variable. So maybe we can merge that logic into the next conditional. - I am not exactly sure why we need to fall back to the async data when
data.length
is zero. Wouldn't the async data have a zero length in that case as well? (A code comment would help if there's a reason.) - The
deferredData
variable name indeferredData = deferredRendering ? shownData : usefulData
also seemed inaccurate, as it is sometimes not the async data.
diff --git a/packages/dataviews/src/view-table.js b/packages/dataviews/src/view-table.js
index cd7ac7cc39..e89e9707ae 100644
--- a/packages/dataviews/src/view-table.js
+++ b/packages/dataviews/src/view-table.js
@@ -315,12 +315,12 @@ function ViewTable( {
field.id
)
);
- const shownData = useAsyncList( data );
- const usefulData = data.length ? data : shownData;
- const deferredData = deferredRendering ? shownData : usefulData;
- const staleData = shownData.length ? shownData : previousData.current;
+ const deferredData = useAsyncList( data );
+ const maybeDeferredData =
+ deferredRendering || ! data.length ? deferredData : data;
+ const staleData = deferredData.length ? deferredData : previousData.current;
const usedData =
- isLoading && ! deferredData.length ? staleData : deferredData;
+ isLoading && ! maybeDeferredData.length ? staleData : maybeDeferredData;
previousData.current = usedData;
const hasData = !! usedData?.length;
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.
Would you be open to extract this to a separate PR? It seems this would be useful to any view type (not only table), and is unrelated to focus control. We could lift this logic to DataViews
and only pass data
to the view component.
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.
While looking at other things, I had a related thought: have you looked at leveraging how the data retrieval hook (useEntityRecords) works? It maintains a cache, and the stale data
is sometimes maintained (and some other times returns null
) while it goes through the lifecycle.
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.
As per @oandregal's suggestion, I've removed these changes in this PR, with a view to normalising them more generally. It doesn't impact focus persistence, so better to free this conversation from that.
Just a wrangling update: I've changed the The |
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.
This is a great improvement, thanks!
I'm pre-approving, though I left some suggestions. Please, accept or discard as you see fit.
Resolves #57341.
What?
This PR does
threetwo relatively small things:It persists stale data, to maintain visual consistency, which is rendered and marked asinert
while fresh data is being fetched (sorting, pagination, etc).Why?
When tables are sorted, filtered, or paginated, new data can be fetched. If this takes time to come back, the header is currently replaced with a "no results" message, until the fetch is complete. With the table header remaining in place, focus within header menus is persisted.
Separately, when columns are hidden from the column header menu, the menu is necessarily removed from the DOM, which leaves focus at the whim of the user agent. We need to be intentional about focus movement in this case.
How?
hasData
check, so that it is always rendered, regardless of whether data exists.A reference is maintained of previously rendered data, which is used when no current data is available.Testing Instructions
Testing Instructions for Keyboard
The same interactions should be validated using the keyboard – focus should not be lost at any point.
Screenshots or screencast
The table layout with no data – the header row is persisted.
Column header removal, showing focus being explicitly moved to a different column.
refocus.on.column.removal.mov