-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[A11Y] amp-image-lightbox set aria attributes #5579
Conversation
@@ -92,6 +92,14 @@ export class ImageViewer { | |||
/** @private {?../../../src/srcset.Srcset} */ | |||
this.srcset_ = null; | |||
|
|||
/** @private {!Object} */ | |||
this.captions_ = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ariaAttributes_
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
@@ -223,10 +235,22 @@ export class ImageViewer { | |||
this.sourceWidth_ = sourceElement./*OK*/offsetWidth; | |||
this.sourceHeight_ = sourceElement./*OK*/offsetHeight; | |||
this.srcset_ = srcsetFromElement(sourceElement); | |||
Object.keys(this.captions_).forEach(key => { | |||
if (key == 'aria-describedby') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would turn aria-describedby
around and have AmpImageLightbox
handle it instead of ImageViewer
since it is not really a cloned attribute like aria-label
it.
Essentially, what I am recommending is to have
AmpImageLightbox
call getImage()
on the ImageViewer
and set the aria-describedby
on init_
to the id it has and remove it on reset_
(so ImageViewer
has no idea of the id of the caption which is part of the AmpImageLightbox
and captionElement
can go back to being private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
/** @private {?Element} */ | ||
this.captionElement_ = null; | ||
/** {?Element} */ | ||
this.captionElement = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comment, we should keep this private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
fix #5501 and #5502