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

Keyboard accessibility problems with dropdowns and modals when using Voiceover and Safari #18537

Open
talldan opened this issue Nov 15, 2019 · 17 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility [Priority] High Used to indicate top priority items that need quick attention

Comments

@talldan
Copy link
Contributor

talldan commented Nov 15, 2019

Not sure how/when this occurred, but it seems to be affecting lots of the UI.

Describe the bug
Attempting to use arrow keys in modals and dropdown menus with voiceover active can cause focus to shift outside of those elements resulting in the unexpected closing of the modal or menu.

Furthermore, left/right arrow keys sometimes causes individual characters to be read by Voiceover.

To reproduce
Modals:

  1. Enable Voiceover
  2. In Safari, load the editor and open a modal (i.e. Block Manager)
  3. Continuously press the up arrow key
  4. Observe that the modal eventually closes

Dropdown Menus:

  1. Enable Voiceover
  2. In Safari, open the editor's 'More tools & options' menu
  3. Continuously press the up arrow key
  4. Observe that the focus jumps around and the menu eventually closes

Expected behavior
Focus is constrained to the modal/menu with arrow keys cycling between elements in menus.

Screenshots
Screenshots show the behaviour when pressing only arrow keys

Modals:
modal

Menus:
menu

Additional context

  • Safari v13.0.3
  • Gutenberg v6.9.0
@talldan talldan added [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 labels Nov 15, 2019
@gziolo
Copy link
Member

gziolo commented Dec 9, 2019

I’m observing some issues with the focus moved to the first element when tabbing inside the block toolbar. It’s also very random. There might be some issue with unnecessary re-renders of the whole layout. We probably should test against the change which refactored the Layout component.

@enriquesanchez enriquesanchez added the Needs Accessibility Feedback Need input from accessibility label Apr 30, 2020
@talldan talldan added the [Priority] High Used to indicate top priority items that need quick attention label May 1, 2020
@talldan
Copy link
Contributor Author

talldan commented May 1, 2020

Raising the priority of this, as a few people have mentioned it, and it seems to affect lots of the UI when using Voiceover.

One issue that makes it hard to debug this is that sourcemaps are not working in Safari - #19057.

@tellthemachines
Copy link
Contributor

I've done a bit of testing, and can reproduce this issue on all editor modals and dropdowns, using any of the arrow keys, but only on VoiceOver with Safari. On VO with Firefox everything works fine (I can't get VO to work at all on Chrome for some reason).

Also, the modal only closes when using the arrow keys in isolation, not when used in combination with Ctrl + Opt (the default VO keys used, with the arrow keys, to navigate text).

Adding a few console.logs in the modal component, I was able to verify that handleFocusOutside is being triggered by focus moving to the <body> element when arrow keys are pressed. I'm not sure why this is happening, but it seems to be related to withFocusOutside HOC, because it stops happening if I remove it from the Modal component.

@tellthemachines
Copy link
Contributor

After some more testing and debugging, I think this is what's happening:

  • withConstrainedTabbing only stops focus from moving out of the modal if we’re using the tab key. But VoiceOver enables moving focus around with the arrow keys, so it’s easy to move focus outside of the modal. This is compounded by VO more or less ignoring aria-hidden: true, so it still accesses elements where that property is set. ( I suspect Windows screen readers work better because they’re stricter at this.)

  • the second problem is we’re closing the modal when focus is detected outside of it, instead of closing it when a mouse click is detected on the overlay. But with a modal it shouldn’t be possible to ever move focus outside while it is open, so we'd be better off closing it on click outside.

So to fix this for modals we can try to prevent focus of any type from moving outside the modal when it is open, and we can make it close on click outside. I think ideally we should do both.

With dropdowns we should still close them on focus outside, because it should be possible to move focus outside (if we’re navigating with a screen reader links menu for example). So it'll be trickier to make them work. We might want to approach each component in a separate PR.

@afercia
Copy link
Contributor

afercia commented Jul 18, 2020

Tested a bit and this seems a pretty serious buggy behavior. Serious at the point the More menu and all the modals appear to be unusable with Safari and VoiceOver.

Raising the priority of this, as a few people have mentioned it, and it seems to affect lots of the UI when using Voiceover.

I'd totally agree this needs to be addressed soon. It's arguable to release WordPress 5.5 with this buggy behavior and force users to wait for a few months for this issue to be solved in a new release.

Specifically on the two issues:

More menu:
Seems to me at some point there's a focus loss. In most of the cases (always?) it seems to happen when arrowing on the "Visual editor" and "Code editor" items. I do see at some point focus fallbacks to the popover content element, which has a tabindex="-1" attribute.

I don't know why the focus loss happens in Safari but I tried to remove the SVG icons from MenuItemsChoice and that seems to improve the behavior a lot. I'm guessing Safari and VoiceOver don't like SVG icons within a menuitemradio?

Anyone have a chance to test if removing the SVG icons improves things on their side? Thanks!

Modals:
I have no idea what's going on with the modals 🙂
I don't think it's related to withConstrainedTabbing. I'd tend to think there's some keydown event somewhere that conflicts with the expected modals behavior. But it's terribly complicated to debug.

@talldan
Copy link
Contributor Author

talldan commented Jul 24, 2020

In most of the cases (always?) it seems to happen when arrowing on the "Visual editor" and "Code editor" items.

@afercia I think you've hit the nail on the head there. It looks like the root cause is that these are menuitemradios and not menuitems. I found this code that's used for keyboard navigation, but it only check's for menuitems before calling preventDefault:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/navigable-container/container.js#L100

That preventDefault seems to have the effect of stopping voiceover from performing its own navigation through text. Makes sense as the arrow key can't do two things at once. Seems like this code should also be checking for other menu item roles, not just plain menuitems.

That would seem to imply that this has been broken since those roles were changed, which was a long time ago 😬.

Working on a PR now to fix things.

I haven't made any progress on modals, but it might be that a similar preventDefault is missing.

@youknowriad youknowriad removed the [Type] Regression Related to a regression in the latest release label Jul 24, 2020
@youknowriad
Copy link
Contributor

Remove the regression label as this is on Gutenberg for a long time now, in 5.4 and possibly even 5.3.

@afercia
Copy link
Contributor

afercia commented Jul 27, 2020

Question related to process: is the [Type] Regression label usage limited in time? This is a regression compare to previous behavior and it was even more a regression when this issue was opened on 15 Nov 2019, which is more than 8 months ago.

I don't see specified anywhere the label [Type] Regression should be used only in the regression happened in the immediately previous WordPress version.

@afercia
Copy link
Contributor

afercia commented Jul 27, 2020

@talldan thanks!

Seems like this code should also be checking for other menu item roles, not just plain menuitems.

Definitely sounds like something to try.

@youknowriad
Copy link
Contributor

Regression is not limited in time but it's not obvious that this was ever a regression, that's why I removed it. Anyway, it's fixed now.

@afercia
Copy link
Contributor

afercia commented Jul 27, 2020

Thanks for the clarification and thanks @talldan for the PR (which I just saw... going through my inbox, bear with me please)

However, only the part related to the menus is fixed. The part related to the modals isn't. So either this issue should be reopened or a new one with the same priority should be created. At this point of the release cycle I think it would be quicker to just reopen this issue.

Aside: quickly testing, seems to me this regressed in WordPress 5.3.

@noisysocks
Copy link
Member

👋 ended up here because of the [Priority] High label. What's the status of this one? Should we prioritise it for WordPress 5.7?

@tellthemachines
Copy link
Contributor

I'd love to see this in for 5.7!

I can still reproduce the issue on certain modals such as Block Manager and the List view in the Navigation block. Pretty sure this is the explanation, and in my view it makes sense to have modals close when click is detected outside, as opposed to focus. But I'm sure there were reasons to detect focus outside in the first place, so would be good to know more about that before making the change.

@youknowriad
Copy link
Contributor

and in my view it makes sense to have modals close when click is detected outside, as opposed to focus

Isn't this a bit inconsistent? I mean "clicking outside" is just a way to "focus outside" for users using the mouse?

But VoiceOver enables moving focus around with the arrow keys, so it’s easy to move focus outside of the modal

This sounds like the real issue for me. So do you think it's possible that withConstrainedTabbing is in fact not concerned about "tab" keys but more about "focus navigation" which mean it should also prevent "arrow keys" to navigate outside and instead loop inside.

@tellthemachines
Copy link
Contributor

This sounds like the real issue for me. So do you think it's possible that withConstrainedTabbing is in fact not concerned about "tab" keys but more about "focus navigation" which mean it should also prevent "arrow keys" to navigate outside and instead loop inside.

You're right, this is a better approach 😄 because focus shouldn't really be leaving the modal at all. But I'm not sure we'll be able to leverage withConstrainedTabbing for this, because as it stands it checks for focus transfer to the first or last focusable elements inside the modal, and arrow key behaviour in VoiceOver is much more erratic than that: those modals I mentioned above will close after a couple of Right arrow keypresses, even if focus hasn't arrived at the bottom yet.

We need to stop focus from leaving the modal unless the close button is clicked or the Esc key pressed, but we also don't want to interfere with VoiceOver's default Left/Right arrow key behaviour, which reads out text letter by letter.

@diegohaz
Copy link
Member

diegohaz commented Aug 20, 2021

I recorded this video a while ago talking about focus management when using React Portal in the context of Gutenberg: Focus management with React Portal.

That's not directly related to this issue, but I think there are some insights there that could be useful here.

The problem with focus trap elements, as we're implementing today, is that Safari/VoiceOver doesn't move the virtual cursor (the area with the black borders) when they get focused. In the video, I mentioned we should set a timeout before moving focus between the popup elements and the focus trap elements. This works sometimes, but it's not really consistent. The exact necessary delay may vary for unknown reasons.

After doing some experiments, I came to the conclusion that the best way to handle popups, especially when they use React Portal, on Safari/VoiceOver is to treat them all as modals and hide the elements behind from the accessibility tree using aria-hidden="true", even when the elements behind are accessible to keyboard and mouse users, instead of relying on focus trap elements (focus traps are still useful for keyboard users, but they won't prevent screen reader users from accessing the content outside, because they're still visible to the screen readers virtual cursor).

@talldan
Copy link
Contributor Author

talldan commented Mar 9, 2023

Just tested and this is still a bug for modals when using Safari/Voiceover.

After doing some experiments, I came to the conclusion that the best way to handle popups, especially when they use React Portal, on Safari/VoiceOver is to treat them all as modals and hide the elements behind from the accessibility tree using aria-hidden="true"

I believe the block editor does this for modals, but VO is still selecting text in aria-hidden parts of the UI (the wpwrap element becomes aria-hidden).

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). Needs Accessibility Feedback Need input from accessibility [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

No branches or pull requests

8 participants