Fix navigation select menu focus loss #42956
Conversation
talldan
added
[Block] Navigation
talldan
requested review from
ntwb,
nerrad,
ajitbohra and
tellthemachines
as code owners
|
Size Change: -127 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
| @@ -225,13 +224,10 @@ function Navigation( { | |||
|
|
|||
| const navRef = useRef(); | |||
|
|
|||
| const isDraftNavigationMenu = navigationMenu?.status === 'draft'; | |||
There was a problem hiding this comment.
This seems to be unused. Previously, 'unsaved inner blocks' when saved were created as drafts, but that was accidentally removed months ago by some refactoring and no-one really complained, so I'll remove this as well. 😄
There was a problem hiding this comment.
Although I think @draganescu might need it back very shortly 😄
There was a problem hiding this comment.
Yes, I was exploring the idea of saving menus as drafts initially indifferent of how they're created so the user does the "final call" action when clicking save. The idea is to avoid fiddling with live site menus by mistake.
There was a problem hiding this comment.
Worth noting that there's still some code that changes the status to publish when saving wp_navigation entities in an editor:
gutenberg/packages/editor/src/components/entities-saved-states/index.js
Lines 149 to 157 in 54c9d81
There was a problem hiding this comment.
@draganescu Let me know what you'd like me to do. I can make a separate PR that removes all of the draft handling, which you can revert when you need it back. Or I can put isDraftNavigationMenu back in this PR
There was a problem hiding this comment.
I think you should proceed as things are no need for any separate PR. We'll see how trunk looks when we start changing the publish behavior 🙏🏻
| const handleSelectNavigation = useCallback( | ||
| ( navPostOrClassicMenu ) => { | ||
| if ( ! navPostOrClassicMenu ) { | ||
| return; | ||
| } | ||
|
|
||
| const isClassicMenu = | ||
| navPostOrClassicMenu.hasOwnProperty( 'auto_add' ); | ||
|
|
||
| if ( isClassicMenu ) { | ||
| convert( navPostOrClassicMenu.id, navPostOrClassicMenu.name ); | ||
| } else { | ||
| handleUpdateMenu( navPostOrClassicMenu.id ); | ||
| } | ||
| setShouldFocusNavigationSelector( true ); | ||
| }, | ||
| [ convert, handleUpdateMenu ] | ||
| ); |
There was a problem hiding this comment.
I've removed this in favour of having separate onSelectNavigationMenu and onSelectClassicMenu props for NavigationMenuSelector. It seems simpler than checking the auto_add property.
There was a problem hiding this comment.
There was definitely a reason I did it this way. I clearly failed to document that in the code so I can't remember what it was now.
I might have been that the menu selector component was used in another context and having a single handler helped to avoid overcomplicating the interface. I found
| const navigationMenu = await convertClassicMenu( | ||
| classicMenu.id, | ||
| classicMenu.name | ||
| ); | ||
| setRef( navigationMenu.id ); |
There was a problem hiding this comment.
I decided to switch to having convertClassicMenu return the resulting menu, rather than using the value from the hook. That means there's now no need to update it in a useEffect.
There was a problem hiding this comment.
I seem to remember the reason the hook works the way it does is to avoid waiting on async stuff in UI onSelect or onClick handlers.
Previously you will recall that the UI used to "hang" when the user took certain actions.
Due to exposing the status of the ongoing async operation this may no longer be such a concern. That said I'd like to ensure we've checked that we're not reintroducing old problems.
There was a problem hiding this comment.
That said I tested in the UI and I didn't notice anything hanging which is good.
There was a problem hiding this comment.
An async function won't cause the UI to hang. I think the problem before was that there was no loading indicator.
| @@ -704,20 +659,24 @@ function Navigation( { | |||
| <EntityProvider kind="postType" type="wp_navigation" id={ ref }> | |||
| <RecursionProvider uniqueId={ recursionId }> | |||
| <BlockControls> | |||
| { ! isDraftNavigationMenu && isEntityAvailable && ( | |||
There was a problem hiding this comment.
I don't think these conditions do anything important, because there are already early returns in the component for missing entities or unselected menus.
| const handleSelect = useCallback( | ||
| ( _onClose ) => ( selectedId ) => { | ||
| _onClose(); | ||
| onSelect( | ||
| navigationMenus?.find( ( post ) => post.id === selectedId ) | ||
| ); | ||
| }, | ||
| [ navigationMenus ] | ||
| ); | ||
|
|
||
| const handleSelectClassic = useCallback( | ||
| ( _onClose, menu ) => () => { | ||
| _onClose(); | ||
| onSelect( menu ); | ||
| }, | ||
| [] | ||
| ); | ||
|
|
There was a problem hiding this comment.
I don't think these need to be memoized, so I removed that. It turns out that the navigationMenus?.find( ( post ) => post.id === selectedId ) isn't needed as the onSelect callback only uses the id of the menu.
The new onSelectNavigationMenu prop can be passed directly to MenuItemsChoice
There was a problem hiding this comment.
Nice tidying although are these related to the purpose of the PR? I only ask because we're in the middle of a lot of refactoring on the block right now and we're really trying to keep our PRs as small as possible.
| menu | ||
| ) } | ||
| onClick={ () => { | ||
| onClose(); |
There was a problem hiding this comment.
Decided to keep onClose here, as converting a classic menu has a saving/loading period, and keeping the menu open while that was happening felt weird.
There was a problem hiding this comment.
Yes this is exactly that I want to avoid. We need to have the UI immediate "respond" and then make sure we have the necessary data available to the wider block to convey the progress of the async action. As you know I did a lot of refactoring in the 6.0 cycle to allow for this but these little touches help. Thank you!
| setStatus( CLASSIC_MENU_CONVERSION_PENDING ); | ||
| setError( null ); | ||
|
|
||
| return await convertClassicMenuToBlockMenu( menuId, menuName ) |
There was a problem hiding this comment.
Now we return the value and allow the caller to handle it rather than use setValue. This feels simpler to me.
I think the same could be done for errors and status too, but I've tried to keep refactoring to a minimum.
There was a problem hiding this comment.
You and I did refactor this specifically to ensure the loading states and errors were available to the wider block and not localised to the caller.
There was a problem hiding this comment.
It doesn't look like I had a lot of input into those PRs. My comment is referenced on that earlier PR, so maybe it's that - #38824 (review).
My suggestion there was to make it an async function called convertClassicToBlockMenu instead of a hook, and it looks like it went the other way. That's absolutely fine, it's just a difference of opinion.
I can go into more detail about why I feel that it shouldn't be a hook. I think the problem with relying on a hook is that creating a menu is a one time operation, but a hook generally returns values that are long lived (over multiple renders).
So I might create a menu once by calling the convert function:
const [ value, convert ] = useConvertClassicToBlockMenu( // ... );
// ...
onSelectClassicMenu={ ( classicMenu ) => {
convert( classicMenu );
} }But because of the way hooks work, value continues to be set to the value of the menu for a long time after it is actually useful. Not only that, but it has to be handled in a useEffect hook separately from the code that called it.
If more work is done in the event handler, then that value can be garbage collected and it has the net result of reducing the complexity of the main Edit function.
There was a problem hiding this comment.
Thank you for this PR. It seems to work as expected although I wasn't 100% sure from the PR test instructions how to test for the focus loss.
I noticed that when you select a Nav menu the dropdown remains open whereas when you select a classic menu the dropdown closes.
Screen.Capture.on.2022-08-04.at.08-58-12.mp4
A wider concern for me is that the scope of changes in this PR is pretty large. A number of contributors are working on the block right now and we've agreed that smaller discreet PRs are the key to avoiding problematic rebases. I notice that some of the changes seem to be beyond the scope of the PR and might be better as individual PRs to allow contributors to better assess them in isolation and also to handle the merge conflicts more easily. I appreciate it's an overhead but would you be open to breaking this up a little bit?
One of my major concerns is that with all these changes we don't regress the work that you and I did to improve the communication of loading states to the user and also avoiding reintroducing excess duplication in to the block. I've left comments which reference previous PRs from the 6.0 (or even 5.9?) cycle to help us assess whether that might be happening here. Again it would be easier if these changes were broken out for individual assessment.
Thanks again for the PR 🙇♂️
| const navigationMenu = await convertClassicMenu( | ||
| classicMenu.id, | ||
| classicMenu.name | ||
| ); | ||
| setRef( navigationMenu.id ); |
There was a problem hiding this comment.
I seem to remember the reason the hook works the way it does is to avoid waiting on async stuff in UI onSelect or onClick handlers.
Previously you will recall that the UI used to "hang" when the user took certain actions.
Due to exposing the status of the ongoing async operation this may no longer be such a concern. That said I'd like to ensure we've checked that we're not reintroducing old problems.
| const navigationMenu = await convertClassicMenu( | ||
| classicMenu.id, | ||
| classicMenu.name | ||
| ); | ||
| setRef( navigationMenu.id ); |
There was a problem hiding this comment.
That said I tested in the UI and I didn't notice anything hanging which is good.
| @@ -225,13 +224,10 @@ function Navigation( { | |||
|
|
|||
| const navRef = useRef(); | |||
|
|
|||
| const isDraftNavigationMenu = navigationMenu?.status === 'draft'; | |||
There was a problem hiding this comment.
Although I think @draganescu might need it back very shortly 😄
| const handleSelect = useCallback( | ||
| ( _onClose ) => ( selectedId ) => { | ||
| _onClose(); | ||
| onSelect( | ||
| navigationMenus?.find( ( post ) => post.id === selectedId ) | ||
| ); | ||
| }, | ||
| [ navigationMenus ] | ||
| ); | ||
|
|
||
| const handleSelectClassic = useCallback( | ||
| ( _onClose, menu ) => () => { | ||
| _onClose(); | ||
| onSelect( menu ); | ||
| }, | ||
| [] | ||
| ); | ||
|
|
There was a problem hiding this comment.
Nice tidying although are these related to the purpose of the PR? I only ask because we're in the middle of a lot of refactoring on the block right now and we're really trying to keep our PRs as small as possible.
| menu | ||
| ) } | ||
| onClick={ () => { | ||
| onClose(); |
There was a problem hiding this comment.
Yes this is exactly that I want to avoid. We need to have the UI immediate "respond" and then make sure we have the necessary data available to the wider block to convey the progress of the async action. As you know I did a lot of refactoring in the 6.0 cycle to allow for this but these little touches help. Thank you!
| setStatus( CLASSIC_MENU_CONVERSION_PENDING ); | ||
| setError( null ); | ||
|
|
||
| return await convertClassicMenuToBlockMenu( menuId, menuName ) |
There was a problem hiding this comment.
You and I did refactor this specifically to ensure the loading states and errors were available to the wider block and not localised to the caller.
| const handleSelectNavigation = useCallback( | ||
| ( navPostOrClassicMenu ) => { | ||
| if ( ! navPostOrClassicMenu ) { | ||
| return; | ||
| } | ||
|
|
||
| const isClassicMenu = | ||
| navPostOrClassicMenu.hasOwnProperty( 'auto_add' ); | ||
|
|
||
| if ( isClassicMenu ) { | ||
| convert( navPostOrClassicMenu.id, navPostOrClassicMenu.name ); | ||
| } else { | ||
| handleUpdateMenu( navPostOrClassicMenu.id ); | ||
| } | ||
| setShouldFocusNavigationSelector( true ); | ||
| }, | ||
| [ convert, handleUpdateMenu ] | ||
| ); |
There was a problem hiding this comment.
There was definitely a reason I did it this way. I clearly failed to document that in the code so I can't remember what it was now.
I might have been that the menu selector component was used in another context and having a single handler helped to avoid overcomplicating the interface. I found
|
As mentioned in the PR it ended up being bigger than expected. I think all the changes are necessary, though in some cases there are multiple ways to solve the problem. I went with what I thought best. I'll see if I can break some out into a refactor PR first. 👍 |
I noted this. Understood. It's a tricky balance. |
|
@talldan Let me know when this is ready for review. I am mostly AFK ATM, but I will check when I can. |
|
@alexstine Thanks. It should be ready for testing, but the code is liable to change until I've merged a few separate PRs. @getdave @scruffian First pure refactor PR in #43057. |
talldan
force-pushed
the
fix/navigation-select-menu-focus-loss
branch
from
a5b8f58 to
6e90091
Compare
|
Second refactor PR - #43081 |
|
Accessibility seems to be much improved. There are still options and functionality that cause focus loss, but selecting menus is no longer one of them. Thanks. |
alexstine
added
the
[Focus] Accessibility (a11y)
talldan
force-pushed
the
fix/navigation-select-menu-focus-loss
branch
from
6e90091 to
59e8a5d
Compare
Remove navigation selector ref Update e2e test Switch back to useCallback Fix test
talldan
force-pushed
the
fix/navigation-select-menu-focus-loss
branch
from
59e8a5d to
56462ff
Compare
|
There were some conflicts, so I've rebased this and solved them. #42735 has since been merged, and that introduced more places where Also, that PR was pretty huge, I don't really get why I had to divide this PR up into smaller parts. 🤷 |
I believe the reason that PR was shipped en masse was because we had to land all the functionality at once otherwise the experience would have become inconsistent. That said, I appreciate it might seem inconsistent and frustrating. I've been away on AFK for 2 weeks so I've been out of the loop so I'm not well placed to provide any further insight. All I can say Dan is that we appreciate your efforts and diligence. |
…rding to #42956 Switching between menus keeps the menu open and focused on the current selection. Importing classic menus or creating new ones focuses the current navigation block. Co-authored-by: Daniel Richards <[email protected]>
…rding to #42956 Switching between menus keeps the menu open and focused on the current selection. Importing classic menus or creating new ones focuses the current navigation block. Co-authored-by: Daniel Richards <[email protected]>
…rding to #42956 Switching between menus keeps the menu open and focused on the current selection. Importing classic menus or creating new ones focuses the current navigation block. Co-authored-by: Daniel Richards <[email protected]>
* adds a list view with the navigation block inner blocks to the block's inspector * adds a first toolpanel item * stub toolpanel item * moves the menu selector block controls to the inspector * fixes label update bug * labels navigation menu selector if no menus or deleted menu, fixes label when importing classic menu * adds the chevron * only show no menus label if no block menus exist, disregard classic menus * adds check for permissions on the manage menus link; removes rogue showManageActions * Implements handling focus persistence in the Navigation Selector according to #42956 Switching between menus keeps the menu open and focused on the current selection. Importing classic menus or creating new ones focuses the current navigation block. Co-authored-by: Daniel Richards <[email protected]> * It looks like `hasManagePermissions` is not cached between API calls so we refresh this quite often when switching between menus. For a more consistent UI experience we disabe the menu management link untill `hasManagePermissions` is refreshed. Another option would be to cache this at a component level. But, this could be more accurate? * Closes the dropdown when importing or creating new menu as in #43529 Fixes slow connection UX by disabling and enabling the selector and the manage menus link depending on data status. Co-authored-by: Dave Smith <[email protected]> * Larger commit, adds: - The label of the currently selected menu updates if menu is renamed from advanced - Uses a visually hidden label to explain this is a menu switcher - Defaults to a button if there are no block menus and no classic menus - Renames "Loading options ..." to "Loading ..." Co-authored-by: Javier Arce <[email protected]> Co-authored-by: Alex Stine <[email protected]> * move styles to editor.scss as they are only needed for the editor * use flex rather than absolute positioning * alphabetize CSS properties * Incorporates review feedback: - replaces disabled with isBusy for toggle working state - refactores some conditionals for better readability - adds consistency to the toggle label Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Javier Arce <[email protected]> Co-authored-by: Paal Joachim Romdahl <[email protected]> Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Javier Arce <[email protected]> Co-authored-by: Alex Stine <[email protected]> Co-authored-by: Ben Dwyer <[email protected]> Co-authored-by: Paal Joachim Romdahl <[email protected]>
|
Closing as this is now outdated and the UI has changed. |
What?
Fixes #38169
After #42916, it's now possible to fix this issue properly. The previous temporary fix in #40390 had the trade-off that it wasn't possible to keep the dropdown menu open when switching menus, but with this fix that's now possible.
How?
This a much bigger change than I anticipated, but it's good that it resulted in more code removed than added.
I had to remove:
selectBlockcalls when switching menus in some places. This particular point required quite a lot of refactoring, and I found I could delete quite a bit of code anyway.Testing Instructions
Screenshots or screencast
Before
Kapture.2022-08-03.at.17.41.12.mp4
After
Kapture.2022-08-03.at.17.36.43.mp4