Always emulated text track list #2391

gkatsev · 2015-07-21T21:56:06Z

TextTrackList has a couple of event listeners that users rely on. By emulated the TextTrackList we can hopefully eliminate some of the friction that can occur during tech changes around the text track event handlers.

When switching techs, we want to clear out the old text tracks and then restore them using the currently appropriate tracks.

pam · 2015-07-21T22:01:47Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: f198faa
Build details: https://travis-ci.org/pam/video.js/builds/72025560

(Please note that this is a fully automated comment.)

gkatsev · 2015-07-21T22:02:50Z

I'll tackle tests next but it's working for me when switching from html to flash and back.

heff · 2015-07-21T22:13:22Z

src/js/tech/html5.js

@@ -86,6 +87,12 @@ class Html5 extends Tech {
   * @method dispose
   */
  dispose() {
+    let tt = this.el().textTracks;
+
+    tt.removeEventListener('change', Fn.bind(this, this.handleTextTrackChange));


If el().textTracks is a native list then I don't know if the Fn.bind is going to do what you want here. The remove-by-same-id only works with our internal event listeners. The native removeEventListener still looks for the same function.

Yep, you're right. I keep getting more and more output when switching techs.

I have a fix for this locally but for some odd reason it only wants to work when I'm setting through the code for removing the old text tracks...

So, the issue seems that if we remove the event listener in the same event tick as removing the track, we don't actually get the removetrack event. Looking at the spec, it seems like the removetrack events are queued to fire when tracks are removed, so, it makes sense for us to miss them.

I believe the solution in this case really is a setTimeout around the removal of tracks. Either that or create a remove handler that gets added on addtrack and bound to a particular track which then gets removed when that particular track gets removed. But I think that solution might be too clever.

Could we queue our own removetrack event in that case? Seems like handling that asynchronously could get messy when switching to the new tech.

I just realized we could probably just clean out our emulated text track list manually. However, if we remove the event listener asynchronously, it works fine too. Or, we can have the removetrack handler be smart and get added on addtrack and removed on removetrack if there are no longer any text tracks (which is what I tried to describe above).

I like the idea of something simple and less async, but whatever you think
works best.

Sent from mobile

heff · 2015-07-21T22:25:08Z

One question around tech changes but other than that looks really good to me.

Add event listeners in the constructor. Remove event listeners in dispose. 'removetrack' listener needs to be removed asynchronously to make sure that we get all the events that are to be triggered.

gkatsev · 2015-07-27T17:44:42Z

I updated things. I currently see a weird issue where when you switch back to the html5 tech, to get the non-current-locale text tracks to show, you need to select it twice. Not sure what is going on there.

pam · 2015-07-27T17:48:22Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 6d75a25
Build details: https://travis-ci.org/pam/video.js/builds/72875915

(Please note that this is a fully automated comment.)

gkatsev · 2015-07-27T18:30:03Z

I decided to not try and do anything asynchronous and just clear out our emulated text track list manually in dispose since nothing should be coming along across the tech switch from html5->flash

pam · 2015-07-27T18:35:14Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 486dc45
Build details: https://travis-ci.org/pam/video.js/builds/72882669

(Please note that this is a fully automated comment.)

gkatsev · 2015-07-27T21:44:51Z

test/unit/tech/html5.test.js

@@ -178,3 +178,48 @@ test('native source handler canHandleSource', function(){
  // Reset test video canPlayType
  Html5.TEST_VID.canPlayType = origCPT;
 });
+
+if (Html5.supportsNativeTextTracks()) {


these only run if we are using and support native text tracks, so, I'm only running the tests in those browsers as well because it'll fail otherwise.

gkatsev · 2015-07-27T21:55:54Z

I think I may have fixed the issue that text tracks have had with safari in this PR.

gkatsev · 2015-07-27T21:56:18Z

test/unit/tech/flash.test.js

@@ -70,6 +70,7 @@ test('dispose removes the object element even before ready fires', function() {
  mockFlash.off = noop;
  mockFlash.trigger = noop;
  mockFlash.el_ = {};
+  mockFlash.textTracks = () => ([]);


I think this is what was causing the issues in safari that we're seeing in master and elsewhere.

Please can we fix this? How do we fix this?

Oh, you did. Sweet. :)

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

Actually, looks like selecting the captions twice is caused by what #2412 fixes.

dmlap · 2015-07-30T15:00:46Z

src/js/tracks/text-track-list-converter.js

@@ -0,0 +1,47 @@
+let trackToJson = function(track) {


This function and file could use an explanatory comment.

dmlap · 2015-07-30T15:02:47Z

Other than my documentation comment, LGTM

heff · 2015-07-30T15:21:41Z

2nd dmlap. Lgtm!

dmlap · 2015-08-03T19:19:49Z

Superseded by #2425

gkatsev added 2 commits July 21, 2015 15:59

Always use emulated text track list.

0268fc6

TextTrackList has a couple of event listeners that users rely on. By emulated the TextTrackList we can hopefully eliminate some of the friction that can occur during tech changes around the text track event handlers.

Store and restore text tracks from JSON.

f198faa

When switching techs, we want to clear out the old text tracks and then restore them using the currently appropriate tracks.

heff reviewed Jul 21, 2015
View reviewed changes

gkatsev added 3 commits July 27, 2015 12:01

Only bind the functions once.

b0cb56e

Add event listeners in the constructor. Remove event listeners in dispose. 'removetrack' listener needs to be removed asynchronously to make sure that we get all the events that are to be triggered.

Remove some bracket notation

549354c

Add removal timeout after disposing things

6d75a25

Clear out emulated text tracks manually in dipose

486dc45

gkatsev added 7 commits July 27, 2015 14:42

mock out textTracks method

8327051

Use object shorthand

83f3cb7

Add some emulated tt tests

314105b

Update to qunit 1.18

228d79c

qunit module namespace utils tests

28e1844

Restore createElement immediately after we're done with it

e54c5cd

only run tests if native text tracks are supported by us

c76b3c9

gkatsev reviewed Jul 27, 2015
View reviewed changes

Rename ttToJson to trackToJson. Also export it.

b6de86d

gkatsev reviewed Jul 27, 2015
View reviewed changes

gkatsev added 4 commits July 27, 2015 18:40

Add mode. Make sure cues exist when re-adding tracks

0f64925

Add tests for the text track list converter

ca4743c

Ignore inband metadata value as well

79b1b19

fix linting errors

6ca59a3

dmlap reviewed Jul 30, 2015
View reviewed changes

dmlap mentioned this pull request Aug 3, 2015

Add documentation to #2391 #2425

Closed

dmlap closed this Aug 3, 2015

nickygerritsen mentioned this pull request Aug 10, 2015

PR #2391 / #2425 break dispose when no native tt is supported #2454

Closed

gkatsev deleted the always-emulated-text-track-list branch August 12, 2015 18:47

gkatsev mentioned this pull request Nov 16, 2015

Emulate HTMLTrackElement to enable load event. #2804

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always emulated text track list #2391

Always emulated text track list #2391

gkatsev commented Jul 21, 2015

pam commented Jul 21, 2015

gkatsev commented Jul 21, 2015

heff Jul 21, 2015

gkatsev Jul 21, 2015

gkatsev Jul 22, 2015

gkatsev Jul 24, 2015

gkatsev Jul 24, 2015

heff Jul 25, 2015

gkatsev Jul 25, 2015

heff Jul 25, 2015

heff commented Jul 21, 2015

gkatsev commented Jul 27, 2015

pam commented Jul 27, 2015

gkatsev commented Jul 27, 2015

pam commented Jul 27, 2015

gkatsev Jul 27, 2015

gkatsev commented Jul 27, 2015

gkatsev Jul 27, 2015

heff Jul 28, 2015

heff Jul 28, 2015

gkatsev commented Jul 28, 2015

dmlap Jul 30, 2015

dmlap commented Jul 30, 2015

heff commented Jul 30, 2015

dmlap commented Aug 3, 2015

Always emulated text track list #2391

Always emulated text track list #2391

Conversation

gkatsev commented Jul 21, 2015

pam commented Jul 21, 2015

gkatsev commented Jul 21, 2015

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

Choose a reason for hiding this comment

heff commented Jul 21, 2015

gkatsev commented Jul 27, 2015

pam commented Jul 27, 2015

gkatsev commented Jul 27, 2015

pam commented Jul 27, 2015

Choose a reason for hiding this comment

gkatsev commented Jul 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Jul 28, 2015

Choose a reason for hiding this comment

dmlap commented Jul 30, 2015

heff commented Jul 30, 2015

dmlap commented Aug 3, 2015