fix: allow captions in devices that use old chrome to be shown #8826

CarlosVillasenor · 2024-08-09T16:17:12Z

Description

In the specific case of using an old chrome version this minor changes will allow the captions to be shown.

Specific Changes proposed

Small fix when using videojs in an old browser that does not support the css 'inset' property.

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)
- Has no DOM changes which impact accessiblilty or trigger warnings (e.g. Chrome issues tab)
- Has no changes to JSDoc which cause npm run docs:api to error
Reviewed by Two Core Contributors

codecov · 2024-08-09T16:18:39Z

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.46%. Comparing base (57c27f8) to head (4e8c5aa).
Report is 8 commits behind head on main.

Files	Patch %	Lines
src/js/tracks/text-track-display.js	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8826      +/-   ##
==========================================
+ Coverage   83.12%   84.46%   +1.33%     
==========================================
  Files         120      120              
  Lines        8052     9492    +1440     
  Branches     1931     2477     +546     
==========================================
+ Hits         6693     8017    +1324     
- Misses       1359     1475     +116

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mister-ben · 2024-08-13T08:54:37Z

Would it work to put the changes to .vjs-text-track-display in updateDisplayOverlay() in this block before the return? Just to keep the alternate styles for the same element close to each other for maintainability.

video.js/src/js/tracks/text-track-display.js

Lines 343 to 345 in 5192a67

    
           if (!this.player_.videoHeight() || !window.CSS.supports('inset-inline: 10px')) { 
        
             return; 
        
           }

test/unit/tracks/text-track-display.test.js

src/js/tracks/text-track-display.js

Co-authored-by: Walter Seymour <[email protected]>

…stainability

mister-ben · 2024-08-14T15:14:52Z

src/js/tracks/text-track-display.js

@@ -319,6 +320,23 @@ class TextTrackDisplay extends Component {
      }
      this.updateForTrack(descriptionsTrack);
    }
+
+    if (browser.IS_SMART_TV && !window.CSS.supports('inset', '10px')) {


Should this be any browser doesn't support inset, and not just smart TVs? Smart TVs are the only real reason to be using such old browser versions, but it's not inconceivable that people might test things destined for a smart TV on an old browser on desktop/browserstack etc.

Yeah, should be a feature test only, unless there's a good reason to only target TVs?
Below should be updated to match as well

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?

gkatsev · 2024-08-14T16:39:54Z

src/js/tracks/text-track-display.js

+        textTrackDisplay.style.position = 'relative';
+        textTrackDisplay.style.height = textTrackDisplayHeight + 'px';
+        textTrackDisplay.style.top = 'unset';
+        textTrackDisplay.style.bottom = '0px';


Suggested change

textTrackDisplay.style.position = 'relative';

textTrackDisplay.style.height = textTrackDisplayHeight + 'px';

textTrackDisplay.style.top = 'unset';

textTrackDisplay.style.bottom = '0px';

tryUpdateStyle(textTrackDisplay, 'position', 'relative');

tryUpdateStyle(textTrackDisplay, 'height', textTrackDisplayHeight + 'px');

tryUpdateStyle(textTrackDisplay, 'top', 'unset');

tryUpdateStyle(textTrackDisplay, 'bottom', '0px');

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 🤔

added this change in the last commit, can this be re-reviewed?

…th the rest of the code

src/js/tracks/text-track-display.js

wseymour15 · 2024-08-21T19:15:19Z

src/css/components/_text-track.scss

@@ -46,3 +46,11 @@ video::-webkit-media-text-track-display {
  text-align: center !important;
  width: 80% !important;
 }
+
+.video-js .vjs-text-track-display > div {


What does the div here represent? Do we want this CSS set always? Or maybe we could add a class name to this div 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..

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.

But I updated the PR to add those styles only when required, can this be re-reviewed?

Cool, I would maybe make that name more descriptive "vjs-text-track-display-inset" or something like that. Otherwise, nice work!

src/css/components/_text-track.scss

gkatsev · 2024-08-26T15:24:25Z

src/js/tracks/text-track-display.js

+      // Add custom class to textTrackDisplay div child for not inset support styles
+      textTrackDisplay.firstChild.classList.add('vjs-text-track-display-inset');


an alternative to adding this class is using css @supports. Then, this line won't be necessary and it could all be done in css.

Suggested change

// Add custom class to textTrackDisplay div child for not inset support styles

textTrackDisplay.firstChild.classList.add('vjs-text-track-display-inset');

updated this in the last commit, can this be re-reviewed?

src/js/tracks/text-track-display.js

Co-authored-by: Gary Katsevman <[email protected]>

fix: allow captions in devices that use old chrome to be shown

f0089e8

fix: add unit test for first case to update caption styles

5192a67

CarlosVillasenor requested review from dzianis-dashkevich and wseymour15 August 13, 2024 02:45

CarlosVillasenor marked this pull request as ready for review August 13, 2024 03:02

wseymour15 reviewed Aug 13, 2024

View reviewed changes

test/unit/tracks/text-track-display.test.js Outdated Show resolved Hide resolved

src/js/tracks/text-track-display.js Outdated Show resolved Hide resolved

src/js/tracks/text-track-display.js Outdated Show resolved Hide resolved

CarlosVillasenor and others added 3 commits August 13, 2024 10:50

Update test/unit/tracks/text-track-display.test.js

de3a4bc

Co-authored-by: Walter Seymour <[email protected]>

fix: use logic to update styles with only one condition for better su…

b077c85

…stainability

fix: add missing unit test

66cd7bb

CarlosVillasenor requested a review from wseymour15 August 14, 2024 00:17

mister-ben reviewed Aug 14, 2024

View reviewed changes

gkatsev reviewed Aug 14, 2024

View reviewed changes

CarlosVillasenor added 2 commits August 14, 2024 12:04

fix: update style changes to be used in browsers

3e17e4d

fix: use tryUpdateStyle to update textTrackDisplay for consistency wi…

2686b51

…th the rest of the code

CarlosVillasenor requested review from gkatsev and mister-ben August 14, 2024 18:35

fix: update condition for update textTrackDisplay styles

7c2a64e

wseymour15 reviewed Aug 14, 2024

View reviewed changes

src/js/tracks/text-track-display.js Outdated Show resolved Hide resolved

fix: verify if inline inset styles are present

3f92d15

CarlosVillasenor requested a review from wseymour15 August 15, 2024 16:58

fix: add condition for position of textTrackDisplay in STV devices

870de98

wseymour15 reviewed Aug 21, 2024

View reviewed changes

CarlosVillasenor added 3 commits August 21, 2024 14:35

fix: use custom class to add backup styles

a43d069

fix: update names for custom class for inset support in old browsers

db21cc1

fix: update assertion name in text-track-display test

d5d52d5

wseymour15 approved these changes Aug 22, 2024

View reviewed changes

gkatsev reviewed Aug 26, 2024

View reviewed changes

Update src/css/components/_text-track.scss

5efd400

Co-authored-by: Gary Katsevman <[email protected]>

fix: use css condition insted of js condition to add custom styles

4e8c5aa

gkatsev approved these changes Aug 26, 2024

View reviewed changes

CarlosVillasenor requested a review from gkatsev August 26, 2024 19:16

CarlosVillasenor merged commit 3e9e4e8 into main Aug 26, 2024
14 checks passed

CarlosVillasenor deleted the fix/captions-old-chrome branch August 26, 2024 19:32

This was referenced Sep 25, 2024

[Snyk] Upgrade video.js from 8.6.0 to 8.17.4 kerasus/soalaa#1729

Open

[Snyk] Upgrade video.js from 8.6.0 to 8.18.0 kerasus/soalaa#1735

Open

nerdy-tech-com-gitub mentioned this pull request Oct 26, 2024

[Snyk] Upgrade video.js from 8.10.0 to 8.18.1 nerdy-tech-com-gitub/filebrowser#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow captions in devices that use old chrome to be shown #8826

fix: allow captions in devices that use old chrome to be shown #8826

CarlosVillasenor commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

mister-ben commented Aug 13, 2024

mister-ben Aug 14, 2024

gkatsev Aug 14, 2024

CarlosVillasenor Aug 14, 2024

gkatsev Aug 14, 2024

CarlosVillasenor Aug 14, 2024 •

edited

Loading

wseymour15 Aug 21, 2024

CarlosVillasenor Aug 21, 2024

CarlosVillasenor Aug 21, 2024

wseymour15 Aug 21, 2024

gkatsev Aug 26, 2024

CarlosVillasenor Aug 26, 2024

		// Add custom class to textTrackDisplay div child for not inset support styles
		textTrackDisplay.firstChild.classList.add('vjs-text-track-display-inset');

fix: allow captions in devices that use old chrome to be shown #8826

fix: allow captions in devices that use old chrome to be shown #8826

Conversation

CarlosVillasenor commented Aug 9, 2024 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

codecov bot commented Aug 9, 2024 • edited Loading

Codecov Report

mister-ben commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosVillasenor Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarlosVillasenor commented Aug 9, 2024 •

edited

Loading

codecov bot commented Aug 9, 2024 •

edited

Loading

CarlosVillasenor Aug 14, 2024 •

edited

Loading