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

Modal Component: Up / Down Keys close modal spawned from editor canvas #22940

Closed
jeryj opened this issue Jun 5, 2020 · 7 comments
Closed

Modal Component: Up / Down Keys close modal spawned from editor canvas #22940

jeryj opened this issue Jun 5, 2020 · 7 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components

Comments

@jeryj
Copy link
Contributor

jeryj commented Jun 5, 2020

KeyPress events from the modal on the editor canvas are bubbling to editor. This means that when you press the up or down arrow keys:

  • the arrow keypress bubbles to the editor canvas,
  • which uses the arrow keys to navigate blocks,
  • which then closes the modal because the focus of the new block is outside the modal.

To reproduce
You can see this by trying to resolve a broken block within the editor canvas.

  • Add a paragraph block
  • Click Edit HTML (from the triple dot more menu in the toolbar)
  • Add some invalid HTML to break the block, like <p> <p>Text</p>
  • On the broken block, click "Resolve" which opens a modal from within the context of the editor canvas
  • Press an up or down arrow
  • The modal is closed

Expected behavior
The modal should only be closed Escape keypress. Other keypresses should not bubble outside the modal. Global keypresses should still be able to be used, like Save (command + s).

Potential Solution

I'm not sure if this has unintended consequences, but adding a e.stopPropagation() at the end of the modal keydown handler, it seems to work to block the arrow keys from closing the modal while still a blocking Up/Down from closing the modal.

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/modal/frame.js#L57-L61

@jeryj jeryj added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Jun 5, 2020
@WunderBart
Copy link
Member

I'm not sure if this has unintended consequences, but adding a e.stopPropagation() at the end of the modal keydown handler, it seems to work to block the arrow keys from closing the modal while still a blocking Up/Down from closing the modal.

This works well for me!

At the same time, I'm wondering if turning the modal into a single (global) instance wouldn't fix and possibly prevent issues like this? We already control its visibility via a global state. We could also use openModal to pass extra props and maybe slotFill to control its content? Another advantage would be that we wouldn't need to import it every time we need to use it.

@jeryj
Copy link
Contributor Author

jeryj commented Jun 8, 2020

@WunderBart Using a single global instance makes sense to me. That's a pattern I'm used to seeing. I assume this was considered when the component was being built, so it would be interesting to know the context of how the Modal was built.

@jeryj
Copy link
Contributor Author

jeryj commented Aug 12, 2020

This would likely be fixed by #19879.

@afercia
Copy link
Contributor

afercia commented Aug 12, 2020

Maybe related: #18537 (for the modals part).

Also to verify if this happens only with Safari + VoiceOver.

@jeryj
Copy link
Contributor Author

jeryj commented Aug 13, 2020

Maybe related: #18537 (for the modals part).

I bet that s related. Thanks for linking, @afercia.

Also to verify if this happens only with Safari + VoiceOver.

This instance will happen with or without VoiceOver. It seems to affect all browsers, even without a screen reader running.

@stokesman
Copy link
Contributor

I just tried on trunk and can't reproduce this. I tried with both the iframed and non-iframed canvas. I don't know what changed but it seems focus does not escape the modal any more when using the arrow keys.

@jeryj
Copy link
Contributor Author

jeryj commented Sep 5, 2023

Thanks for testing it again, @stokesman! I can't reproduce it either.

@jeryj jeryj closed this as completed Sep 5, 2023
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] Components /packages/components
Projects
None yet
Development

No branches or pull requests

4 participants