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: allow captions in devices that use old chrome to be shown #8826

Merged
merged 15 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/css/components/_text-track.scss
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,10 @@ video::-webkit-media-text-track-display {
text-align: center !important;
width: 80% !important;
}

.video-js .vjs-text-track-display > div {
Copy link
Contributor

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 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..

Copy link
Contributor Author

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.

Screenshot 2024-08-21 at 13 35 50

Copy link
Contributor Author

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?

Copy link
Contributor

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!

top: 0;
right: 0;
bottom: 0;
left: 0;
}
25 changes: 25 additions & 0 deletions src/js/tracks/text-track-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
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' */

Expand Down Expand Up @@ -319,6 +320,17 @@
}
this.updateForTrack(descriptionsTrack);
}

if (browser.IS_SMART_TV && !window.CSS.supports('inset', '10px')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?
Below should be updated to match as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 textTrack = window.document.querySelector('.vjs-text-track-display');
wseymour15 marked this conversation as resolved.
Show resolved Hide resolved
const textTrackHeight = textTrack.getBoundingClientRect().height;

// This styles are required to be inline
textTrack.style.position = 'relative';
textTrack.style.height = textTrackHeight + 'px';
textTrack.style.top = 'unset';
textTrack.style.bottom = '0px';
}
}

/**
Expand Down Expand Up @@ -484,6 +496,19 @@
this.updateDisplayState(track);
}
}

if (browser.IS_SMART_TV && !window.CSS.supports('inset', '10px')) {
wseymour15 marked this conversation as resolved.
Show resolved Hide resolved
const document_ = window.document;
const vjsTextTrackCue = document_.querySelector('.vjs-text-track-cue');

if (vjsTextTrackCue) {
const insetStyles = vjsTextTrackCue.style.inset.split(' ');

if (insetStyles.length === 3) {
Object.assign(vjsTextTrackCue.style, { top: insetStyles[0], right: insetStyles[1], bottom: insetStyles[2], left: 'unset' });

Check warning on line 508 in src/js/tracks/text-track-display.js

View check run for this annotation

Codecov / codecov/patch

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

Added line #L508 was not covered by tests
}
}
}
}

}
Expand Down
28 changes: 28 additions & 0 deletions test/unit/tracks/text-track-display.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -500,4 +500,32 @@ if (!Html5.supportsNativeTextTracks()) {

player.dispose();
});

QUnit.test('should use relative position for vjs-text-track-display element if browser is does not support inset property', function(assert) {
CarlosVillasenor marked this conversation as resolved.
Show resolved Hide resolved
// Set conditions for the use of the style modifications
window.CSS.supports = () => false;
browser.IS_SMART_TV = () => true;

const player = TestHelpers.makePlayer();
const track1 = {
kind: 'captions',
label: 'English',
language: 'en',
src: 'en.vtt',
default: true
};

// Add the text track
player.addRemoteTextTrack(track1, true);

// Make sure the ready handler runs
this.clock.tick(1);

const textTrack = window.document.querySelector('.vjs-text-track-display');

assert.ok(textTrack.style.position === 'relative', 'Style of position for vjs-text-track-display element should be relative');
assert.ok(textTrack.style.top === 'unset', 'Style of position for vjs-text-track-display element should be unset');
assert.ok(textTrack.style.bottom === '0px', 'Style of bottom for vjs-text-track-display element should be 0px');
player.dispose();
});
}