DataViews: navigate through list items via arrow keys #57895
Conversation
oandregal
added
[Type] Experimental
|
Size Change: +262 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
| { onDetailsChange && ( | ||
| <Button | ||
| className="dataviews-view-list__details-button" | ||
| <TreeGridCell> |
There was a problem hiding this comment.
I'm trying to use the TreeGrid component in DataViews. In trunk, using the list layout, the user moves with tabs through the list of items. See:
Gravacao.do.ecra.2024-01-16.as.18.40.58.mov
Note how this div is acting as a button, and it's a tab stop. When migrating to use TreeGrid, this element is skipped: the focus goes to the next TreeGridCell (details button) instead.
Gravacao.do.ecra.2024-01-16.as.18.46.07.mov
Once the focus is in the button, I cannot reach this div through the arrow keys either (right/left):
Gravacao.do.ecra.2024-01-16.as.18.49.44.mov
There was a problem hiding this comment.
Pinging people I've seen contributed to this and/or may be interested in following-up. @mirka @andrewhayward @ciampo @talldan @tellthemachines @andrewserong
There was a problem hiding this comment.
I've tried the following: setting the tabIndex to 0 when the focus is outside the list view and let TreeGrid handle it when it's inside. It doesn't work, once the focus is moved via the arrow keys to the next item, the details button becomes focused:
Gravacao.do.ecra.2024-01-16.as.18.54.11.mov
Diff in case anyone wants to try
diff --git a/packages/dataviews/src/view-list.js b/packages/dataviews/src/view-list.js
index ce1f3a2b4f..18949f2bd5 100644
--- a/packages/dataviews/src/view-list.js
+++ b/packages/dataviews/src/view-list.js
@@ -18,6 +18,7 @@ import {
import { ENTER, SPACE } from '@wordpress/keycodes';
import { info } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';
+import { useRef } from '@wordpress/element';
export default function ViewList( {
view,
@@ -30,6 +31,7 @@ export default function ViewList( {
selection,
deferredRendering,
} ) {
+ const treeGridRef = useRef();
const shownData = useAsyncList( data, { step: 3 } );
const usedData = deferredRendering ? shownData : data;
const mediaField = fields.find(
@@ -69,8 +71,15 @@ export default function ViewList( {
);
}
+ const isFocusWithin = () => {
+ if ( ! treeGridRef.current ) {
+ return false;
+ }
+ return treeGridRef.current.contains( document.activeElement );
+ }
+
return (
- <TreeGrid className="dataviews-view-list">
+ <TreeGrid ref={ treeGridRef } className="dataviews-view-list">
{ usedData.map( ( item, index ) => {
return (
<TreeGridRow
@@ -95,7 +104,7 @@ export default function ViewList( {
onSelectionChange( [ item ] )
}
{ ...otherProps }
- tabIndex={ tabIndex }
+ tabIndex={ isFocusWithin() ? tabIndex : 0 }
>
<HStack spacing={ 3 } justify="start">
<div className="dataviews-view-list__media-wrapper">
There was a problem hiding this comment.
I think the issue is that the <div> element isn't detected as a focusable.
TreeGrid uses the dom package's focusable utility internally, and that doesn't seem to be returning the div as one of the focusable elements in the row. This is the relevant code:
gutenberg/packages/components/src/tree-grid/index.tsx
Lines 23 to 31 in e84b38d
The selector list for focusable is here:
gutenberg/packages/dom/src/focusable.js
Lines 31 to 45 in e84b38d
Out of interest, why use a div?
setting the tabIndex to 0 when the focus is outside the list view and let TreeGrid handle it when it's inside.
I think this would prevent the roving tab index from working correctly.
There was a problem hiding this comment.
I put together an example of a fix in #57905 for you, but IMO it might be better to use a <button> if possible.
I'm also wondering about the suitability of treegrid. I think you should only use it if there's some hierarchy/nesting to the list. Perhaps you have that with parent pages?
It can be fun to style as it makes everything a table 😄
There was a problem hiding this comment.
I put together an example of a fix in #57905 for you
Nice, thanks Dan! Merged it to keep the conversation in one place.
but IMO it might be better to use a if possible.
I can investigate alternatives.
I'm also wondering about the suitability of treegrid. I think you should only use it if there's some hierarchy/nesting to the list. Perhaps you have that with parent pages?
Yeah, exactly.
There was a problem hiding this comment.
I'm also wondering about the suitability of treegrid. I think you should only use it if there's some hierarchy/nesting to the list. Perhaps you have that with parent pages?
Yeah, exactly.
Would using Composite be a better alternative? (cc @andrewhayward )
oandregal
requested review from
andrewhayward,
youknowriad,
jameskoster,
jorgefilipecosta and
ntsekouras
|
Also flagging this PR from @andrewhayward #56513 that may try to achieve a similar result for the Table layout, but using the |
|
Aside from the technical discussion, here are a couple of notes on the UX based on a discussion I had with Andrew about this last week:
|
|
I'm no longer convinced this is a good approach. The reason is that consumers of the DataViews package (Pages, Templates, Patterns, any 3rd party) control field rendering: they can render any focusable element within the list item which will break the interactions. This is a test case: render a link for the title field. After merging Dan's patch, it was working as expected. Then, I updated the Pages so it renders a link for the title using the patch below. Patch for rendering a link.diff --git a/packages/edit-site/src/components/page-pages/index.js b/packages/edit-site/src/components/page-pages/index.js
index 5ef05273be..39cec46a0d 100644
--- a/packages/edit-site/src/components/page-pages/index.js
+++ b/packages/edit-site/src/components/page-pages/index.js
@@ -231,7 +231,7 @@ export default function PagePages() {
id: 'title',
getValue: ( { item } ) => item.title?.rendered,
render: ( { item } ) => {
- return [ LAYOUT_TABLE, LAYOUT_GRID ].includes(
+ return [ LAYOUT_TABLE, LAYOUT_GRID, LAYOUT_LIST ].includes(
view.type
) ? (
<LinkThis is the result: the roving tabindex no longer works (users can tab, doesn't find the proper nodes, etc.). Gravacao.do.ecra.2024-01-17.as.11.50.15.mov |
@jameskoster You mean this flow?
Right now you can reach it after pagination. |
Yes.
That's correct, but I don't suppose it's all that intuitive. |
This is a tricky one. If the relevant content loads asynchronously, it's generally better to leave it as a manual step, otherwise moving around in the list can cause a lot of overhead. Because this is a flexible and multi-use view, we can't say for sure whether the content is going to be async or not (though I'm guessing in most cases it probably will be), so it's probably better to leave it as is, at least to start. |
|
Happy to start with that. |
Yeah, that's a tricky one, as it requires wrapping every control in a I think there is some precedence with the block toolbar and the |
|
A few high-level thoughts...
As mentioned above, this isn't really a suitable candidate for a
Regarding the tab flow aspect, without further testing, I would probably expect it to look something like this:
Step 4 might get a bit weird if the preview panel has further interactive content in it, so we might have to adjust this. For arrow key navigation, I'd go with something like this:
Of particular note, if list items can contain interactive elements, we won't be able to have the list item itself be a button, or equivalent... <!-- ⛔️ We can't do this... -->
<ul>
...
<li class="list-item">
<button>
<img src="..." class="thumb">
<div class="primary">Title</div>
<div class="fields">
<a href="...">Field</a>
</div>
</button>
<a href="..." class="info">ℹ</a>
</li>
...
</ul>Instead we'll have to move the actual button to the primary field, but maintain the visual and interactive appearance by being clever with either CSS, or event handlers. <!-- ✅ This is better... -->
<ul>
...
<li class="list-item">
<img src="..." class="thumb">
<button class="primary">Title</button>
<div class="fields">
<a href="...">Field</a>
</div>
<a href="..." class="info">ℹ</a>
</li>
...
</ul>Let me know if there are any specific questions, or if you want any help wrangling that in |
I took @oandregal's response to mean that there is a tree aspect to it (parent/child pages). From memory, I think TreeGrid support is still a bit ropey in screenreaders, I'm not sure there's any inbuilt announcements about nesting, so a grid that relays adequate information about nesting might be just as good. 🤷 |
|
The hierarchical relationship exists, but we haven't defined how to visualize it yet. I wanted to try the Keep the comments coming, it had been already useful to clarify expectations around interactions. |
Just to follow up on this bit quickly, we can achieve the end goal here with the following mark-up (presentational stuff omitted):
|
|
Closing in favor of #59637 |
Part of #55083
Work In Progress.
What?
Implements arrow key navigation for the list layout. The list of items behaves like a single stop.
Why?
This provides a better keyboard navigation and follows accessibility best practices.
How?
Substitutes the unordered
<ul>list by a<TreeGrid>.Testing Instructions