✨amp-consent: Added a11y focusing for consent UI #20750
Merged
Conversation
This file contains 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
zhouyx
reviewed
Feb 21, 2019
| @@ -244,6 +262,11 @@ export class ConsentUI { | |||
| this.baseInstance_.getViewport().removeFromFixedLayer(this.parent_); | |||
| toggle(dev().assertElement(this.ui_), false); | |||
| this.isVisible_ = false; | |||
|
|
|||
| if (this.elementWithFocusBeforeShowing_) { | |||
There was a problem hiding this comment.
As we agreed, if this.elementWithFocusBeforeShowing_ == null, can we focus on the first element of the page.
There was a problem hiding this comment.
Done. Used children[0] instead of firstChild due to: https://stackoverflow.com/questions/10474676/get-immediate-first-child-element
There was a problem hiding this comment.
Thank you. Do you know what happens if children[0] is not focusable? Ideally we want to find and cache the first focusable element. Don't want to block you, we can leave that as a TODO
zhouyx
approved these changes
Mar 1, 2019
noranazmy
pushed a commit
to noranazmy/amphtml
that referenced
this pull request
Mar 22, 2019
* Added a11y focusing for consent UI * Made PR Fix * Made PR Comments * Fixed linting
bramanudom
pushed a commit
to bramanudom/amphtml
that referenced
this pull request
Mar 22, 2019
* Added a11y focusing for consent UI * Made PR Fix * Made PR Comments * Fixed linting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
relates to #17257
This adds some a11y support to UIs in
amp-consent. This automatically focuses on UIs that are shown, and are not post prompt. On hide, the focus is then returned that the element was focus on before. This follows the method mentioned at this a11y blog post.Also, this updates the examples to add some missing scripts, and support the focus on the CMP iframe
NOTE: I used the
/*OK*/for the.focus()calls. As focus was a part of the Paul Irish Layout Reflow gist, but in our case we do this on a user action ormutateElement, so I assume this would be okay?Example
Please see the orange outline for where my chromevox shows focus. I know it may look a little weird, but from my experience using chrome vox, everything seems to flow well and can be accessed. 😄