Components that override createPlaceholderCallback() (and amp-youtube) need to set a reasonable alt for the image placeholder they create. For instance amp-jwplayer may add "Loading Video " + (aria-label || textContent of aria-labelledby) as the alt for the placeholder image.
extensions/name-of-component/0.1/name-of-component.js. createPlaceholderCallback() function, make sure that the placeholder image has a reasonable alt attribute for accessibility. /test/manual/ to ensure that the poster image does indeed have a reasonable alt attribute that can be read by a screenreader. gulp in the repository root directory. test/manual/name-of-component.amp.html. These tests already exists for some of the components below (e.g. amp-gfycat). localhost:8000/test/manual/name-of-component.amp.html to manually verify that your code behaves as intended. Unit tests can be found extensions/name-of-component/0.1/test/test-name-of-component.js. You can run the unit test for that file with the command gulp test --files=extensions/name-of-component/0.1/test/test-name-of-component.js. To run your test only, feel free to use it.only for testing, but remember to revert that back before you submit your PR. Once the PR merges, feel free to check off the component on this list below.
List of components that require this change:
Special case instructions for <amp-youtube>:
For <amp-youtube>, this logic needs to happen within the buildImagePlaceholder function.
addresses Issue #5508 in the description.Once approved, your changes will be merged. ⚡⚡⚡Congrats on making your first contribution to the AMP Project!⚡⚡⚡ You'll be able to see it live across the web soon!
Thanks, and we hope to see more contributions from you soon.
If you have questions ask in this issue or on your Pull Request (mentioning @cathyxz) or see the How to get help section of the Getting Started guide.
/to @erwinmombay
hi - have submitted a pr for what i think is a fix for amp-gfycat - if that's ok, happy to have the rest assigned to me
@cathyxz I've taken a look at the remaining components and have just submitted a pr for them.
I haven't included amp-apester or amp-playbuzz as they don't create a placeholder image - they create a placeholder div with animated loading divs inside. I could carry the aria-label attribute over or set a aria-label with a loading message if you think it's appropriate?
I think the same strategy as what you describe above is fine, i.e. setting aria-label since alt does not apply to divs. The purpose is to allow screen readers to have something to read (they will read alt, aria-label, etc.). Basically so long as the screen reader will say "loading video ..." with whatever defaults they provide, it is fine. If they provide no aria-label attribute, "Loading Video" is technically fine too, since that at least tells what they are currently selecting on the screen is a loading video. =)
Closed by https://github.com/ampproject/amphtml/pull/14468.
Thank you very much!
Most helpful comment
hi - have submitted a pr for what i think is a fix for amp-gfycat - if that's ok, happy to have the rest assigned to me