-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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: allow captions in devices that use old chrome to be shown #8826
Changes from 5 commits
f0089e8
5192a67
de3a4bc
b077c85
66cd7bb
3e17e4d
2686b51
7c2a64e
3f92d15
870de98
a43d069
db21cc1
d5d52d5
5efd400
4e8c5aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,6 +5,7 @@ import Component from '../component'; | |||||||||||||||||
import * as Fn from '../utils/fn.js'; | ||||||||||||||||||
import * as Dom from '../utils/dom.js'; | ||||||||||||||||||
import window from 'global/window'; | ||||||||||||||||||
import * as browser from '../utils/browser'; | ||||||||||||||||||
|
||||||||||||||||||
/** @import Player from '../player' */ | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -319,6 +320,23 @@ class TextTrackDisplay extends Component { | |||||||||||||||||
} | ||||||||||||||||||
this.updateForTrack(descriptionsTrack); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (browser.IS_SMART_TV && !window.CSS.supports('inset', '10px')) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be any browser doesn't support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, should be a feature test only, unless there's a good reason to only target TVs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the PR to support this changes on any browser, I got to move the update of the styles of textTrackDisplay to add support when user switches to full screen, can this be re-reviewed? |
||||||||||||||||||
const textTrackDisplay = this.el_; | ||||||||||||||||||
const vjsTextTrackCues = textTrackDisplay.querySelectorAll('.vjs-text-track-cue'); | ||||||||||||||||||
|
||||||||||||||||||
// vjsTextTrackCue style updates | ||||||||||||||||||
if (vjsTextTrackCues.length > 0) { | ||||||||||||||||||
vjsTextTrackCues.forEach((vjsTextTrackCue) => { | ||||||||||||||||||
const insetStyles = vjsTextTrackCue.style.inset.split(' '); | ||||||||||||||||||
wseymour15 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
|
||||||||||||||||||
// expected value is always 3 | ||||||||||||||||||
if (insetStyles.length === 3) { | ||||||||||||||||||
Object.assign(vjsTextTrackCue.style, { top: insetStyles[0], right: insetStyles[1], bottom: insetStyles[2], left: 'unset' }); | ||||||||||||||||||
} | ||||||||||||||||||
}); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
|
@@ -329,6 +347,17 @@ class TextTrackDisplay extends Component { | |||||||||||||||||
// inset-inline and inset-block are not supprted on old chrome, but these are | ||||||||||||||||||
// only likely to be used on TV devices | ||||||||||||||||||
if (!this.player_.videoHeight() || !window.CSS.supports('inset-inline: 10px')) { | ||||||||||||||||||
if (browser.IS_SMART_TV && !window.CSS.supports('inset', '10px')) { | ||||||||||||||||||
const textTrackDisplay = this.el_; | ||||||||||||||||||
const textTrackDisplayHeight = textTrackDisplay.getBoundingClientRect().height; | ||||||||||||||||||
|
||||||||||||||||||
// textrack style updates, this styles are required to be inline | ||||||||||||||||||
textTrackDisplay.style.position = 'relative'; | ||||||||||||||||||
textTrackDisplay.style.height = textTrackDisplayHeight + 'px'; | ||||||||||||||||||
textTrackDisplay.style.top = 'unset'; | ||||||||||||||||||
textTrackDisplay.style.bottom = '0px'; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
this updates to use tryUpdateStyle for consistency with the rest of the code, but maybe that isn't really neccessary as the comment for tryUpdateStyle metions IE8 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added this change in the last commit, can this be re-reviewed? |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the
div
here represent? Do we want this CSS set always? Or maybe we could add a class name to thisdiv
in the scenario we want so we know it is SMART TV related..I just want to be sure this does not have any unexpected side effects..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a div that is dynamically generated without a classname it is the parent of the vjs-text-track-cue, all of this styles are just backup styles, are all overwritten by inline styles except in old chrome devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I updated the PR to add those styles only when required, can this be re-reviewed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I would maybe make that name more descriptive "vjs-text-track-display-inset" or something like that. Otherwise, nice work!