Only add dialog role to navigation when modal is open.#37434
Merged
Conversation
|
Size Change: +25 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
MarcoZehe
reviewed
Dec 16, 2021
| { ! isOpen && ( | ||
| <Button | ||
| aria-haspopup="true" | ||
| aria-expanded={ isOpen } |
Contributor
There was a problem hiding this comment.
Thank you for removing the aria-expandedattribute here, since changing the button label is enough to convey the status the modal is currently in and what will happen when the button is activated. 👍
jasmussen
approved these changes
Dec 16, 2021
jasmussen
left a comment
Contributor
There was a problem hiding this comment.
This is working very well for me, thanks so much for working on this. It's less code, it's clearer, and as best I can tell it's working as intended: when collapsed, it's not a dialog:
When the modal is opened, it's a dialog:
Giving a green check here based on my best ability, but I would appreciate if we could boost the confidence with testing by folks more skilled in the area. Thank you!
talldan
reviewed
Dec 16, 2021
| <> | ||
| { ! isOpen && ( | ||
| <Button | ||
| aria-haspopup="true" |
Contributor
There was a problem hiding this comment.
It's definitely a dialog, so I don't know whether it should be aria-haspop="dialog". It is a menu in a dialog, so maybe that makes it ok to use true. 🤔
Contributor
Could you add testing instructions to the PR? I'm not really sure what I'm supposed to be testing. The issue isn't very clear, either as it's very long with multiple sets of instructions. On I'm not sure this completely fixes #36600. One of the bits of feedback there was that it's hard to close the dialog, and I found this is still the case within the editor. Once I've navigated to the first block it's hard to get back. It seems to be because you need to use the horizontal arrow key to get to the button. I haven't really tested this overlay menu much before, but it seems like there are a couple of issues still outstanding after this fix:
|
talldan
approved these changes
Dec 16, 2021
talldan
left a comment
Contributor
There was a problem hiding this comment.
Ok, I was able to figure out how to reproduce eventually and this does fix it. Nice one!
It might be nice to just return children when the menu isn't active, but I guess we can't guarantee the breakpoint a theme will use in JS.
Contributor
|
I've unlinked the PR from the issue because I don't think we should close the issue until we address the other problems mentioned. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Addresses part of #36600; alternative to #36617.
This PR adds the
dialogrole andaria-modal="true"to the navigation only when the modal is open. The attributes are added dynamically in both editor and front-end, so we don't need separate sets of markup for each state.How has this been tested?
Tested with VoiceOver in Safari; if navigation is not in responsive mode no dialog role is present.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).