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

fix: Set alt attr on poster img #8043

Merged
merged 4 commits into from
Jan 24, 2023
Merged

Conversation

mister-ben
Copy link
Contributor

@mister-ben mister-ben commented Dec 12, 2022

Description

As the poster now uses an img element, it should have an alt attribute.

Specific Changes proposed

Sets a null alt attribute on the poster image. This conveys to screen readers (and accessibility validators) that the image is decorative and does not need descriptive text.

To do in a future update (needs discussion)

  • Allow an arbitrary string to be set, so that an image can have a meaningful alt text where appropriate. Requires update to poster/player options.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@OwenEdwards
Copy link
Member

The <img> element should definitely have an alt attribute, but as far as whether it should have text or just a "null alt" (alt=""), that's a little complicated.

There was a lot of discussion at W3C about whether the <video> element should accept an alt attribute for the poster image, but the decision was that the poster image is an alternate for the video itself, so the title of the video (either as a title attribute or an aria-label/aria-labelledby on the video element) was effectively the text alternative for the image. That was under the specific expectation that the poster image is always a still from the video and not another image. In that case, I personally think the term "poster" is misleading because many people think of the movie theater poster when they hear that term related to a video. A movie theater poster is rarely just a still from the video and often includes text which isn't available to non-sighted users anywhere else. For example (from https://en.wikipedia.org/wiki/Gone_with_the_Wind_(film)):

The theatrical pre-release poster for the film Gone With the Wind, from Wikipedia

So, I think the default should be to make it a null alt, but there should be an option to change that to a description of the poster image.

@mister-ben
Copy link
Contributor Author

Thanks @OwenEdwards, very useful feedback. Let's keep this simple as a quick fix with the null alt and figure out how best to allow a more meaningful value to be set as a feature.

src/js/poster-image.js Outdated Show resolved Hide resolved
@mister-ben
Copy link
Contributor Author

mister-ben commented Dec 12, 2022

The codecov failures are codecov hitting rate limits. codecov/feedback#126. Apparently thr "fix" to add a codecov token to gh secrets doesn't apply to prs from forks, like this.

Well it runs successfully after hitting retry a few times (having to do that won't help their overall rate limiting issues) but the result is nonsense again, this PR definitely doesn't decrease coverage.

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #8043 (c579a37) into main (e21d295) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #8043   +/-   ##
=======================================
  Coverage   81.91%   81.91%           
=======================================
  Files         110      110           
  Lines        7342     7342           
  Branches     1772     1772           
=======================================
  Hits         6014     6014           
  Misses       1328     1328           
Impacted Files Coverage Δ
src/js/poster-image.js 75.60% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mister-ben mister-ben added the patch This PR can be added to a patch release. label Dec 16, 2022
@mister-ben mister-ben merged commit 3accbc7 into videojs:main Jan 24, 2023
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
* fix: Set alt attr on poster img

* use null alt instead

* remove debug text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants