Open talldan opened 4 years ago
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.
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 - https://github.com/WordPress/gutenberg/issues/19057.
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.log
s 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.
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.
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.
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 menuitemradio
s and not menuitem
s. I found this code that's used for keyboard navigation, but it only check's for menuitem
s 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.
Remove the regression label as this is on Gutenberg for a long time now, in 5.4 and possibly even 5.3.
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.
@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.
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.
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.
👋 ended up here because of the [Priority] High
label. What's the status of this one? Should we prioritise it for WordPress 5.7?
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.
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.
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.
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).
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
).
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:
Dropdown Menus:
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:
Menus:
Additional context