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

Focus loss when unlocking a block from the block toolbar Unlock button #51447

Closed
afercia opened this issue Jun 13, 2023 · 8 comments · Fixed by #57229
Closed

Focus loss when unlocking a block from the block toolbar Unlock button #51447

afercia opened this issue Jun 13, 2023 · 8 comments · Fixed by #57229
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jun 13, 2023

Description

Focus losses should be avoided at all costs, as they're one of the most terrible experiences for keyboard users.

When a modal dialog closes, focus is expected to be moved back to the control that opened the modal dialog. In the editor, this is what withFocusReturn is meant for.

However, when the control that opened the modal dialog gets removed from the DOM or gets hidden with CSS, there's no place where focus can be moved back to and focus is lost. This is hte case when unlocking a block from the 'Unlock' button in the block toolbar.

Step-by-step reproduction instructions

  • Edit a post and lock a block e.g. a paragraph block.
  • Click on the paragraph content and from now on use the keyboard.
  • Press Shift + Tab to move focus to the block toolbar.
  • Use the Right arrow key to go to the Options ellipsis button. Press Enter to open the Options menu.
  • Use the Down arrow key to move focus to the 'Lock' button and press Enter.
  • The Lock block modal dialog opens.
  • Lock the block, move focus to the Apply button and press Enter.
  • The modal dialog closes.
  • Observe focus is moved back to the button in the menu that is now named 'Unlock'.
  • Press Enter to open the Lock block modal dialog again.
  • Unlock the button, move focus to the Apply button and press Enter.
  • Observe focus is moved back to the button in the menu that is now named 'Lock'. So far, so good: focus managed correctly.
Screenshot 2023-06-13 at 11 08 16
  • Lock the block again.
  • Click on the paragraph content and from now on use the keyboard.
  • Press Shift + Tab to move focus to the block toolbar.
  • Use the Right arrow key to go to the Unlock button, the one placed after the block switcher button:
Screenshot 2023-06-13 at 11 08 36
  • Press Enter. The lock block modal dialog opens. Unlock the block and press Apply.
  • The modal dialog closes.
  • The 'Unlock' button in the block toolbar gets removed. There's no place where to move focus back to.
  • Press the Tab key.
  • Observe that focus is lost and the tab sequence starts from scratch from the document root: focus goes to the Site logo button at the beginning of the editor:
Screenshot 2023-06-13 at 11 08 56

Screenshots, screen recording, code snippet

No response

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 [Package] Components /packages/components [Package] Block editor /packages/block-editor [a11y] Keyboard & Focus labels Jun 13, 2023
@afercia
Copy link
Contributor Author

afercia commented Jun 13, 2023

I can think of a few options to solve this issue.

Ideally, toolbar buttons should not be rendered dynamically. The block toolbar is an ARIA toolbar pattern implementation, where focus management is expected to have very specific behaviors. Also, the modal dialog itself expects a DOM element where move focus to when it closes. I would say the root problem here is in the design itself of this feature. The design didn't take into consideration how to implement a correct keyboard interaction.

That said:

  • I'd consider to always render the Lock/Unlock button in the toolbar. I'm not sure there are good reasons to render this button dynamically. This would be the most simple solution and would provide some better affordance. No surprises, no controls that appear and disappear.
  • Alternatively, focus should land on the most logical place. Maybe, the first button within the toolbar. This would probably require to change withFocusReturn so that it is possible to implement a fallback mechanism of some sort. Not sure this would be ideal from a components perspective.
  • Also, I'd tend to think withFocusReturn should implement some check and throw some warning when there's no DOM element where to move focus back, This would greatly help avoiding such focus losses in the future.

Pinging @ciampo and @mirka 👋 because of your experrtise about components.

@ciampo
Copy link
Contributor

ciampo commented Jun 19, 2023

I believe that there's more than one conditional toolbar button being rendered:

  • when the block is unlocked, the drag handle is shown (and the unlock button is hidden)
  • when the block is locked, the unlock button is shown (and the drag handle is locked)

As you mentioned, the simplest solution would be to always show the lock. Although I'm not sure that would be feasible because it would imply showing an extra button for every "lockable" block, and I'm not sure we want / can do that (it's out of my area). Always in the group of simple solutions, we could remove the "unlock" button from the toolbar altogether, but again — not my call, really. I wonder if @jasmussen can chime in on this aspect.

As you suggest, we could also look into the withFocusReturn hook and see if we can make improvements. For example, if the original trigger gets removed from the dom, we could try to focus the previous/next focusable element. Or at least expose a new argument for the "fallback focus restoration element" in case the original trigger is not in view.

@jasmussen
Copy link
Contributor

I understand the situation but would hope we can find creative solutions other than always showing the lock in the block toolbar. For most blocks, there won't be room to have it there, and it would also give undue prominence to the feature.

@ciampo
Copy link
Contributor

ciampo commented Jun 19, 2023

Tentative approach in #51666

@alexstine
Copy link
Contributor

The conditional button rendering needs to go away. We need to make a choice where lock/unlock should go. Either in the toolbar or in the options submenu, but not both depending on a given state.

@jasmussen Please work on the design to maybe always keep this option in the options submenu. This pattern that is constant through the editor of appearing and disappearing buttons has to stop for the better good of all users.

Do keep in mind that React does try to save focus for us but sometimes when the UI changes so much, this being one of those times, we have to add work-arounds in code to manage focus. We shouldn't need to do this much work to manage focus. This might mean that the design itself is inaccessible.

Thanks.

@afercia
Copy link
Contributor Author

afercia commented Jun 21, 2023

I'd second @alexstine about the fact the root problem here is that design didn't take into account keyboard accessibility since the beginning. I'd love to see more awareness and knowledge that keyboard must be taken into account since the design phase.

@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Keyboard & Focus labels Jul 24, 2023
@scruffian
Copy link
Contributor

I tested in Chrome, Firefox and Safari, and they all send focus to the first button in the toolbar when the lock button is removed. How can I replicate the focus loss issue?
Screenshot 2023-12-21 at 12 35 30

@jeryj
Copy link
Contributor

jeryj commented Dec 21, 2023

they all send focus to the first button in the toolbar when the lock button is removed.

That prevents a total focus loss, but to follow expected patterns, it should return focus to the button in the toolbar that opened it (the lock button).

#57229 should be a better fix for this issue, as it leaves the lock button in the toolbar until the toolbar is unmounted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block editor /packages/block-editor [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants