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: don't let the player be translated except captions #7474

Merged
merged 1 commit into from
Nov 9, 2021
Merged

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Oct 14, 2021

This is another follow-up to #6699.

Potentially, it means we could get rid of #6977

This is another follow-up to #6699.

Potentially, it means we could get rid of #6977
@gkatsev
Copy link
Member Author

gkatsev commented Oct 14, 2021

@OwenEdwards do you have any thoughts on this? Basically, we have issues if translations mess with our elements, but keeping captions translatable seems worthwhile.

@codecov
Copy link

codecov bot commented Oct 14, 2021

Codecov Report

Merging #7474 (5afbb47) into main (4d1fb4a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7474   +/-   ##
=======================================
  Coverage   79.72%   79.73%           
=======================================
  Files         116      116           
  Lines        7296     7297    +1     
  Branches     1754     1754           
=======================================
+ Hits         5817     5818    +1     
  Misses       1479     1479           
Impacted Files Coverage Δ
src/js/tracks/text-track-display.js 81.81% <ø> (ø)
src/js/player.js 87.24% <100.00%> (+<0.01%) ⬆️

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 4d1fb4a...5afbb47. Read the comment docs.

@OwenEdwards
Copy link
Member

@gkatsev honestly, I don't know anything about the translate attribute. This is separate from video.js' lang/ translation JSON files, right?

And, I'm confused about the captions being translated as opposed to a separate subtitles track being present with the video?!

@gkatsev
Copy link
Member Author

gkatsev commented Oct 27, 2021

@OwenEdwards so, the translate attribute is to stop or allow browsers like Google Chrome from modifying and translating elements. For example, if you have a page and a player in Spanish, but your locale is in English, Chrome might show a "do you want to translate this page" dialog. If you click yet, it'll translate various text to your locale. Mostly it's fine, but it definitely messes up with some of our controls, particularly the progress bar.
The idea here is to tell browsers not to translate the player itself but that if captions in Spanish on that Spanish page, it could translate the captions into English, under the assumption that you're translating the page and that official captions don't exist in that language.

I've tried it out locally, and it does work not to translate the player itself but only the captions, though, I didn't test how well it worked during playback (if new cues will get translated or not)

Hope that makes sense.

@OwenEdwards
Copy link
Member

@gkatsev I'm trying to understand how this would work in practice, so these are just my thoughts, rather than me saying what I think should be done:

it seems like it could get really complicated, because, in the example you give of a Spanish page with Spanish captions, there might be an English captions track (strictly speaking, a subtitles track), which would make more sense for the user to select than automatically translating the Spanish captions.

Also, come to think of it, if there is an English text track for a video on a page that's in Spanish, does video.js mark the lang attribute of the text track display area to indicate that that text is in English rather than Spanish? If not, then that's technically an accessibility violation of Success Criterion 3.1.2 Language of Parts, and that's something that should be fixed. It also may also mess up the automatic translation if video.js isn't marking the language of the text track correctly (in fact, a text track could be in any language, not just the language of the page or the language that the user wants the page to be translated into, right? The srclang attribute indicates the track's text language).

And then, there's also the issue of what language each text track's label is in; that's displayed in the video.js menu, and wouldn't get translated with this translate attribute change, right?

@gkatsev
Copy link
Member Author

gkatsev commented Nov 1, 2021

@OwenEdwards yeah, I was thinking the scenario of only Spanish captions without English captions. Theoretically, English->English in that case would be a no-op.
#7493 makes sense to have.

The items in the text track menu won't get translated because they're covered by the main translate=no.

@OwenEdwards
Copy link
Member

@gkatsev you wrote:

The items in the text track menu won't get translated because they're covered by the main translate=no.

Wouldn't it make sense to put translate="yes" on those items, and on the content of the Captions Settings dialog?

Broadly, I understand the #6699 identifies a specific case where using Google Translate to translate a page that includes video.js causes a problem, and applying translate="no" to the whole player is a quick fix. Longer-term, could we be more targeted about what gets translated and what doesn't?

@gkatsev
Copy link
Member Author

gkatsev commented Nov 1, 2021

Wouldn't it make sense to put translate="yes" on those items, and on the content of the Captions Settings dialog?

Maybe, if we can get just the text isolated enough. Since, we wouldn't want to introduce issues where translate replaces the elements and using the menu doesn't work anymore.

Yup, applying translate=no to the entire player is definitely the quicker fix, and we can definitely enable more elements to be available as translate=yes, though, I think time is better spent on making our translation support better and more usable so that people don't need to go through this translation thing.

If there's no objections with proceeding with this PR, I'd like to merge it in.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 1, 2021

Also, I guess the alternative is to only do translate=no on the player right now and then audit the player to see where we can/should do translate=yes

@OwenEdwards
Copy link
Member

LGTM.

@gkatsev gkatsev merged commit bcd80f9 into main Nov 9, 2021
@gkatsev gkatsev deleted the no-translate branch November 9, 2021 15:17
Copy link

@Zyhazeeb Zyhazeeb left a comment

Choose a reason for hiding this comment

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

Hi

@Zyhazeeb
Copy link

Hi friends

edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
This is another follow-up to videojs#6699.

Potentially, it means we could get rid of videojs#6977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants