benetech / MathShare

MIT License
9 stars 3 forks source link

Create common modal component and replace non deprecated modals #1266

Open rupeshparab opened 4 years ago

rupeshparab commented 4 years ago

Create a common component to use initialFocus attribute instead of using JS to focus the heading. Replace all non-deprecated modals to use this common component.

johnhbenetech commented 4 years ago

@rupeshparab im breaking out the deprecation removal and common component into two separate tasks.

This task will handle the creation of a common modal component, including:

johnhbenetech commented 4 years ago

@rupeshparab can you make sure #1298 is addressed as a result of this work

johnhbenetech commented 4 years ago

I'm gonna move this as it is primarily a code change

alexrcabral commented 4 years ago

I have one question on how the common modal component handles headings and focus.

@Sanjog1605 On the the "Share Problem Set" "Share My Answers" modals it's particularly noticeable that if you navigate backwards on headings, it surfaces all of the headings in the grayed out page. Is this expected behavior?

Sanjog1605 commented 4 years ago

Hello @alexrcabral Sir,

Under normal circumstances, screen reader is not supposed to read anything apart from the modal, and its focus should be inside the modal for the time it is open. When I tested "Share my Answer", "Share Problem", and "Login" modals, I did not encounter the behavior which you did. I used Mozilla Firefox and Google Chrome with NVDA and JAWS. I also checked it when I read the comment but the result is same.

alexrcabral commented 4 years ago

Thanks @Sanjog1605 -- it seems to be a VoiceOver specific bug. I encountered this on both Chrome and Safari on VoiceOver (macOS 10.15.5). I checked on NVDA, and it wasn't an issue there.

I took a screen recording (including screen reader audio, and spoken descriptions of context): https://www.loom.com/share/4f59462be68249828362c67a6eeb66e9

Full list of Headings using the VoiceOver rotor on the Share Problem Set modal:

Sanjog1605 commented 4 years ago

Hello, @alexrcabral sir, the exact same behavior was with NVDA in Firefox and Chrome, but it is now fixed. It was with the same modal that you refered in the video, that is, the "Share Problem Set" modal. It is a good idea to share the screen recording as well. I will also do it to explain more complicated issues. 😀

johnhbenetech commented 4 years ago

@alexrcabral can you clarify what the next steps are?

alexrcabral commented 3 years ago

Next steps are remediating the issue so the behavior is correct on Safari + VoiceOver.

Expected behavior: The screen reader should not read anything apart from the modal, and the focus should remain inside the modal for as the time it is open.

Since this is explicitly addressing accessibility of modals, this should be correct and largely consistent across the top five screen reader / browser combinations:

rupeshparab commented 3 years ago

@alexrcabral I did some research on this, it seems there are 2 different focus - one is of Voiceover and the second is the actual Keyboard focus. The keyboard focus is getting trapped within the modal as the modal has a focus trap to improve accessibility. This trap cannot be used with VoiceOver, which causes the VoiceOver focus to go out of the modal

To fix this, there are 2 options: 1) We remove the focus trap and allow the keyboard to go outside the modal 2) Newer versions (from High Sierra) support an option to Sync keyboard and voiceover focus, it seems to reduce this issue of focus going outside the modal. It needs to be enabled here: image