DataViews: Add grid keyboard navigation
#68225
Open
+104
−62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
youknowriad,
oandregal,
jameskoster,
afercia and
jorgefilipecosta
ntsekouras
commented
Dec 22, 2024
| @@ -65,7 +65,7 @@ export function useHasAPossibleBulkAction< Item >( | |||
| item: Item | |||
| ) { | |||
| return useMemo( () => { | |||
| return actions.some( ( action ) => { | |||
| return actions?.some( ( action ) => { | |||
There was a problem hiding this comment.
This is not related to this PR.
There was a problem hiding this comment.
Does it make sense to move it out to another PR?
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ntsekouras
force-pushed
the
dataviews/grid-keyboard-navigation
branch
from
f54b260 to
4453d8e
Compare
|
Size Change: +146 B (+0.01%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ce8f5db. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12480784310
|
ntsekouras
force-pushed
the
dataviews/grid-keyboard-navigation
branch
from
4453d8e to
ce8f5db
Compare
|
Hey, tested this one and this is what I see. The tab stops are irregular: sometimes you stop at the item wrapper, and sometimes you don't. Additionally, I don't think we should have a stop at the item wrapper if it isn't interactive. Screen.Recording.2024-12-24.at.14.22.05.movAs for arrow key navigation, my understanding is that that arrow keys are bounded and should only move you through the direction (horizontal or vertical). Examples:
Screen.Recording.2024-12-24.at.14.28.44.mov |
Can you add some more steps to reproduce this? The composite item is always the wrapper and you should have the first grid item as an 'active' one, by pressing tab, while the focus was on Are you referring as
These are just two flags in our Composite component (focusLoop and focusWrap) and we can remove them if we want. I don't think it's agains some a11y patterns though. |
Try just navigating with tabs, one direction and then another. What I mean by irregular is that the number of stops is not consistent, it changes depending on direction, for example:
(also, for clarity: I don't think we should stop at any wrapper that is not interactive) Screen.Recording.2024-12-26.at.12.01.50.mov |
tyxla
reviewed
Dec 30, 2024
| @@ -65,7 +65,7 @@ export function useHasAPossibleBulkAction< Item >( | |||
| item: Item | |||
| ) { | |||
| return useMemo( () => { | |||
| return actions.some( ( action ) => { | |||
| return actions?.some( ( action ) => { | |||
There was a problem hiding this comment.
Does it make sense to move it out to another PR?
Comment on lines
+154
to
+165
| &[data-focus-visible] { | ||
| outline-color: var(--wp-admin-theme-color); | ||
| outline-width: var(--wp-admin-border-width-focus); | ||
| } | ||
|
|
||
| &:hover { | ||
| outline-color: rgba($black, 0.3); | ||
| outline-width: var(--wp-admin-border-width-focus); | ||
| } |
There was a problem hiding this comment.
The outer hover / focus states seem a bit awkward in terms of border-to-content spacing, and border radius:
There was a problem hiding this comment.
Yeah, I'd need some help with these styles from some designer probably..
|
Thanks for working on this ❤️ However, to my understanding, the element that receives focus is the container of the element with
I'm not sure that's correct.
There shouldn't be other elements 'in between' the ones above. A general note about the
It would be great to have this tested with screen readers on Windows, pinging @alexstine to check if he's willing to.
|
|
@afercia Testing with the latest version of NVDA, the grid is indeed announced as a table but as you pointed out, the screen reader does not switch to focus mode when interacting with the grid. If this is needed for this role, it will likely have to be wrapped in an application role. I hate to even consider that but the hard truth is, NVDA does not support ARIA roles very well for mode switching. This is something Slack has had a ton of problems with so I know it's not just Gutenberg in this case. Thanks. |
|
Hm reading this specific NVDA issue nvaccess/nvda#13392 looks like at NVDA they still have to 'determine' whether or not NVDA should switch back to browse mode when landing on a nested interactive control within a gridcell. A littlre disappointing.
I'm not sure it's really needed as long as you're able to navigate the grid cells? @alexstine For non-screen reader keyboard users the 'spatial' navigation with all the 4 arrow keys works, of course. The problem is for screen readers that don't automatically switch to focus/forms/interaction mode as expected for a |
ciampo
reviewed
Jan 7, 2025
ciampo
reviewed
Jan 7, 2025
ntsekouras
force-pushed
the
dataviews/grid-keyboard-navigation
branch
from
ce8f5db to
76d3ec4
Compare
|
Thank you all for the reviews! Besides the suggested structure, that needs the removal of only one wrapper now:
What else is missing from an a11y point of view. Did I misunderstood that we don't need to add a |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Resolves: #59188
This PR aims to add proper keyboard navigation in
gridlayout for DataViews.Tasks
outlineand how it plays with thepreview fieldoutline or when there is a selected item (also has outline).Testing Instructions
preview size(from view options) or not. This is important as the default styles are always applied throughJSnow. This is needed because we have to split programmaticallyrows.tab.Screenshots or screencast
Screen.Recording.2024-12-22.at.12.30.37.PM.mov