Add block sidebar toggle to block settings menu #65185
Conversation
getdave
added
[Type] Enhancement
getdave
changed the title
| addFilter( | ||
| 'editor.BlockEdit', | ||
| 'core/editor/with-block-settings-inspector-toggle', | ||
| withBlockToolbar | ||
| ); |
There was a problem hiding this comment.
I'm unsure as to whether a hook is the best option here. I'm principally concerned about performance impacts of block edit hooks which I know can be problematic.
As an alternative, we could try adding the component here:
gutenberg/packages/editor/src/components/provider/index.js
Lines 301 to 318 in 8b4ca34
This would avoid it running on each block.
There was a problem hiding this comment.
@youknowriad I would value your perspective as to whether menu items that apply to all blocks in the Editor (via the @wordpress/editor package) are better included as a component in the Provider? Or is using a hook (as currently in this PR) suitable?
I've noted my concerns around performance of the hooks approach which is why I am suggesting this alternative.
|
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. |
|
@spencerfinnell in case you are interested. |
|
@jeryj I noticed that when we toggle the complementary area focus is not moved to it. I think that's correct as you're interacting with a menu and therefore focus should remain on that menu item and the menu should remain open. |
|
Taking for a quick spin, here'a a GIF showing the new "Open Block settings" menu item:
The fact that it becomes a grayed menu item when the menu is open is the main gotcha. There's a discussion about focus transferrance right above, but it does seem useful to transfer focus to the inspector; in doing so the button would never need to be disabled, because transferring focus there would always be useful. |
|
Size Change: -9.51 kB (-0.53%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
@jasmussen Here is the alternative approach suggested by @mtias which was to remove the menu item if the sidebar is open. It also has some gotcha's in terms of the item disappearing once clicked. This is likely bad for a11y (confusing, loss of focus...etc) and UX in general, but I'd be curious as to your thoughts... Screen.Capture.on.2024-09-10.at.10-23-05.mp4 |
|
that would be the other approach, but I still suspect a fundamental underpinning relates, as you imply, to where focus goes. If it goes to the inspector, shouldn't the menu also close rather than stay open? |
|
Transferring focus to the Inspector seems like it could work, especially as shift+tab returns focus to the block. |
|
Thanks for trying this one! It seems the flow should include 1) open the sidebar 2) transfer focus 3) close the dropdown in some order. If the dropdown doesn't close it, it creates a weird state where you either remove the item or keep it disabled. |
👍
Yes the flow feels strange as it currently stands. My main concern was a11y. I'll work with @jeryj on this to find a good solution along the lines of what you (and others) have suggested. |
There was a problem hiding this comment.
I think this is working well to expose that there are more block settings. Since the block settings are so close tab-stop wise (only takes two tab spots), I think moving focus to the settings when you click may cause more accessibility issues than it helps. I'm unsure on it though. @joedolson do you have thoughts on if we should transfer focus to the block settings sidebar after clicking this? It seems odd to click an option within a menu to transfer focus to a non-modal. But at the same time I can see how it could be useful.
|
With the text 'Open block settings', I think that focus should not be moved. If the text were 'Go to block settings', then I think moving focus would be fine. But I think that given the short keyboard path, just opening the settings is fine. Moving focus would be jarring to users who weren't expecting a focus change, and I think there are many use cases where a user might want to open the block settings without going to them. |
|
Thank you for your insight @joedolson. This aligned with my understanding of a11y best practises. I propose we move forward with the current state of this PR. To that end I'm now seeking a approving review from @WordPress/gutenberg-design. @jasmussen Here are some responses to your initial review:
I'm concerned that this will significantly deprioritise this menu item. This significantly undermines the purpose of the PR which is to make it easier for users to discover the full block settings (something we repeatedly see is a source of confusion for users). It's clear we do not have buy in for adding a permanent button to the block toolbar. Therefore I'm keen to emphasise the need not to further deprioritise the discoverability of the tool.
Contributors from the a11y team have confirmed that this is expected behaviour. That doesn't diminish the other valid concerns you've raised but it's important to consider this when we're designing the optimal solution. I sense you are concerned that leaving the menu open on click might be confusing for sighted users? Personally I don't believe this will be an issue - their attention will be drawn to the freshly opened panel with the menu disappearing as soon as they interact. If necessary I suppose we could try and detect when the mouse has moved outside of the block and close the menu in that case? But I fear it would introduce more problems than it would solve.
Happy to change that now. |
|
I'll defer to others, including Joe and Matías. |
|
@mtias how about instead of generic words like settings we actually use the thing that is found in the inspector and it's not found in the toolbar: settings and styles. So, instead of one item we add two, "Block settings" and "Block style". This would also justify what @jasmussen suggests to add them lower in the menu in separator jail. It would also allow focus transfer. |
I like the idea of exposing both of these.
I'm not sure how this changes the idea around focus transfer. Can you elaborate? |
|
Separate buttons was also suggested here: #65118 (comment) |
Unless I'm mistaken @draganescu is suggesting putting x2 items within the block options menu rather than as top level items in the block toolbar. |
I meant that being more specific we can use phrasing like "go to" and we focus a more direct place. |
|
One thought I had here was this
One issue with this approach is that we don't have any good mechanism for programmatically transferring focus to specific parts of the editor. |
|
@jameskoster @mtias any objections to having two items and opening the settings or styles directly? This would allow us to both segment that menu and also word the labels in a way that allows for focus transfer. |
2 things I'm thinking:
|
|
@draganescu I think a direct link to "styles" should be a separate exploration to this |
|
@draganescu might try adding separate menu items in a separate PR. Thanks Andrei 👍 Back to this PR - any objections to my proposal? |
It should be one consistent action. It would be very confusing why focus moves in one scenario and not the other. I think the options are:
|
This is what I was trying to suggest 😅 But with the caveat that the visible label will always stay the same and only the aria label would change as the additional context is - in my opinion - really only required for non-sighted users. Note that this idea was based on an idea I saw @alexstine suggest in #65603 (comment). |
|
That would bring us back to the exact state we removed before? Doesn't seem great and makes the UI a bit confusing. |
|
If the |
|
Thanks for the feedback. We could refine the implementation of the aria attributes. I don't see a way forward here. We have two opposing requirements:
My proposal is the middle ground.
If there is a better solution available that satisfies the requirements outlined in the discussion above then I'm happy to look into it (maybe what @draganescu suggested - although that doesn't to have much support). Otherwise I think we may have to close this out. |
|
@getdave I'm not sure I follow where those requirements are coming from. It seems that if the sidebar is closed, we need to trigger an open sidebar and a focus transfer, similar to inserter button in the editor header. And when the sidebar is open, avoid rendering the button in the ellipsis menu. I understand the order of operations can be tricky and it's fine to revisit this later. |
I'm getting those (as loose requirements) based on the discussion above. Specifically this comment from @joedolson which outlines why moving focus could be jarring. That is to be juxtaposed with your recommendation which I appreciate you've made several times here now. |
What?
Adds ability to access Block Inspector Controls sidebar from block settings menu.
Alternative / complementary to #65118
Why?
As discussed in #65118 it's very difficult for users to find the block sidebar.
This is one step towards making that easier again as suggested by @mtias in #65118 (comment).
How?
Adds as first item in block setitngs dropdown. Disables if the sidebar is already opened.
Note I tried removing the menu item if the toggle is open but the experience seemed quite jarring as the menu remains open even after you toggle it resulting in the item you just clicked being removed from the dropdown menu.
Video showing remove on click
Screen.Capture.on.2024-09-10.at.10-23-05.mp4
Testing Instructions
Please ensure you read the
Howsection above.Testing Instructions for Keyboard
Same instructions as above but use keyboard to navigate.
Must ensure that focus is logical and no focus losses occur.
Screenshots or screencast
Screen.Capture.on.2024-09-10.at.10-29-40.mp4