amp-consent a11y improvement#26456
Conversation
d65985c to
120af9b
Compare
caroqliu
left a comment
There was a problem hiding this comment.
Thanks for working on this Mica!!
|
|
||
| /** @private {string} */ | ||
| this.consentTitle_ = | ||
| (config['captions'] && config['captions']['consentTitle']) || |
There was a problem hiding this comment.
nit: Convert the first argument to a boolean here and below: !!config['captions']
b79e70e to
2f52083
Compare
| titleDiv.textContent = this.consentTitle_; | ||
| button.textContent = this.buttonTitle_; | ||
| button.onclick = () => { | ||
| this.ui_ = dev().assertElement(this.ui_); |
There was a problem hiding this comment.
For type checking... is there a better way to do this?
There was a problem hiding this comment.
tryFocus(dev().assertElement(this.ui_));
2f52083 to
d895cab
Compare
|
|
||
| /** @private {string} */ | ||
| this.buttonTitle_ = | ||
| (config['captions'] && config['captions']['buttonTitle']) || |
There was a problem hiding this comment.
nit: Would be great to define the default message in a const string.
There was a problem hiding this comment.
buttonTitle is a bit confusing to me, it's an invisible button for screen reader only. captions kind of convey that message, but I am not sure if developer understand what to put there.
I don't have a good name in mind though. any idea?
There was a problem hiding this comment.
Changed to consentPromptCaption_ and buttonActionCaption_, I think this is more clear.
test/manual/amp-consent/legacy_config_support/amp-consent-cmp-screen-reader.html
Outdated
Show resolved
Hide resolved
b3c3eb2 to
060dccf
Compare
caroqliu
left a comment
There was a problem hiding this comment.
Thanks for making these changes. It looks like a number of these unit tests are testing user interaction more than individual methods. I recommend introducing end-to-end tests for some of these cases instead, though you can do that in a follow-up PR.
| titleDiv.textContent = this.consentTitle_; | ||
| button.textContent = this.buttonTitle_; | ||
| button.onclick = () => { | ||
| this.ui_ = dev().assertElement(this.ui_); |
There was a problem hiding this comment.
tryFocus(dev().assertElement(this.ui_));
Great idea, I'll add it as a task :) |
zhouyx
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the improvement 🎉
Task 2 of #26432
When the consent iframe loads (CMP and publisher), append an 'invisible' div that uses
role=alertdialogattribute to trigger Screen Reader to read aconsentTitle&buttonTitle(optionally provided by pubs). Clicking on button will give focus on the button that will transfer focus to the iframe.On subsequent viewings of the consent prompt (i.e. user hits manage my consent), do not show this div again as the user has already interacted.