fix: cues at startTime 0 do not fire (#4152)

Previously timeupdate would fire before the video was playing, and the tech was not ready. This caused issues when preload was set to auto, because cuechange would fire before the video was even started for cues with a startTime of 0.

Wait for tech to be ready before watching for timeupdate
update unit tests to use TechFaker
Add a unit test to verify that we wait for Tech to be ready.

src/js/tracks/text-track.js

@@ -198,7 +198,9 @@ class TextTrack extends Track {
     });
 
     if (mode !== 'disabled') {
-      tt.tech_.on('timeupdate', timeupdateHandler);
+      tt.tech_.ready(() => {
+        tt.tech_.on('timeupdate', timeupdateHandler);
+      }, true);
     }
 
     /**
@@ -236,7 +238,10 @@ class TextTrack extends Track {
         }
         mode = newMode;
         if (mode === 'showing') {
-          this.tech_.on('timeupdate', timeupdateHandler);
+
+          this.tech_.ready(() => {
+            this.tech_.on('timeupdate', timeupdateHandler);
+          }, true);
         }
         /**
          * An event that fires when mode changes on this track. This allows
@@ -339,14 +344,17 @@ class TextTrack extends Track {
   addCue(originalCue) {
     let cue = originalCue;
 
-    if (!(originalCue instanceof window.vttjs.VTTCue)) {
+    if (window.vttjs && !(originalCue instanceof window.vttjs.VTTCue)) {
       cue = new window.vttjs.VTTCue(originalCue.startTime, originalCue.endTime, originalCue.text);
 
       for (const prop in originalCue) {
         if (!(prop in cue)) {
           cue[prop] = originalCue[prop];
         }
       }
+
+      // make sure that `id` is copied over
+      cue.id = originalCue.id;
     }
 
     const tracks = this.tech_.textTracks();

test/unit/tech/tech-faker.js

@@ -9,7 +9,9 @@ class TechFaker extends Tech {
 
   constructor(options, handleReady) {
     super(options, handleReady);
-    this.triggerReady();
+    if (!options || options.autoReady !== false) {
+      this.triggerReady();
+    }
   }
 
   createEl() {

test/unit/tracks/html-track-element.test.js

@@ -1,17 +1,16 @@
 /* eslint-env qunit */
 import HTMLTrackElement from '../../../src/js/tracks/html-track-element.js';
-import TextTrackList from '../../../src/js/tracks/text-track-list.js';
+import TechFaker from '../tech/tech-faker';
 
-const defaultTech = {
-  textTracks() {
-    return new TextTrackList();
+QUnit.module('HTML Track Element', {
+  beforeEach() {
+    this.tech = new TechFaker();
   },
-  on() {},
-  off() {},
-  currentTime() {}
-};
-
-QUnit.module('HTML Track Element');
+  afterEach() {
+    this.tech.dispose();
+    this.tech = null;
+  }
+});
 
 QUnit.test('html track element requires a tech', function(assert) {
   assert.throws(
@@ -34,7 +33,7 @@ QUnit.test('can create a html track element with various properties', function(a
     label,
     language,
     src,
-    tech: defaultTech
+    tech: this.tech
   });
 
   assert.equal(typeof htmlTrackElement.default, 'undefined', 'we have a default');
@@ -48,7 +47,7 @@ QUnit.test('can create a html track element with various properties', function(a
 
 QUnit.test('defaults when items not provided', function(assert) {
   const htmlTrackElement = new HTMLTrackElement({
-    tech: defaultTech
+    tech: this.tech
   });
 
   assert.equal(typeof htmlTrackElement.default, 'undefined', 'we have a default');

test/unit/tracks/text-track-list.test.js

@@ -2,6 +2,7 @@
 import TextTrackList from '../../../src/js/tracks/text-track-list.js';
 import TextTrack from '../../../src/js/tracks/text-track.js';
 import EventTarget from '../../../src/js/event-target.js';
+import TechFaker from '../tech/tech-faker';
 
 QUnit.module('Text Track List');
 QUnit.test('trigger "change" event when "modechange" is fired on a track', function(assert) {
@@ -23,11 +24,7 @@ QUnit.test('trigger "change" event when "modechange" is fired on a track', funct
 });
 
 QUnit.test('trigger "change" event when mode changes on a TextTrack', function(assert) {
-  const tt = new TextTrack({
-    tech: {
-      on() {}
-    }
-  });
+  const tt = new TextTrack({tech: new TechFaker()});
   const ttl = new TextTrackList([tt]);
   let changes = 0;
   const changeHandler = function() {

test/unit/tracks/text-track.test.js

@@ -7,27 +7,16 @@ import TextTrack from '../../../src/js/tracks/text-track.js';
 import TestHelpers from '../test-helpers.js';
 import proxyquireify from 'proxyquireify';
 import sinon from 'sinon';
-import TextTrackList from '../../../src/js/tracks/text-track-list.js';
 
 const proxyquire = proxyquireify(require);
-const defaultTech = {
-  textTracks() {
-    return new TextTrackList();
-  },
-  on() {},
-  off() {},
-  currentTime() {}
-};
 
 QUnit.module('Text Track', {
   beforeEach() {
-    this.oldVttjs = window.vttjs;
-    window.vttjs = {
-      VTTCue: Object
-    };
+    this.tech = new TechFaker();
   },
   afterEach() {
-    window.vttjs = this.oldVttjs;
+    this.tech.dispose();
+    this.tech = null;
   }
 });
 
@@ -38,7 +27,7 @@ TrackBaseline(TextTrack, {
   mode: 'disabled',
   label: 'English',
   language: 'en',
-  tech: defaultTech
+  tech: new TechFaker()
 });
 
 QUnit.test('requires a tech', function(assert) {
@@ -52,15 +41,15 @@ QUnit.test('can create a TextTrack with a mode property', function(assert) {
   const mode = 'disabled';
   const tt = new TextTrack({
     mode,
-    tech: defaultTech
+    tech: this.tech
   });
 
   assert.equal(tt.mode, mode, 'we have a mode');
 });
 
 QUnit.test('defaults when items not provided', function(assert) {
   const tt = new TextTrack({
-    tech: TechFaker
+    tech: this.tech
   });
 
   assert.equal(tt.kind, 'subtitles', 'kind defaulted to subtitles');
@@ -71,43 +60,43 @@ QUnit.test('defaults when items not provided', function(assert) {
 
 QUnit.test('kind can only be one of several options, defaults to subtitles', function(assert) {
   let tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     kind: 'foo'
   });
 
   assert.equal(tt.kind, 'subtitles', 'the kind is set to subtitles, not foo');
   assert.notEqual(tt.kind, 'foo', 'the kind is set to subtitles, not foo');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     kind: 'subtitles'
   });
 
   assert.equal(tt.kind, 'subtitles', 'the kind is set to subtitles');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     kind: 'captions'
   });
 
   assert.equal(tt.kind, 'captions', 'the kind is set to captions');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     kind: 'descriptions'
   });
 
   assert.equal(tt.kind, 'descriptions', 'the kind is set to descriptions');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     kind: 'chapters'
   });
 
   assert.equal(tt.kind, 'chapters', 'the kind is set to chapters');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     kind: 'metadata'
   });
 
@@ -116,29 +105,29 @@ QUnit.test('kind can only be one of several options, defaults to subtitles', fun
 
 QUnit.test('mode can only be one of several options, defaults to disabled', function(assert) {
   let tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     mode: 'foo'
   });
 
   assert.equal(tt.mode, 'disabled', 'the mode is set to disabled, not foo');
   assert.notEqual(tt.mode, 'foo', 'the mode is set to disabld, not foo');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     mode: 'disabled'
   });
 
   assert.equal(tt.mode, 'disabled', 'the mode is set to disabled');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     mode: 'hidden'
   });
 
   assert.equal(tt.mode, 'hidden', 'the mode is set to hidden');
 
   tt = new TextTrack({
-    tech: defaultTech,
+    tech: this.tech,
     mode: 'showing'
   });
 
@@ -149,7 +138,7 @@ QUnit.test('cue and activeCues are read only', function(assert) {
   const mode = 'disabled';
   const tt = new TextTrack({
     mode,
-    tech: defaultTech
+    tech: this.tech
   });
 
   tt.cues = 'foo';
@@ -161,7 +150,7 @@ QUnit.test('cue and activeCues are read only', function(assert) {
 
 QUnit.test('mode can only be set to a few options', function(assert) {
   const tt = new TextTrack({
-    tech: defaultTech
+    tech: this.tech
   });
 
   tt.mode = 'foo';
@@ -186,7 +175,7 @@ QUnit.test('mode can only be set to a few options', function(assert) {
 
 QUnit.test('cues and activeCues return a TextTrackCueList', function(assert) {
   const tt = new TextTrack({
-    tech: defaultTech
+    tech: this.tech
   });
 
   assert.ok(tt.cues.getCueById, 'cues are a TextTrackCueList');
@@ -195,7 +184,7 @@ QUnit.test('cues and activeCues return a TextTrackCueList', function(assert) {
 
 QUnit.test('cues can be added and removed from a TextTrack', function(assert) {
   const tt = new TextTrack({
-    tech: defaultTech
+    tech: this.tech
   });
   const cues = tt.cues;
 
@@ -216,6 +205,49 @@ QUnit.test('cues can be added and removed from a TextTrack', function(assert) {
   assert.equal(cues.length, 3, 'we now have 3 cues');
 });
 
+QUnit.test('does not fire cuechange before Tech is ready', function(assert) {
+  const done = assert.async();
+  const player = TestHelpers.makePlayer({techfaker: {autoReady: false}});
+  let changes = 0;
+  const tt = new TextTrack({
+    tech: player.tech_,
+    mode: 'showing'
+  });
+  const cuechangeHandler = function() {
+    changes++;
+  };
+
+  tt.addCue({
+    id: '1',
+    startTime: 0,
+    endTime: 5
+  });
+
+  tt.oncuechange = cuechangeHandler;
+  tt.addEventListener('cuechange', cuechangeHandler);
+
+  player.tech_.currentTime = function() {
+    return 0;
+  };
+
+  player.tech_.trigger('timeupdate');
+  assert.equal(changes, 0, 'a cuechange event is not triggered');
+
+  player.tech_.on('ready', function() {
+    player.tech_.currentTime = function() {
+      return 0.2;
+    };
+
+    player.tech_.trigger('timeupdate');
+
+    assert.equal(changes, 2, 'a cuechange event trigger addEventListener and oncuechange');
+
+    player.dispose();
+    done();
+  });
+  player.tech_.triggerReady();
+});
+
 QUnit.test('fires cuechange when cues become active and inactive', function(assert) {
   const player = TestHelpers.makePlayer();
   let changes = 0;
@@ -283,7 +315,7 @@ QUnit.test('tracks are parsed if vttjs is loaded', function(assert) {
 
   /* eslint-disable no-unused-vars */
   const tt = new TextTrack_({
-    tech: defaultTech,
+    tech: this.tech,
     src: 'http://example.com'
   });
   /* eslint-enable no-unused-vars */