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 captions sticking to bottom for webkit browsers. #2702

Closed
wants to merge 2 commits into from

Conversation

chemoish
Copy link
Member

This PR is to start a conversation about addressing #2193.

screen shot 2015-10-14 at 7 15 54 pm

screen shot 2015-10-14 at 7 57 50 pm

Native (Chrome):

screen shot 2015-10-14 at 7 17 16 pm

screen shot 2015-10-14 at 7 17 02 pm

Resources:

Browsers:

  • Tested in OSX Chrome, Safari (Works), FF, IE (Not so much—to varying degrees)

Compatibility:


Evaluations:

  1. px, rem, em?
  2. bottom, top, translateY?
  3. -webkit-media-text-track-container, -webkit-media-text-track-display?

1. px

px must be used to keep interface consistency. However, this doesn't follow the em standard that is currently used in the player.

/* will make controls larger due to em usage */
.video-js
{
  font-size: 20px;
}

The problem with using em is that the browser? will dynamically modify the font-size on the -webkit-media-text-track-container, based on player's width. The larger the width, the larger the font-size, the larger the bottom offset will be.

/* will look fine on large screens, but poor/poorer on smaller screens */
video::-webkit-media-text-track-container {
  bottom: 0.75em;
}

As mentioned #2193 (comment).

2. & 3. translateY

bottom can only be used on the -webkit-media-text-track-container since it conflicts with styles found in -webkit-media-text-track-display [WebVTT implementation of line (WebVTT) modifies top (css)]. However, this will cause placement issues if you want to utilize line (WebVTT), as they both will stack/conflict.

translateY seems to be the best option. translateY gives the most predictable offset and it doesn't conflict with any other embedded styles.

The best part about it is that, IF, line (WebVTT) is configured, it will override any translateY styles set on -webkit-media-text-track-display.

WEBVTT

00:00.700 --> 00:04.110
Captions describe all relevant audio for the hearing impaired.
[ Heroic music playing for a seagull ]
/* will offset the text 45px from the bottom of the player */
video::-webkit-media-text-track-display {
    transform: transateY(-45px);
}
WEBVTT

00:00.700 --> 00:04.110 line:50%
Captions describe all relevant audio for the hearing impaired.
[ Heroic music playing for a seagull ]
/* will be ignored and overridden by `-webkit-media-text-track-display`'s `top`' to force the captions to be vertically centered */
video::-webkit-media-text-track-display {
    transform: transateY(-45px);
}

This will allow users to benefit from both.


Other stuff:

  • Positioning captions based on user activity or inactivity is possible (Control bar visibility).
  • No idea where the CSS belongs.
  • @mixin can be removed if npm i compass-mixins was favored.

@gkatsev
Copy link
Member

gkatsev commented Oct 15, 2015

Thanks for the PR and the detailed description!
The last time I tested this in safari, safari did weird stuff with this by first rendering the result in the correct place but then jumping it up by that same amount. It was very weird. I wonder whether using translate might not have this issue or maybe using pixel values fixes it but that is definitely something that should be looked into before merging.

@chemoish
Copy link
Member Author

I believe this is the case with modifying the container with bottom.
Translate did not have the double repaint issue.

I will thoroughly test tomorrow.
On Oct 14, 2015 8:50 PM, "Gary Katsevman" notifications@github.com wrote:

Thanks for the PR and the detailed description!
The last time I tested this in safari, safari did weird stuff with this by
first rendering the result in the correct place but then jumping it up by
that same amount. It was very weird. I wonder whether using translate might
not have this issue or maybe using pixel values fixes it but that is
definitely something that should be looked into before merging.


Reply to this email directly or view it on GitHub
#2702 (comment).

@chemoish
Copy link
Member Author

Safari has a double repaint issue with modifying the position of the -webkit-media-text-track-container.

translateY has no issues when placed on either -webkit-media-text-track-container or -webkit-media-text-track-display (Though benefits come with putting it on the display).

Safari does have a intermittent issue initializing/disposing captions, where the font-size quadruples* and the caption balloons, not sure how this can be overcome.

Safari:

screen shot 2015-10-15 at 2 38 06 pm

screen shot 2015-10-15 at 2 38 22 pm

@gkatsev gkatsev mentioned this pull request Oct 26, 2015
18 tasks
@@ -24,3 +24,7 @@
.vjs-subtitles { color: #fff /* Subtitles are white */; }
.vjs-captions { color: #fc6 /* Captions are yellow */; }
.vjs-tt-cue { display: block; }

video::-webkit-media-text-track-display {
@include transform(translateY(-45px));
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be 4em rather than 45px. 4.5em is a bit much, I think.
Also, can we add this value as a variable that defaults to 4em to the vars file? That way, a user could more easily change where the captions are if they have a custom control bar and are including the sass files.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should make this align with where we stick the emulated captions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do. I'll retest em, but expect to have the captions bleed into the video depending on the font-size set by the browser width.

Also, should the captions work like native -webkit, where they adjust for the control bar, or should they stay at a fixed (relative) position.

Also, we should make this align with where we stick the emulated captions.

Don't follow.

Copy link
Member

Choose a reason for hiding this comment

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

We should make the emulated captions and native captions work as close to each other as possible. The emulated captions already are aligned to leave room for the control bar, so, we should make sure that the native captions are similar. If the emulated captions go up/down with the control bar, we should do the same for the native captions as well.
I tried using em locally and it didn't actually change. But that is kind of what we want because the control bar resizes itself based on the font size :)

@gkatsev
Copy link
Member

gkatsev commented Oct 28, 2015

Also, thanks for this PR. Glad you were able to figure out how to get safari to behave (which I also verified). It looks good to me, but had a comment.

@chemoish
Copy link
Member Author

I initially didn't follow the whole emulated notion, but I figured it out ><.

Added some comments to make it more explicit.

So its closer, but won't be perfect. Emulated tracks modify the font size of the .vjs-text-track-display element, a child element. This causes the positioning to be based on the the font-size of the .video-js element.

Native tracks have their positioning and font-size tied to the same element, so the positioning is much more noticeable and, of course, varies depending on the width of the viewport.


Control bar (Chrome 3em/1.5em, FF 3em/1em):

screen shot 2015-10-28 at 5 50 59 pm

No control bar (Chrome 3em/1.5em, FF 3em/1em):

screen shot 2015-10-28 at 5 51 36 pm


^ is the reason why its such a different experience on huge viewports, but looks pretty similar on small ones.

@gkatsev
Copy link
Member

gkatsev commented Oct 29, 2015

Awesome. I'll try it out tomorrow and probably merge it in.

@gkatsev
Copy link
Member

gkatsev commented Oct 30, 2015

LGTM

@dmlap
Copy link
Member

dmlap commented Oct 30, 2015

Awesome! Thanks again, @chemoish!

@gkatsev gkatsev closed this in d0efd16 Oct 30, 2015
@chemoish chemoish deleted the feature/style-caption branch November 2, 2015 18:46
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.

3 participants