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

Set the 'lang' attribute on text track display elements, if the language of the track is known #7493

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

OwenEdwards
Copy link
Member

Description

Fixes #7487

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 OwenEdwards added the a11y This item might affect the accessibility of the player label Oct 30, 2021
@codecov
Copy link

codecov bot commented Oct 30, 2021

Codecov Report

Merging #7493 (31ec5de) into main (6faad26) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head 31ec5de differs from pull request most recent head dede411. Consider uploading reports for the commit dede411 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7493      +/-   ##
==========================================
- Coverage   79.72%   79.72%   -0.01%     
==========================================
  Files         116      116              
  Lines        7296     7298       +2     
  Branches     1754     1755       +1     
==========================================
+ Hits         5817     5818       +1     
- Misses       1479     1480       +1     
Impacted Files Coverage Δ
src/js/tracks/text-track-display.js 81.43% <50.00%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6faad26...dede411. Read the comment docs.

@OwenEdwards
Copy link
Member Author

I wish I knew how to add a test for this change, in part so that it doesn't make the code coverage (fractionally) worse, but I don't! Does anyone want to add a test to this?

@gkatsev
Copy link
Member

gkatsev commented Nov 1, 2021

@OwenEdwards got some tests for this here OwenEdwards#3

@mister-ben
Copy link
Contributor

This also fixes this problem I didn't realise we have that Chinese glyphs could be used incorrectly with Japanese subs, if the player language isn't Japanese.

https://heistak.github.io/your-code-displays-japanese-wrong/

* add some tests

* update test naming
@gkatsev
Copy link
Member

gkatsev commented Nov 2, 2021

lol, not sure why CI doesn't want to re-run stuff. Maybe I should've made the PR from my fork instead.

@gkatsev gkatsev merged commit f326cf3 into videojs:main Nov 2, 2021
@gkatsev
Copy link
Member

gkatsev commented Nov 2, 2021

The code coverage went a tiny bit up.

@OwenEdwards OwenEdwards deleted the fix/lang-of-text-track-display branch November 3, 2021 01:51
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y This item might affect the accessibility of the player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The language of text track's cues is not marked programmatically when they are displayed
3 participants