Skip to content
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

[amp-video] ♿ override aria-label with alt #32963

Merged
merged 7 commits into from Mar 1, 2021

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Feb 26, 2021

amp-video accepts alt but it's not being read by screen readers.
On a video element, screen readers will read aria-label but not alt.

  • Propagates title attribute
  • Sets alt as aria-label if aria-label and title is not specified

Contributes to #32493

@processprocess processprocess requested review from gmajoulet and removed request for micajuine-ho February 26, 2021 18:47
@processprocess processprocess changed the title override aria-label with alt [amp-video] ♿ override aria-label with alt Feb 26, 2021
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you mentioned alt is not valid HTML but valid AMP. The correct HTML way for publishers would be to add either title or aria-label.

  • aria-label is already propagated to the <video> element, but title is not, we should fix this
  • propagating alt as aria-label sounds good as a fallback if no title or aria-label was already provided

extensions/amp-video/0.1/amp-video.js Outdated Show resolved Hide resolved
@amp-owners-bot
Copy link

Hey @rsimha! These files were changed:

.circleci/config.yml
build-system/pr-check/module-tests.js
build-system/pr-check/nomodule-tests.js

Hey @estherkim! These files were changed:

build-system/tasks/e2e/package-lock.json
build-system/tasks/e2e/package.json

Hey @danielrozenberg! These files were changed:

build-system/tasks/visual-diff/package-lock.json
build-system/tasks/visual-diff/package.json

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story.js

Hey @Enriqe! These files were changed:

extensions/amp-story/1.0/amp-story.js
src/amp-story-player/amp-story-player-impl.js

@processprocess processprocess merged commit 6a74422 into ampproject:master Mar 1, 2021
@processprocess processprocess deleted the alt-video branch March 1, 2021 17:22
@processprocess processprocess added this to In progress in wg-stories Sprint via automation Mar 1, 2021
@processprocess processprocess moved this from In progress to Done in wg-stories Sprint Mar 1, 2021
@mszylkowski mszylkowski removed this from Done in wg-stories Sprint Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants