Navigation block: After choosing an option from Select Menu, focus after block rerender #40390
Conversation
alexstine
added
[Type] Enhancement
| // Focus support after menu selection. | ||
| useEffect( () => { | ||
| if ( | ||
| isDraftNavigationMenu || |
There was a problem hiding this comment.
Should not focus on draft?
There was a problem hiding this comment.
I don't know that this draft variable actually does anything. I introduced it a while ago, and then someone accidentally broke it without realizing, and now I don't think it'll ever be true. I didn't notice it was broken until much later.
I'm not sure what the situation is now. I'll maybe put together a PR to look at removing it 😄
| useEffect( () => { | ||
| if ( | ||
| isDraftNavigationMenu || | ||
| ! isEntityAvailable || |
There was a problem hiding this comment.
If menu is not available, don't try to focus.
| if ( | ||
| isDraftNavigationMenu || | ||
| ! isEntityAvailable || | ||
| ! shouldFocusNavigationSelector |
There was a problem hiding this comment.
The state should not be false. If it is true, that means the navigation menus have updated and I should attempt focus now.
| return; | ||
| } | ||
| navigationSelectorRef?.current?.focus(); | ||
| setShouldFocusNavigationSelector( false ); |
There was a problem hiding this comment.
Reset this to false that way this effect quits firing.
| }, [ | ||
| isDraftNavigationMenu, | ||
| isEntityAvailable, | ||
| shouldFocusNavigationSelector, |
There was a problem hiding this comment.
I wasn't sure how to properly detect this because calling it in useCallback was too early. I needed to make sure the toolbar could gain focus.
alexstine
changed the title
|
Size Change: +44 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
Following the steps outlined, I can confirm that after selecting a menu, focus remains on the button. I believe many of the folks pinged for review are AFK today, but they should be able to give it a quick code review so we can land this in short order. Thank you for the fix! |
There was a problem hiding this comment.
Thanks for working on this, Alex. I think this is good, it fixes a long standing problem.
There are still a few underlying issues in the nav block that might be good to solve. One is that the menu switcher is conditionally rendered (this code here - https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/navigation/edit/index.js#L663-L675), and one of those conditions (isEntityAvailable) causes it to be unmounted from the DOM (causing a focus loss and the menu to be closed). And then there's also #39044, which is more complicated. I don't think anyone is working on these things at the moment, so maybe I'll try to set aside some time to improve things.
While those are still problems that could be fixed, it may take a while and it's worth prioritizing user experience here. What do you think about keeping #38169 open to keep track of those other issues, but still merging your PR?
| // Focus support after menu selection. | ||
| useEffect( () => { | ||
| if ( | ||
| isDraftNavigationMenu || |
There was a problem hiding this comment.
I don't know that this draft variable actually does anything. I introduced it a while ago, and then someone accidentally broke it without realizing, and now I don't think it'll ever be true. I didn't notice it was broken until much later.
I'm not sure what the situation is now. I'll maybe put together a PR to look at removing it 😄
|
@talldan I removed the issue reference so hopefully it will not close now. 👍 Is it too late to backport this to 6.0? Yes, would be great to make sure all this unmounting/mounting stops. The navigation block is a crazy work of art. Do you think there is a way to create some type of hook or listener that can force focus the block after unmount/mount? Would not want to focus on initial render, but every time after that. Very similar to how I am doing it now but for the loading states mainly. Whenever the block is loading/updating, simply set the state to true so focus can be established again. |
annezazu
added
[Type] Bug
|
Changing this to a bug after chatting with @alexstine about the original problem itself. To be clear, this never worked to begin with so it's less that something wasn't working as intended and more that something didn't work at all. Going to tag this for backport consideration for 6.0 beta 3. |
annezazu
added
the
Backport to WP 6.9 Beta/RC
|
Sounds like a good idea to backport it 👍
I think ideally it'd just stay mounted. The block has changed quite a bit, so maybe it's easier now than it was last time I looked. I can try again. |
|
Thanks @talldan ! 👍 |
|
Thank you for all the work here! I've been on vacation but it was great to come back to this 👏 👏 👏 |
…ter block rerender (#40390) * Focus the select menu button after selection is made. * Add E2E test. * Add code comments to E2E test.
|
This was backported to wp/6.0 branch to be included in WordPress 6.0 Beta 3 tomorrow: d8a0f27 |
adamziel
removed
the
Backport to WP 6.9 Beta/RC
@talldan The placeholder is currently conditionally rendered and thus (as you note) the menu selector is mounted and unmounted according to various piece of "state". More importantly however, you wlil recall that we have two usages of the
Therefore as we switch from Placeholder -> Edit there will always be a mount/unmount of the Menu Selector. I'm not sure how we can easily avoid this as the Menu Selector is used differently in the two scenarios and thus we cannot keep it mounted. Open to ideas if there is a way but I'm not sure there is.
That one is much more complex and we didn't find time to address it alongside all the other a11y improvements and refactoring that has been going on. We should look to address it for 6.1. |
What?
Part of #38169
Why?
Accessibility is important.
How?
I track the state of useCallback and then fire useEffect once menu is done updating.
Testing Instructions
Screenshots or screencast