Switching pattern categories inserter to Tabs component with arrow key navigation #60257
Conversation
jeryj
changed the title
|
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. |
|
Size Change: +455 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d0c1e4339f9e58f9e5ef1b4fe4646057c78364ff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8468795238
|
jeryj
added
[Focus] Accessibility (a11y)
jeryj
force-pushed
the
try/pattern-category-arrows
branch
from
fa581ff
to
6dc5836
Compare
jeryj
changed the title
jeryj
changed the title
|
There's a bug somewhere where:
|
|
I added the needs design feedback as I am not sure if opening the 1st category directly is a good interaction. I fear people won't bother checking out the other categories and will assume that what has opened are all of the patterns. |
There was a problem hiding this comment.
I tested this and I think it's a big improvement:
- the focus remains on the selected category
- the up and down arrows move selection to a new category
- the list of categories does not scroll on chaning categories
The current implementation is quite unpredictable and even with the mouse the focus capture by the category filter button is disorientating.
|
I'm also not convinced about auto-opening the first category. Probably one to extract and explore in detail separately? |
|
Regarding the pattern category open or closed automatically... Either I'm doing it wrong, or there is a bug where you can't have the first item of the tab panel category focused without also being active. I've asked the components team about this behavior.
Isn't the All tab all of patterns in a paginated list? |
jeryj
force-pushed
the
try/pattern-category-arrows
branch
from
ec81f3e
to
1188837
Compare
This seems to no longer be the case in this PR |
| @@ -215,6 +191,9 @@ function InserterMenu( | |||
| ! delayedFilterValue && | |||
| selectedMediaCategory; | |||
|
|
|||
| // When the pattern panel is showing, we want to use zoom out mode | |||
| useZoomOut( showPatternPanel ); | |||
There was a problem hiding this comment.
Why have we moved this here, rather than keeping it inside PatternCategoryPreviewPanel?
There was a problem hiding this comment.
Before, the PatternCategoryPreviewPanel was mounted and then the patterns switched out inside of it. Now, each panel is its own component (via the tabs component). So, if we left it within PatternCategoryPreviewPanel it would rerun the hook each time you switched categories, toggling between zoom out and edit. By moving it up a level we can preserve the right mode.
Yup - changed it yesterday since that's the behavior people think will be better. Forgot to update the PR description. Current status is waiting on response from the components team about how to move focus to the first selected tab but not activate it. |
jeryj
force-pushed
the
try/pattern-category-arrows
branch
from
d7c42bc
to
b7c495e
Compare
|
The behavior of the patterns sidebar now matches trunk. You have to press enter on your selected tab to see the patterns for that category. |
There was a problem hiding this comment.
Thanks for working on consistency in the editor 👍
There are a lot of changes in this PR which meant that I found it quite hard to review. That said, I recognise that this is completely transplanting one UI component for another and so I guess there was a need for a fair amount of small changes across a lot of files to facilitate this. If there is a way to break this down it might be better to land in smaller chunks but I appreciate that might not be practical.
Some things I noticed:
- There doesn't appear to be any visual focus on the pattern items. Did we loose this?
Video: no visual focus
Screen.Capture.on.2024-04-17.at.10-27-08.mp4
- Unsure if it's related but the animation to enter zoom out mode when select a pattern category is jarring. If it's not a bug in this PR it needs to have an Issue to track it because it's noticeable.
Video: janky animation
Screen.Capture.on.2024-04-17.at.10-29-40.mp4
- I expected to be able to use the right arrow key to move into the pattern category. Instead I had to hit enter. I'm not sure if it's the correct paradigm but it felt natural to me that if I'm using arrows to move up and down between categories I should also be able to use the arrow to enter the category itself.
- Once I select a pattern category with
ENTERI need to hitTABto move into the panel. Is that right? It felt as though focus should be auto-transferred. I noticed this comment so I assume that's expected behaviour.
Overall this is better than what we have on trunk right now. If we can sort the visual focus for keyboard I'd be ok with merging and iterating.
| ) } | ||
| </MobileTabNavigation> | ||
| ) } | ||
| { showPatternsExplorer && ( | ||
| <PatternsExplorerModal | ||
| initialCategory={ initialCategory } | ||
| initialCategory={ selectedCategory || categories[ 0 ] } |
There was a problem hiding this comment.
Are categories guaranteed to exist / be an array?
There was a problem hiding this comment.
I believe so. I didn't change how categories is used. I matched what trunk did.
| const container = useRef(); | ||
|
|
||
| useEffect( () => { | ||
| const timeout = setTimeout( () => { | ||
| const [ firstTabbable ] = focus.tabbable.find( container.current ); | ||
| firstTabbable?.focus(); | ||
| } ); | ||
| return () => clearTimeout( timeout ); | ||
| }, [ category ] ); | ||
|
|
||
| // Move to zoom out mode when this component is mounted | ||
| // and back to the previous mode when unmounted. | ||
| useZoomOut(); | ||
|
|
There was a problem hiding this comment.
Can you explain why it was ok to remove all this code? It seems it was handling zoom out and autofocus.
There was a problem hiding this comment.
This did handle autofocus and zoom out. I'd like to remove autofocus, as none of the other tab components move focus into the panel, nor is it mentioned on the tabs pattern ARIA guide. I played around with it a bit on the keyboard and quite liked not moving autofocus for being able to activate and see the patterns in a category without committing to the category. It makes browsing categories a lot easier IMO.
The zoom out mode hook had to be moved up a level to the menu.js.
| @@ -223,8 +202,50 @@ function InserterMenu( | |||
| setSelectedTab( value ); | |||
| }; | |||
|
|
|||
| const patternsTab = useMemo( | |||
There was a problem hiding this comment.
Can you explain the choice to useMemo here?
There was a problem hiding this comment.
It was memoized before: https://github.com/WordPress/gutenberg/pull/60257/files#diff-2745181e57f35c49a91ec773fd2b1740c75f87b0f74efb857dd7a08a90b064e7L158.
I can't remember why I moved it down in the file though. I'll see if I can move it closer to its previous position so that relationship in git is clearer.
| _Parameters_ | ||
|
|
||
| - _zoomOut_ `boolean`: If we should enter into zoomOut mode or not | ||
|
|
There was a problem hiding this comment.
Can you help me understand why this change is part of this PR? What required the addition of this new param?
There was a problem hiding this comment.
Previously the useZoomOut hook toggled between zoom in/out mode each time it ran. Passing a param of true/false allows us to intentionally choose if we want to use zoom out mode or not.
This param wasn't necessary in the previous set-up because the pattern category panel would unmount (turning zoom out mode off) then re mount a new category (turning zoom out mode back on). There's more context in this comment.
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Nope, it's missing on trunk too. Created an issue: #60872
Also the same animation on
I can see what you mean, but using focus to move into tabs isn't a standard pattern. There was talk about making this into a menu instead in the future. Using the arrows in that way would align more with menu pattern semantics. The tab semantics don't expect an arrow into the panel interaction though.
Explained my take on removing autofocus here: #60257 (comment)
Let's merge, as visual focus is an issue on trunk as well. The issue is created (#60872) and I'd rather not expand this PR. |
| <div className="block-editor-inserter__menu"> | ||
| <div | ||
| className={ classnames( 'block-editor-inserter__menu', { | ||
| 'show-panel': showPatternPanel, |
There was a problem hiding this comment.
I don't understand why this class name is show-panel and the toggle bool is showPatternPanel - shouldn't the class be about pattern panel as well?
There was a problem hiding this comment.
I left it generic, as the panel is a fly out of the menu for anything to get put into. For example, the media tab should get changed to this pattern as well. We could also add a secondary class that says it's a pattern panel if needed.
Yes, the new active id fallback behavior was approved in #60681 and approval for using the tabs here via #60573 (review) |
I see what you mean. It's on Trunk too. |
|
Did you consider looking at the "Media" tab as well which I believe was modeled to match the patterns inserter. It would be weird to have two different ways of implementing the same UI. |
|
Also @ntsekouras might be interested in this PR as well. |
@youknowriad - Yes, I think they should share the same interactions. |
That sounds great! I'm happy to help with reviews to apply it in media tab for consistency. It should be a very similar changeset. |
Fixes #60209
What?
Improves the accessibility and keyboard navigation of using the pattern categories list in the inserter.
Why?
The current implementation is cumbersome with too many tabstops and the panel is disconnected from the activating button, making it hard to switch between categories using a keyboard.
How?
Changes the structure of the pattern categories from a listbox of buttons that open a panel to Tabs implementation that considers all the categories as one tabstop with arrow keys to move between the categories.
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2024-03-29.at.10.03.06.AM.mov