Fix track onload #2412

gkatsev · 2015-07-28T20:30:19Z

I believe this fixes #1914. Also, this makes it so that users can actually select native text tracks on the first try.

native captions load on demand. Sometimes, some of the tracks get loaded in by default by the user agent. We don't want the user agent to load stuff by default (unless it's a `default` track) so we add an `onload` handler to `disable` the tracks. However, if a user chooses the track, we want to let it through. `playing` is our arbitrary cutoff for user-agent vs user events.

imbcmdth · 2015-07-29T15:18:57Z

LGTM 🍔

imbcmdth · 2015-07-29T15:21:09Z

@gkatsev Is this something that will have to be fixed in vjs5 as well?

misteroneill · 2015-07-29T17:13:06Z

@imbcmdth Yes, there's a PR for that! #2413

dmlap · 2015-07-30T18:13:12Z

src/js/media/html5.js

@@ -61,6 +61,23 @@ vjs.Html5 = vjs.MediaTechController.extend({

    if (this['featuresNativeTextTracks']) {
      this.on('loadstart', vjs.bind(this, this.hideCaptions));
+
+      // native captions load on demand. Sometimes, some of the tracks get loaded in by default
+      // by the user agent. We don't want the user agent to load stuff by default


Why don't we want user agents to load unless the default attribute is present?

Certain browsers/players will display captions for the first track it encounters if there is no track element with the default attribute - without any interaction from the user. Merely having track elements will cause captions to show.

Could we fix this by setting the track mode to hidden explicitly when creating text tracks?

Nope, setting the mode to anything before the track has finished loading does nothing.

heff · 2015-08-03T23:57:05Z

Referenced in #1865, but there's a chromium bug open for this issue.

Just reiterating that showing any captions that exist by default is not really how the browsers should work.

(an it looks like it might be solved soon?)

misteroneill · 2015-08-04T12:31:55Z

IIRC, it was happening in iOS as well (I could be imagining that), which prompted this PR originally (the fix for the ticket mentioned above was a happy accident).

I can dig into this today and do a quick pass to see which browsers have the bad behavior.

misteroneill · 2015-08-04T21:23:18Z

This PR originally added another layer of workarounds to fix broken behavior in Safari on iPhone that was reported internally. But that issue was caused by the workaround from #1865. Upon further investigation, it seems the initial workaround is no longer needed - Chrome does not appear to be playing a track by default when no default attribute is present.

Both @dmlap and I agree that allowing clients to keep their default behavior is preferable to adding workarounds for workarounds, so I've changed this to simply remove the workaround from #1865.

Safari on iPhone still has some interesting and perhaps unintuitive behavior. It appears that it will choose a caption for the user if the user previously chose that caption language. If "Off" was previously selected, it will be going forward. I couldn't find a way to clear this apparent preference because it appears to be cached by the player - not the browser!

I did notice one strange thing with Safari on iPhone. The TextTracksList instance that gets created appears to magically gain an extra TextTrack captions object with an empty "language"... without a call to addTrack_()! This appears to be the cause of iOS's display of an "Unknown CC" option in the CC menu. I wasn't able to track down the cause of this today, but wanted to note it for future investigation.

@dmlap

After some pretty rigorous testing today and discussion with @dmlap, it seems the best course of action here is to let Safari on iOS keep its default behavior because it appears that it caches caption language preferences in the player application rather than the browser. Safari's behavior appears to be: - If no caption language was previously selected (e.g. "Off") then no captions are downloaded or displayed. - When the user chooses a language, the .vtt file is downloaded and captions for that language begin. - When the user leaves and returns to the video in question (even with an explicit cache clear) the previously selected .vtt file is used. Removing this code lets all browsers/user agents do their default behavior, which seems like the best course of action.

misteroneill · 2015-08-04T21:56:06Z

Additional discovery. Going to Settings > Accessibility > Subtitles & Captioning and disabling Closed Captions + SDH appears to turn off the language selection caching behavior. Videos will then default to "Auto (Recommended)".

dmlap · 2015-08-04T22:08:40Z

Deserves a test pass but lgtm

heff · 2015-08-05T17:56:14Z

src/js/media/html5.js

@@ -438,8 +415,6 @@ vjs.Html5.prototype.addRemoteTextTrack = function(options) {
    if (track.readyState >= 2) {


If we're going for default behavior now would we not just kill this whole function, or do we still need to force metadata tracks to hidden?

I was hesitant to delete this bit because I didn't totally understand its intent. I'd like @gkatsev to review tomorrow when he's back.

heff · 2015-08-05T17:56:46Z

Just confirmed the latest Chrome doesn't show tracks by default unless they have a default attribute (woo!), and Firefox works the same way.

Safari on desktop and mobile still shows tracks by default. They make it easy enough to turn off the tracks in the native UI, but it's still not ideal. Would anyone up for submitting a webkit bug to mimic the chrome bug that was fixed?

If we revert this fix I think we should test how the captions menu performs in the different browsers. Whether or not it shows the right track as 'on' depending on the browser's behavior.

Do we have any idea how this works in IE?

One other comment inline but otherwise this looks good to me.

misteroneill · 2015-08-05T18:17:45Z

I checked IE9 and it seems to behave the same as Chrome/Firefox. I'll check others today. I didn't see the same behavior you're describing with Safari on desktop at all - it behaved like Chrome/Firefox/IE9.

iOS seems to be the only odd case for me, but its behavior doesn't seem broken - it should only choose a text track if that language has been chosen previously in the native player. Turning off the captions accessibility feature resets it (though choosing a caption again seems to cause the defaulting behavior).

heff · 2015-08-05T19:13:09Z

Just checked Desktop Safari 8.0.7 on a coworker's machine who doesn't use safari, and captions showed by default, using a plain video tag and no default attribute on the captions. Could you have already selected 'off' instead of 'auto' some time previously?

On iOS 8.3 it seems to also always show captions by default for me. I tried toggling Closed Captions + SDH but it didn't change anything. I'm upgrading to 8.4 to check that. Otherwise we might need to get a fresh phone somehow to know for sure. Basically I think it's worth submitting a webkit bug if on a new phone, captions show by default on a plain video tag when no default attribute is used.

misteroneill · 2015-08-05T20:08:47Z

Ok, I was finally able to reproduce in Safari and in Chrome (surprisingly). I don't have access to iOS at home and the simulator is crashing when I try to play a video.

I'm thinking this is a difference between local text tracks and remote text tracks. The issue that was reported internally was for a video with remote text tracks. Now that I'm looking at a video with local text tracks (the default index.html in the VideoJS sandbox for both 4 and 5), I'm seeing the behavior you're describing - and the Chrome bug mentioned earlier.

Edit: Also, verified that IE9/10/11 and Firefox behave in the seemingly expected way: they don't play any captions for local or remote tracks unless explicitly requested.

heff · 2015-08-06T00:16:54Z

By remote do you mean cross-domain, and that local tracks work differently from cross-domain tracks??

misteroneill · 2015-08-06T14:10:34Z

I meant the distinction in code between addTextTrack and addRemoteTextTrack. I'm not entirely clear on the difference - or if I'm right on this - but the original changes in this PR were in response to looking at a Brightcove player pulling a video from the catalog. Upon looking at the default sandbox player (which does not), the default behavior is totally different across browsers. I'm starting to think I may just need to pop off my commit on this branch and go back to the workaround behavior, but I'll talk to @gkatsev today about that.

misteroneill · 2015-08-06T16:02:35Z

I've created a wiki page to start documenting the various handling of text tracks / captions in different browsers because - as @gkatsev has pointed out to me - there are some idiosyncratic behaviors there.

This reverts commit 0a33e1c.

misteroneill · 2015-08-06T17:15:46Z

Rolled back the rollback. 🎢

gkatsev · 2015-08-10T19:14:38Z

So... how bad would it be if we just removed all this code, made the browser just do whatever it does and opened a bug against webkit? @videojs/core-committers

dmlap · 2015-08-10T19:19:32Z

@gkatsev +1 I'm in favor of letting the browser do what it does naturally.

gkatsev · 2015-08-10T19:20:35Z

This does mean that until webkit will get it fixed, we're going to get captions showing up by default in some cases (particualrly, iOS ans Safari), even if the default attribute wasn't set.

misteroneill · 2015-08-10T19:20:51Z

If you want to do that, just revert my revert: 5ec859c 😄

gkatsev · 2015-08-10T19:27:11Z

@misteroneill well, we'd be able to delete some more stuff than just that commit. :)

gkatsev · 2015-08-12T18:32:58Z

I'm going to close this in favor of a new PR that removes all the codes.

gkatsev · 2015-08-12T20:36:11Z

Opened a bug against webkit: https://bugs.webkit.org/show_bug.cgi?id=147951

…l browser behavior fixed in videojs/video.js#2412.

gkatsev added 2 commits July 28, 2015 16:26

Only supress default loading non-default tracks

4450e51

gkatsev mentioned this pull request Jul 28, 2015

Always emulated text track list #2391

Closed

add semicolon

059791e

dmlap reviewed Jul 30, 2015
View reviewed changes

misteroneill force-pushed the fix-track-onload branch from 792944b to 0a33e1c Compare August 4, 2015 21:34

heff reviewed Aug 5, 2015
View reviewed changes

heff mentioned this pull request Aug 5, 2015

Track metadata issues with video.js#4.12.12 #2436

Closed

Revert "Roll back changes and remove breaking code"

5ec859c

This reverts commit 0a33e1c.

gkatsev added a commit to gkatsev/video.js that referenced this pull request Aug 6, 2015

port videojs#2412 to master

044dcf5

gkatsev added this to the Text Tracks milestone Aug 10, 2015

gkatsev mentioned this pull request Aug 10, 2015

port #2412 to master #2413

Closed

gkatsev closed this Aug 12, 2015

gkatsev deleted the fix-track-onload branch August 12, 2015 18:47

chemoish pushed a commit to chemoish/videojs-chapter-thumbnails that referenced this pull request Sep 1, 2015

fix(chapter-thumbnail): Setting track to default to accommodate norma…

a55aa95

…l browser behavior fixed in videojs/video.js#2412.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix track onload #2412

Fix track onload #2412

gkatsev commented Jul 28, 2015

imbcmdth commented Jul 29, 2015

imbcmdth commented Jul 29, 2015

misteroneill commented Jul 29, 2015

dmlap Jul 30, 2015

misteroneill Jul 30, 2015

dmlap Jul 30, 2015

gkatsev Jul 31, 2015

heff commented Aug 3, 2015

misteroneill commented Aug 4, 2015

misteroneill commented Aug 4, 2015

misteroneill commented Aug 4, 2015

dmlap commented Aug 4, 2015

heff Aug 5, 2015

misteroneill Aug 5, 2015

heff commented Aug 5, 2015

misteroneill commented Aug 5, 2015

heff commented Aug 5, 2015

misteroneill commented Aug 5, 2015

heff commented Aug 6, 2015

misteroneill commented Aug 6, 2015

misteroneill commented Aug 6, 2015

misteroneill commented Aug 6, 2015

gkatsev commented Aug 10, 2015

dmlap commented Aug 10, 2015

gkatsev commented Aug 10, 2015

misteroneill commented Aug 10, 2015

gkatsev commented Aug 10, 2015

gkatsev commented Aug 12, 2015

gkatsev commented Aug 12, 2015

		@@ -438,8 +415,6 @@ vjs.Html5.prototype.addRemoteTextTrack = function(options) {
		if (track.readyState >= 2) {

Fix track onload #2412

Fix track onload #2412

Conversation

gkatsev commented Jul 28, 2015

imbcmdth commented Jul 29, 2015

imbcmdth commented Jul 29, 2015

misteroneill commented Jul 29, 2015

dmlap Jul 30, 2015

Choose a reason for hiding this comment

misteroneill Jul 30, 2015

Choose a reason for hiding this comment

dmlap Jul 30, 2015

Choose a reason for hiding this comment

gkatsev Jul 31, 2015

Choose a reason for hiding this comment

heff commented Aug 3, 2015

misteroneill commented Aug 4, 2015

misteroneill commented Aug 4, 2015

misteroneill commented Aug 4, 2015

dmlap commented Aug 4, 2015

heff Aug 5, 2015

Choose a reason for hiding this comment

misteroneill Aug 5, 2015

Choose a reason for hiding this comment

heff commented Aug 5, 2015

misteroneill commented Aug 5, 2015

heff commented Aug 5, 2015

misteroneill commented Aug 5, 2015

heff commented Aug 6, 2015

misteroneill commented Aug 6, 2015

misteroneill commented Aug 6, 2015

misteroneill commented Aug 6, 2015

gkatsev commented Aug 10, 2015

dmlap commented Aug 10, 2015

gkatsev commented Aug 10, 2015

misteroneill commented Aug 10, 2015

gkatsev commented Aug 10, 2015

gkatsev commented Aug 12, 2015

gkatsev commented Aug 12, 2015