useNavigateRegions: Use closest to determine the next region to navigate to #44883
Conversation
alexstine
added
[Type] Bug
alexstine
requested review from
draganescu,
talldan,
ciampo,
MarcoZehe,
joedolson,
ellatrix and
tellthemachines
|
Size Change: +11 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
|
Maybe @afercia could help with accessibility feedback here? Or really anyone with a Windows + NVDA setup (I don't currently have one) |
|
Thanks for the ping. However, I'm not sure what kind of problem we're trying to solve. useNavigateRegions was designed to emulate screen readers native landmarks navigation. It's not primarily designed for screen reader users. Instead, it aims to provide keyboard users who don't use a screen reader a navigation tool similar to the screen readers native one. Worth reminding all screen readers provide landmarks navigation via dedicated shortcuts, for example:
Improvements are always welcome but at first glance seems to me it's important to keep useNavigateRegions as close as possible to the native screen readers feature and not introduce changes. Will test to check if there's anything broken in the current implementation (the focus style is for sure) and then test the proposed changes. |
This was my gut feeling too — I usually prefer not to interfere with standard browser/screen reader behavior when possible! I'd be curious to hear more in details why these changes are being proposed in the first place, and if anyone else could also chime in to help assess the potential problem and its proposed solution. |
|
These changes are being proposed because the current behavior is not accurate. I have recorded a screen share with audio to show how and why this should be improved. @afercia The reason why we should not rely on landmark navigation with screen readers is also explained. Gutenberg requires stuff the accessibility spec is still lacking on so I have found a work-around just like other apps including Slack, Teams, and many others. Having a centralized way to move around that is not screen reader specific is a win every time. https://drive.google.com/file/d/1DKeUpy26nr-fNiXkEmTl4X1zFbgRzLVh/view?usp=sharing Hope this helps. I am still planning on refining this a bit further. Just have not had the time. Thanks. |
|
Okay, this is a complete solution now.
I honestly do not expect most people will understand this, it is fairly high-level workings of how screen readers work. However, I do hope this gets further review because I believe this could impact Gutenberg for the better. Thanks. |
|
As you mentioned, reviewing this PR properly requires the reviewer(s) to have deep familiarity with how a screen reader works — since I don't, I will keep an eye on this PR and facilitate the review process if needed but I don't think I can be the main reviewer. A couple of things that I wanted to point out:
|
|
@ciampo Answers.
No, this is common pattern across all screen readers. NVDA and Voiceover are free, that is why I suggest testing with them.
What I am trying to explain is the current version does not follow such standards. Placing focus on a negative tabindex of a region is not how any other screen reader handles it. Slack and Teams got this right. The E2E tests are beyond me right now, I just want to get some feedback started. I will also give this a test with Voiceover and JAWS over the weekend. The important take-away here is this does not follow standards, my PR makes it follow more closely to what those standards are. Now it can be the same for everyone, screen reader or not. Thanks. |
|
@ggordon-vispero If you have some spare time, could you please sanity check this one for me? I believe this brings our region navigation more in-line with how screen readers work. Feel free to disagree. You can test at: http://gutenberg.run As mentioned earlier in the PR, I tried to replicate what Slack has been doing with F6. I really enjoy how it places focus on the first tabbable since it can assist in switching screen reader mode automatically. Please let me know if you need more context. Thanks. |
|
@alexstine thanks for the video and the clarifications. I know that requires patience and some effort. To my understanding, we can maybe split this in 4 main points:
On a general note, as mentioned earlier, this feature was originally designed for sighted keyboard users. I'm not sure optimizing this feature for screen readers is the way to go. We need to cover diverse needs of a divers range of users. I do believe that, in the long term, education is way more effective. I get one of your concerns is that many screen reader users don't know how to use native landmarks navigation. It would be great to educate them by the means of some good help guide instead of implementing hyper-technical solutions. Native features are always better. That said, I'm all for improvements. There are a few problems though. NVDA reads the entire content of the focused region.I'm afraid this is more a NVDA quirk rather than something we should fix. JAWS doesn't exhibit this behavior. VoiceOver neither. They just announce the region aria-label, as expected. I think we should report upstream this issue to NVDA. To me, when a landmark region receives focus and the region is labelled, NVDA should read only the label. I made a simplified codepen with 3 different tests, to make testing with various screen readers easier at https://codepen.io/afercia/full/PoegWdR Move focus to the first focusable instead of the region.It's important to note that the screen readers native landmarks navigation doesn't actually move focus. It moves the virtual cursor. This matters because the virtual cursor can move to any element, even the non-focusable ones. That is: if a landmark starts with a heading, or a paragraph, or a list, etc., landmarks navigation jumps to the landmark, moves the virtual cursor to the first element and announces it. This native behavior can be easily tested on the W3C landmark example pages, for instance at https://www.w3.org/WAI/ARIA/apg/example-index/landmarks/region.html It's not the only problem though. Turns out that setting and removing the tabindex attribute as proposed in this PR doesn't work well in Chrome. See the third test on the codepen. With Chrome, removing the tabindex triggers a focus loss both with NVDA and JAWS. Nothing is announced. Lastly, there's a visual thing to take into consideration. The current Overall, I wouldn't recommend to move focus to the first element. We shouldn't optimize for screen readers only. We should always try to help the broadest range of users. Screen reader users can use the native landmarks navigation: it works better and we should educate them on how to use it. Cycle through regions starting from the current region.This would be a great improvement and I'd be all for it. RIght now, region navigation starts again from the first region and that's not ideal. It makes totally sense to start from the next region. Empty tab stops.I'm not sure the empty tab stops you mention in your video are related to the negative |
alexstine
force-pushed
the
try/removing-tabindex-from-toolbars
branch
from
745c9a0 to
faefdc3
Compare
|
@ellatrix I tried a |
|
@youknowriad @kevin940726 Do you have any ideas why these tests are still failing? This is far beyond my ability to debug at this point. |
|
The playwright test is failing because we didn't update them in |
…ing-tabindex-from-toolbars
|
Thanks @kevin940726 ! I think I managed to fix this up. |
|
@kevin940726 Do you know why the main E2E tests are still failing? Playwright is resolved, but I can't tell if my changes are causing test failures or if they are just flaky test related. |
|
I think it's the same issue, we need to update the test to reflect the changes made here in: |
|
@kevin940726 Thanks! Those logs are always so confusing, I missed it. @afercia Could you please check this and approve so this fix can be merged in? Thanks. |
|
Hi @afercia I see you still have a blocking review here. Did the recent changes and discussion address your points raised above? What's remaining here? |
youknowriad
dismissed
afercia’s stale review
Removing this blocking review as it looks like these were addressed, if there's still something remaining, please let us know. Thank you
|
Sorry I missed the pings. Thanks everyone for this cool improvement. |
What?
This fixes a small bug in region navigation.
Why?
Better accessibility.
How?
Replaced:
This only works if the current element is the region itself so if the user so much as interacts with the content within a region, the user will automatically start at region index 0 or the first region. This is super bad UX. My solution brings us closer to the way it should act with most screen readers but we're still off a bit.
With:
By using closest, we'll traverse upwards in the HTML until the wrapping region is found. This gives us the index in the regions array so we can best determine previous/next index.
Testing Instructions
The following must be tested with a screen reader enabled such as NVDA. I did my testing in Firefox.
Screenshots or screencast