Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix focus loss regression in menu creation #59771

Closed
afercia opened this issue Mar 12, 2024 · 13 comments · Fixed by #59801
Closed

Fix focus loss regression in menu creation #59771

afercia opened this issue Mar 12, 2024 · 13 comments · Fixed by #59801
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Type] Regression Related to a regression in the latest release

Comments

@afercia
Copy link
Contributor

afercia commented Mar 12, 2024

Description

#59630 introduced a regression that is now in core for WP 6.5 so this would need to be addressed for the release.

Previously:
In a navigation block, when clicking the 'Create new menu' in the dropdown in the inspector:

  • The menu immediately closed.
  • Focus was moved back to the ellipsis toggle button.

Thie made sure there was no focus loss.

After #59630

  • The dropdown menu stays open.
  • All the menu items in the dropdown get disabled, not sure why.
  • When using a keyboard, the 'Create new menu' item has focus but it gets disabled so there is a focus loss.
  • After the menu is created, the dropdown closes ad focus is lost.

I'm not sure wjy the menu stays open in the first place, this doesn't seem to be the best user scperience.
Anyways, keeping the menu open and disabling all its menu items doesn't allow the menu built-in focus management to work properly.

See animated GIF below to better illustarate the focus loss.

Step-by-step reproduction instructions

  • Go to the Site editor and edit a Navigation block.
  • In the settings panel (the inpector) use the keyboard to tab to the ellipsis button after the title 'Menu'.
  • Press Enter to open the dropdown and use the arrow keys or Tab key to move focus to the 'Create new menu' button.
  • Press Enter.
  • Observe the menu stays open. Expected: the menu to close and focus to be moved back to the ellipsis button.
  • Observe all items within the dropdown menu get disabled.
  • Wait the new menu to be created and the dropdown to close.
  • Press the Tab key.
  • Observe focus is lost and, depending on the browser, th tab sequence either starts from the document root or from the top of the settings panel.

Screenshots, screen recording, code snippet

focus loss

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Regression Related to a regression in the latest release [Block] Navigation Affects the Navigation Block [Package] Edit Site /packages/edit-site Backported to WP Core Pull request that has been successfully merged into WP Core labels Mar 12, 2024
@afercia
Copy link
Contributor Author

afercia commented Mar 12, 2024

@priethor @youknowriad @annezazu this is a regression in RC as #59630 has been cherry-picked to the pick/wp-65-rc-2 branch. Can we please fix this regression for the release? Thanks.

@draganescu
Copy link
Contributor

I can work on this right away.

@draganescu
Copy link
Contributor

The bug was introduced because the right behaviour was implemented: we now await the async menu operations (create and import). So the onClose is called later, the menu items get disabled correctly, so focus loss. It is not a regression as much as it is unearthing a problem that was there from the start but hidden: we should maybe close the dropdown immediately after an option is picked.

I think the easiest solution is to call onClose right away. Spinning up a PR.

On a separate note, I am not very convinced about the disabling of the options as a good UX. But we do not have enough time to undo that bit during RC. However, ideally we should be able to intercept something that happens, stop it and start a new action, instead of disabling UI for fear of concurrent actions.

@draganescu draganescu self-assigned this Mar 12, 2024
@getdave getdave removed [Type] Bug An existing feature does not function as intended Backported to WP Core Pull request that has been successfully merged into WP Core labels Mar 12, 2024
@getdave
Copy link
Contributor

getdave commented Mar 12, 2024

Noting that the Backported to WP Core label is reserved for release purposes only. I appreciate it may have been added in error.

@afercia
Copy link
Contributor Author

afercia commented Mar 12, 2024

The bug was introduced because the right behaviour was implemented: we now await the async menu operations (create and import). So the onClose is called later, the menu items get disabled correctly, so focus loss. It is not a regression as much as it is unearthing a problem that was there from the start

I disagree. The expected behavior for a dropdown menu is to:

  • Close when a menu item is clicked.
  • Move focu back to the toggle that opened the menu.

In WordPress 6.4 this works correctly.
On trunk before #59630 this works correctly.
Now after #59630 it doesn't work and a new focus loss is triggered.

As such, to me, this is a clear regression.

@afercia
Copy link
Contributor Author

afercia commented Mar 12, 2024

Noting that the Backported to WP Core label is reserved for release purposes only. I appreciate it may have been added in error.

Thanks @getdave, good to know. What is the correct label to have this fixed for the 6.5 release? Thanks.

@getdave
Copy link
Contributor

getdave commented Mar 12, 2024

You would need to raise with the Editor Tech Leads for this release. Luckily I'm one of them and I'm happy that we should look at addressing this Issue so I'll add it to the board on that basis.

Then any PR that is raised would need the Backport to WP Beta / RC label.

@getdave
Copy link
Contributor

getdave commented Mar 12, 2024

@fabiankaegy @annezazu @youknowriad I added this to be resolved during RC3.

@draganescu
Copy link
Contributor

@afercia in 6.4.3 on create new menu the focus goes to the block, not to the toggle.

@jeryj
Copy link
Contributor

jeryj commented Mar 12, 2024

I've tested this in Gutenberg and in 6.4.3. It looks like the block is focused because of the blue ring on it, but focus is lost in both instances. So, I don't think it's a regression based on the current terms, but it is a serious focus loss bug so we should address it as quickly as possible.

I don't know what that means for if it can be added to the 6.5 release or not, but I'll start working on a fix and leave the backporting decision up the release leads.

@getdave
Copy link
Contributor

getdave commented Mar 13, 2024

Thanks for diagnosing and looking to fix this @jeryj 👍

I don't know what that means for if it can be added to the 6.5 release or not, but I'll start working on a fix and leave the backporting decision up the release leads.

Let's get the PR merged to Gutenberg trunk and then it can be discussed with editor release leads: myself, @fabiankaegy @annezazu and @youknowriad.

@draganescu
Copy link
Contributor

#59801 fixes this by making it so that on import and create the focus moves to the block. This was already supposed to work like this but due to some markup replacement it was broken. We should include this fix because any focus loss which is known and fixed is high priority.

Note: when switching navigation menus, the focus goes to the drop down menu toggle. This is intended as it's likely to want to switch again, whereas on create and on import it's likely to want to edit.

@annezazu
Copy link
Contributor

We should include this fix because any focus loss which is known and fixed is high priority.

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Type] Regression Related to a regression in the latest release
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants