benetech / MathShare

MIT License
9 stars 3 forks source link

Home -> NewProblemsForm: Incorrect Modal Dismissal Behavior #814

Open jscholes opened 4 years ago

jscholes commented 4 years ago

Similar to #765. When the "Add New Problem" modal is closed, focus should return to the triggering "Add New Problem" button.

Aha! Link: https://diagramlabs.aha.io/features/B-107

rupeshparab commented 4 years ago

I have fixed the focus issue

jscholes commented 4 years ago

@rupeshparab - focus does eventually return to the "New Problem Set" button, but it seems to encounter other elements along the way.

rupeshparab commented 4 years ago

I wasn't able to replicate the focus going onto Intercom, but I did notice when modal was closed by anything other than the cancel button, the focus wasn't working. I have changed the focus on modal dismissal mechanism to include a limited retrial logic to retry a few times in a short span until the target can be focused.

Please check it out and let me know if you are still facing the 2 mentioned issues.

jscholes commented 4 years ago

@rupeshparab - still not ideal. When I dismiss the modal, my focus often moves to the first problem in the list of existing ones (if there are any). Maybe the modal is being dismissed before the focus change explicitly occurs, meaning that the browser is trying to work out a focus point first? Could we unhide the parent content, set keyboard focus, then dismiss the modal last of all?

rupeshparab commented 4 years ago

I have changed this to start trying to focus the required element before the modal gets dismissed

jscholes commented 4 years ago

@rupeshparab - when I hit Escape to close the modal, it doesn't seem to work at all now. More often than not my focus just lands on a random point, be that the bottom of the page, the "Edit Problem Set Title" or somewhere else.

rupeshparab commented 4 years ago

I am noticing some weird behaviour when I try dismissing modal with VoiceOver enabled. Without VoiceOver I can dismiss the modal if I press Esc once (when the focus is not on MathEditor as Esc is a shortcut key inside the editor), but while using VoiceOver, I need to press Esc twice or thrice in order to dismiss the modal.

I am investigating if the aria modal library is detecting the usage of screen reader and causing some changes in focus which might be causing this.

rupeshparab commented 4 years ago

@jscholes I tried dialog dismissal using VoiceOver on other sites, it seems it is standard to double tap Escape for VoiceOver. On Dismissing the modal, VoiceOver always lands on the 'Add new Problem' button. But on NVDA, it seems the focus sometimes moves to Intercom before moving back to 'Add new Problem' button.

I am checking if Intercom can be configured to avoid bringing itself in focus, but on pressing Escape twice I am able to dismiss the modal on VoiceOver and NVDA both.

sinabahram commented 4 years ago

Can’t speak to VO’s default behavior on other sites, but a single escape for NVDA or Jaws is what’s required to make sure this passes. If double escape is required, then there’s something that needs to be fixed.

rupeshparab commented 4 years ago

@sinabahram It seems that the first Escape is to switch from focus to browse mode https://github.com/nvaccess/nvda/issues/4428#issuecomment-155326950

I think this behaviour is occurring on our modal as the library (https://github.com/davidtheclark/react-aria-modal) which we are using traps the focus in the modal.

sinabahram commented 4 years ago

I suspected that was the case, which is not great. I’m going to let James comment on this, but not sure why the modal is not being treated as a document role.

Do we really need a library for modals? It sounds like it’s offering more problems than solutions?

jscholes commented 4 years ago

The referenced NVDA ticket hasn‘t been updated since 2014. Switching to focus mode is no longer the default behaviour in modals.

jscholes commented 4 years ago

I will look into this further. Just to confirm, modals should trap focus, but that is not the same as NVDA focus mode. Complicating this specific issue is the fact that when the dialogue opens, the editor incorrectly receives focus. that is a separate problem, but interferes with the debugging of this one.

johnhbenetech commented 4 years ago

@alexrcabral please coordinate a11y QA on this one

alexrcabral commented 4 years ago

@Sanjog1605 I'd like for you to take a look at this issue. The behavior has changed over time, so I would like to understand the current status of dismissal on modals (for instance, the current login modal), and if/how it differs from expected behavior.

(I believe this is also updated from #1266 on handling dismissal when the originating element is no longer present)

Sanjog1605 commented 4 years ago

In all the modals, namely, "Share my Answer", "Share Problem Set", "Log In", and "Personalization" modals, I found the same behavior and that is, all of them do close when "Esc" key is pressed once. This testing was done using NVDA with Google Chrome and Mozilla Firefox. Moreover, I also tested the functioning of the save and close buttons on "Personalization modal and they are also working fine.

Please kindly elaborate on your sentence "(I believe this is also updated from #1266 on handling dismissal when the originating element is no longer present)". I did not understand the meaning.

alexrcabral commented 4 years ago

@Sanjog1605 Is the focus also behaving correctly after dismissal?

I advise you to read through the prior comments on this issue on the problems that James and Sina were historically encountering. The problem would not necessarily be on how the modal is dismissed (which is an important piece of it), but where the focus ends up post-dismissal.

For clarification on the sentence "I believe this is also updated from #1266 on handling dismissal when the originating element is no longer present": it's referencing the new issue which updated the modals to have one common, accessible component (a piece of the code which is reused, like a template). With this update, it should be more consistent in where focus goes after the modal is gone, if the button you pressed to access the modal is no longer there. For example, if you press "Log In", and you successfully log in using the modal, where should the focus go when you close that modal, since the button to Log In should no longer be present.

Sanjog1605 commented 4 years ago

Following are the observations that I have made regarding the three modals namely, “Share My Answer”, “Share Problem Set”, and “Login”:

Login Modal

Share Problem Set Modal

Share My Answers Modal

@alexrcabral sir, please kindly let me know if I have missed anything.

alexrcabral commented 4 years ago

Thanks @Sanjog1605, please be sure to also include a summary of suggested changes. This can help clarify between background context and observations, and which specific items need to be fixed.