Emulate HTMLTrackElement to enable load event. #2804

chemoish · 2015-11-11T03:02:27Z

Figure out when track elements are loaded so we can manipulate cues without having to do setTimeout hacks.

Addresses #2784.

Also addresses (maybe) #2799.

chemoish · 2015-11-11T03:03:15Z

src/js/control-bar/text-track-controls/chapters-button.js

      let track = tracks[i];
+
      if (track['kind'] === this.kind_) {


Followed #1949

pam · 2015-11-11T03:12:33Z

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

Commit: d318bcc2d401d2744a7454e9c00bcd47435c0b4f
Build details: https://travis-ci.org/pam/video.js/builds/90449472

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

chemoish · 2015-11-11T03:13:54Z

src/js/tracks/html-track-element-list.js

+
+import * as browser from '../utils/browser.js';
+
+class HtmlTrackElementList {


Followed TextTrackList even if this wasn't shimming anything.

gkatsev · 2015-11-11T03:20:15Z

Awesome, I'll take a look tomorrow.

chemoish · 2015-11-11T03:21:24Z

Will need help:

browser compatibility
testing strategy
probably needs more documentation—fit with existing jsdoc integration

Works (OSX 10.10.5):

Chrome 46 (mp4, mpeg-dash [did not test drm])
Safari 8.0.8
Firefox 40
IE 11 Windows 8.1 (virtualbox)

this.remoteTextTracks() and this.remoteTextTrackEls() work as expected.

chemoish · 2015-11-11T18:36:03Z

src/js/tracks/text-track.js

@@ -269,7 +271,15 @@ var loadTrack = function(src, track) {
    }

    track.loaded_ = true;
-    parseCues(responseBody, track);
+
+    if (typeof window['WebVTT'] !== 'function') {


add comments if this isn't an issue in any environment beyond local.

This is for the alt/video.novtt.js build.

gkatsev · 2015-11-16T15:46:25Z

Thanks, this is definitely the right track, if you will ;)

chemoish · 2015-11-18T01:39:28Z

src/js/tracks/html-track-element.js

+    });
+
+    if (browser.IS_IE8) {
+      return this;


pam · 2015-11-18T02:10:17Z

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

Commit: 54b35f881b1d1968f4c6e56ee65aac362145fcc7
Build details: https://travis-ci.org/pam/video.js/builds/91732554

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

…indow.WebVTT` is inlined): 1. Create a `TextTrack` 2. Added `TextTrack` to `TextTrackList` 3. Loaded `TextTrack.cues` (!= null) All before the `htmlTrackElement` was added to `this.remoteTextTrackEls()` within `addRemoteTextTrack()`.

…ks correctly from DOM. - Added documentation - Added player test - Added api test NOTE: important that `addTrack_` order matters in tech.js::addRemoteTextTrack.

chemoish · 2015-12-07T23:36:56Z

test/unit/player.test.js

@@ -835,6 +835,39 @@ test('createModal()', function() {
  strictEqual(player.children().indexOf(modal), -1, 'modal was removed from player\'s children');
 });

+test('should return correct remote text track values', function () {


Not sure where this should go or how this should be tested.

Note: techOrder: ['html5']

techFaker doesn't initialize the <track> correctly, but fires the player.addRemoteTextTrack correctly. Open to recommendations.

I think it probably would be better to move this test into test/tracks/tracks.test.js

chemoish · 2015-12-07T23:39:10Z

Tested <track>:

IE 11 Win 8.1 (virtualbox)
Chrome (latest)
Safari (latest)
FF (latest)

Tested addRemoteTextTrack:

IE 11 Win 8.1 (virtualbox)
Chrome (latest)
Safari (latest)
FF (latest)

All passed.

chemoish · 2015-12-07T23:39:30Z

~~Tests pass locally.~~ FF doesn't pass.

player is made ready asynchronously

gkatsev · 2015-12-07T23:56:20Z

We also should add some tests for the new functionality of addRemoteTextTrack as well as tests for remoteTextTracksEls()

chemoish · 2015-12-08T00:39:48Z

I am down for better tests, but what specifically did you have in mind?

The only new functionality specified for addRemoteTextTrack was the addition of managing the remoteTextTrackEls.

https://github.com/videojs/video.js/pull/2804/files?diff=unified#diff-5c5c01f73339e29a9ae3816bd7fd777d (should cover remoteTextTrackEls())
https://github.com/videojs/video.js/pull/2804/files?diff=unified#diff-ff7bdfd93378f289476b912a6a3e07afR838 (should cover remoteTextTrackEls instantiation; addRemoteTextTrack, remoteRemoteTextTrack verification).

tech::addRemoteTextTrack did unify the return of the htmlTrackElement, would that be beneficial to test?

gkatsev · 2015-12-08T01:09:34Z

Yep, basically just a simple test that verifies that when you call addRemoteTextTrack you can then see the element that was returned in the remoteTextTrackEls list.

- Add html track element test

gkatsev · 2015-12-08T02:10:02Z

Sweet, LGTM.

gkatsev · 2015-12-08T16:26:43Z

Merged! Thanks @chemoish. Also, thanks @OwenEdwards for the initial workup oh so long ago :)

@gkatsev

* upstream/stable: (479 commits) v5.4.4 @gkatsev switched to use custom vtt.js from npm. closes videojs#2905 v5.4.3 @gkatsev updated options customizer and github-release options. closes videojs#2903 v5.4.2 @gkatsev updated grunt-release config. closes videojs#2900 v5.4.1 @gkatsev added chg- and github- release for next releases. closes videojs#2899 v5.4.0 @gkatsev added ability to release next tag from master. closes videojs#2894 @gkatsev added nullcheck for cues in updateForTrack. Fixes videojs#2870. closes videojs#2896 @chemoish emulated HTMLTrackElement to enable track load events. closes videojs#2804 @gkatsev added a Player#reset method. Fixes videojs#2852. closes videojs#2880 @nick11703 changed multiline comments in sass with single-line comments. closes videojs#2827 @gkatsev added Player#tech. Fixes videojs#2617. closes videojs#2883 @misteroneill updated videojs-ie8 to 1.1.1. closes videojs#2869 v5.3.0 @imbcmdth added sourceOrder option for source-first ordering in selectSource. closes videojs#2847 @forbesjo updated formatTime to not go negative. closes videojs#2821 v5.2.4 ...

chemoish reviewed Nov 11, 2015
View reviewed changes

gkatsev mentioned this pull request Nov 17, 2015

Chapter Menu is Not Working in 4.12 #2131

Closed

chemoish reviewed Nov 18, 2015
View reviewed changes

src/js/tracks/html-track-element.js

});

if (browser.IS_IE8) {

return this;

Copy link

Member Author

chemoish Nov 18, 2015

Choose a reason for hiding this comment

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

fix ^

Carey Hinoki added 10 commits December 7, 2015 15:20

IE8 compatibility.

ceaf4af

Formatting and documentation.

452554f

Fix removeTrack_ to trigger event if track exists.

47e6bab

Addition fix for IE8 JScript_DontEnum_Bug.

1c90f74

Fix trackElement.srclang to be sourced from track.language.

c7a0033

Formatting.

71828a7

Adding HTMLTrackElement + HTMLTrackElementList tests.

756af99

Fix TextTrack existence identification for IE8 + clean up.

4ad1d2f

Fix native text track capable browsers to initialize remote text trac…

eec4a2c

…ks correctly from DOM. - Added documentation - Added player test - Added api test NOTE: important that `addTrack_` order matters in tech.js::addRemoteTextTrack.

chemoish force-pushed the feature/text-track-element branch from 6b29dbc to eec4a2c Compare December 7, 2015 23:22

chemoish reviewed Dec 7, 2015
View reviewed changes

gkatsev and others added 2 commits December 7, 2015 18:48

player is made ready asynchronously

f054347

Merge pull request #1 from gkatsev/chemoish-ttel

4af827d

player is made ready asynchronously

Refactor remote text track test.

077a4c3

- Add html track element test

gkatsev added this to the Text Tracks milestone Dec 8, 2015

chemoish closed this in 8f2bc92 Dec 8, 2015

chemoish mentioned this pull request Jan 26, 2016

Text Track chapters have a race condition on instantiation. #2784

Closed

walsh9 mentioned this pull request Jan 28, 2016

Use new videojs 'loadeddata' event instead of old hack walsh9/videojs-transcript#7

Open

borzaka mentioned this pull request Jul 6, 2017

The chapters Text Tracks cues sometimes isn't ready on loadedmetadata #4468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emulate HTMLTrackElement to enable load event. #2804

Emulate HTMLTrackElement to enable load event. #2804

chemoish commented Nov 11, 2015

chemoish Nov 11, 2015

pam commented Nov 11, 2015

chemoish Nov 11, 2015

gkatsev commented Nov 11, 2015

chemoish commented Nov 11, 2015

chemoish Nov 11, 2015

gkatsev Nov 16, 2015

gkatsev commented Nov 16, 2015

chemoish Nov 18, 2015

pam commented Nov 18, 2015

chemoish Dec 7, 2015

gkatsev Dec 7, 2015

chemoish commented Dec 7, 2015

chemoish commented Dec 7, 2015

gkatsev commented Dec 7, 2015

chemoish commented Dec 8, 2015

gkatsev commented Dec 8, 2015

gkatsev commented Dec 8, 2015

gkatsev commented Dec 8, 2015


		import * as browser from '../utils/browser.js';

		class HtmlTrackElementList {

Emulate HTMLTrackElement to enable load event. #2804

Emulate HTMLTrackElement to enable load event. #2804

Conversation

chemoish commented Nov 11, 2015

chemoish Nov 11, 2015

Choose a reason for hiding this comment

pam commented Nov 11, 2015

chemoish Nov 11, 2015

Choose a reason for hiding this comment

gkatsev commented Nov 11, 2015

chemoish commented Nov 11, 2015

chemoish Nov 11, 2015

Choose a reason for hiding this comment

gkatsev Nov 16, 2015

Choose a reason for hiding this comment

gkatsev commented Nov 16, 2015

chemoish Nov 18, 2015

Choose a reason for hiding this comment

pam commented Nov 18, 2015

chemoish Dec 7, 2015

Choose a reason for hiding this comment

gkatsev Dec 7, 2015

Choose a reason for hiding this comment

chemoish commented Dec 7, 2015

chemoish commented Dec 7, 2015

gkatsev commented Dec 7, 2015

chemoish commented Dec 8, 2015

gkatsev commented Dec 8, 2015

gkatsev commented Dec 8, 2015

gkatsev commented Dec 8, 2015