Make grid items tababble and add a focused state #57896
Closed
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
jorgefilipecosta
added
the
[Feature] DataViews
I think it should be possible to use I'm going to cc @andrewhayward as he's the person with the most expertise on this topic. You can also take a look at #56513 for reference, where I believe Andrew has been trying something similar for list view. |
|
+1 let's see what Andrew says. I'd also be a little anxious about merging this change alone since it doesn't add any benefit until the selection behavior is added? Moreover, if we include checkboxes then I'm not sure the grid item needs to be focussable, or that we need the shift+enter to select pattern. It's essentially duplicating the checkbox behavior while adding more weight to the UX? Shift+click could perhaps be a mouse-user power tool. |
|
Hi @jameskoster
This PR alone adds a focus style for the grid item container, which we were missing, and makes it focusable with an aria-label. I think this alone already makes things better. |
|
Ah, so the benefit is the announcement of the focussed item on screen readers? |
Yes thanks to the aria label and also visually showing it is focused with the border as show in the screenshot. Currently there is no visual focused state. |
oandregal
reviewed
Jan 17, 2024
| @@ -49,6 +49,8 @@ export default function ViewGrid( { | |||
| spacing={ 3 } | |||
| key={ getItemId( item ) } | |||
| className="dataviews-view-grid__card" | |||
| tabIndex={ 0 } | |||
There was a problem hiding this comment.
This means that we are nesting interactive elements, which is something we don't want, afaik. To avoid this, dataviews' list view followed how the block-editor list view works: it renders a pseudo-element that is a sibling to the other button in the DOM but, visually, it fills the space of the container. We could do something similar here.
Alternatively, a simpler small step would be to make the media field a tab stop (and clicking or pressing enter when focused takes you to edit the pattern). I believe @ntsekouras was also looking into this.
For grid, in trunk I also see that:
- There's a tab stop in the icons (such as lock or synced) even though there's no action there: I see the stable version of patterns also have it, but I understand we should not.
- The titles (which contain the link) do not create tab stops: if we make the media a tab stop with a link to edit the pattern, it sounds like this should be fine.
There was a problem hiding this comment.
Alternatively, a simpler small step would be to make the media field a tab stop (and clicking or pressing enter when focused takes you to edit the pattern). I believe @ntsekouras was also looking into this.
That's a different thing from here, but yes. I've started looking at how is best to make the preview an interactive element without being one 😅 . The current patterns page uses a button with divs inside which is not good.. Still not sure what the best approach would be..
|
I rebased to pull in the latest grid layout updates, and tweaked the focus ring to match others: 
|
|
Size Change: +25 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Thanks for working on this, @jorgefilipecosta – great to see this work happening.
Sorry to be the bearer of bad news, but unfortunately, this approach isn't going to do what you're expecting, and may well actually make the experience worse. Note While So adding As things stand, it's not clear to me what that role would be anyway; it doesn't seem like the container is actually supposed to do anything. So while adding the I can see where you're coming from though, and entirely agree that making it clearer where focus is would be a big gain. A better solution might be to use CSS (or an event handler) to make the grid item appear to have focus when the primary actionable item has focus. Happy to expand on that idea, but see this comment for what I'm getting at.
Per @ciampo's suggestion, this would be a great candidate for |
|
Closing this PR as it seems we are going for alternative approaches. Thank you to all for the insights! |
oandregal
deleted the
update/make-grid-items-tabbable-and-add-a-focus-state
branch
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.
This PR makes the container of a grid item tabbable and adds a focused state.
It will allow them to do things like select an element by pressing shift+enter when an item has focus.
It would be nice to be able to use the arrow to navigate the items on the grid. However, I did not find a component suitable for this case.
It seems like in this case, we don't follow the role of a Composite or a NavigableMenu. cc: @ciampo in case you know a component to make this case easily navigable with arrows.
Screenshots