Refactoring chapters button handling and fixing several issues

[cervengoc]

Description

• Refactored ChaptersButton, broke logic into several methods.
• Fixed the issues in Issues with chapters #3447 about in some browsers tracks have an empty cues array instead of null. Now we always subscribe to load event, and force an update. Also, track changes are handled, so chapters track can now be changed at runtime.
• Fixed the issue in Issues with chapters #3447 about chapters menu items are not selectable. Now automatic update of the selected item based on player time works fine.
• Implemented the usage of the chapters track's label attribute as menu title, if it's present. If not, we fall back to the localized "Chapters" title.
• Refined the menu styling, so that vjs-menu-title telement won't get the hover effect, It would confuse users, because they might believe that the title item is a clickable item too.

Testing

No unit tests for the functionality so far, because

• there were no unit tests before either
• I'm a new contributor and don't know the technical test environment

I've added some unit tests too.

Manually tested in IE 11, Chrome 51.

[cervengoc]

I added some basic unit tests for chapters,

@gkatsev Could you please review this PR?

[cervengoc]

This could be improved by implementing somehow 'cueadded' and 'cueremoved' events. Then ChaptersButton could listen to them and update automatically.

[gkatsev]

why increase specificity? Can we drop li in here?

[cervengoc]

Because the menu title is a li too and I wanted to disable the hover effect there. Could have used the "not" selector but chose not to because of browser support.

[cervengoc]

@gkatsev Any news on this one? Did you have time to review the code? I tried to add the mentioned test with disabling native text tracks, but I got error then, seemed like tech_ is undefined. I had no time to investigate the cause though.

[gkatsev]

cue should just be declared on line 116.

Also, it seems like it's possible fors cues to be null or undefined in here, so, we'd want a null check.

[gkatsev]

It seems like with these changes I can no longer see chapters if I'm using the native text tracks. With emulated text tracks, it seems to work great.

[cervengoc]

Interesting, for me it was working with native chapters track in Chrome latest. Could you please share you non-working example somehow so I can work on it?

[gkatsev]

I was just testing in the sandbox example, loading in the chapter from the docs folder:

    <track kind="chapters" src="../docs/examples/elephantsdream/chapters.en.vtt" srclang="en" label="English"></track>

[cervengoc]

When I enable native text tracks in the sandbox at descriptions.html it seems like none of the tracks are actually loading. Can't understand at first glance, I'll try to look at it tomorrow more deeply. I'd be happy if you could check it too.

On the other hand I realized an issue in my code at this review, which occures when the chapters track is not the last track. I'll fix that too, but that's not related to this native tracks issue.

[cervengoc]

Ok I'm too tired to understand my own code. There should not be issue with track order. Still I have no idea yet what's happening in the example.

[cervengoc]

It seems like there is some cross-origin issue in Chrome with the example html, as Chrome blocks loading tracks with file:// protocol. I think this is why the load event actually never fires. Is there any way to work it around? Probably I'm doing something simple in the wrong way.

[gkatsev]

You'd probably want to run it through a web server. You can try grunt connect:dev which makes one.

[cervengoc]

Thank you, didn't know about grunt connect, always good to learn something new :)

I've found the issue. The updateHandler_ was initialized after parent constructor call, so it was undefined at first setTrack, so no handler was attached to load event. Now it works fine at me, and tests pass too.

[cervengoc]

Okay, had quite a bit of work and now it seems to be all fine.

[cervengoc]

One more notice, in the sandbox chapters demo I've noticed that the chapters menu shows an guly vertical scrollbar and an even uglier horizontal one. I think the vertical is reasonable, but the mnu could stretch a bit more. But the horizontal is really annoying.

I can't solve these right now, but if you have any idea I'd be happy to include some additional styling fixes here.

[cervengoc]

@gkatsev Would you please have a look at current state when you have time? This would be important for us to have without our current external fixing hacks.

[gkatsev]

@OwenEdwards unless you disagree, I'd like to get this PR in shape to merge in and then we can fix the "selectable" issue afterwards?

[OwenEdwards]

@gkatsev okay, I guess just opening a separate issue on splitting "selectable" and "selected" to clarify this works. So LGTM.

[gkatsev]

A couple of small questions.
Also, would you be able to rebase this against master? Otherwise, I can probably figure it out myself when merging.

I'll also open a new issue to track whether chapters menu items should be "selected"

[gkatsev]

can we add a comment here that says that for this if, this._track_ is still the old value?

[cervengoc]

Done

[gkatsev]

and a comment here saying that this.track_ is now the new track we want?

[cervengoc]

Done

[gkatsev]

what about !this.track_ instead of this.track_ === undefined?

[cervengoc]

I reviewed it and it sounds reasonable, I've changed it.

[gkatsev]

@cervengoc also, thanks for this PR and sorry it's taken so long to get it pushed through.

[gkatsev]

@OwenEdwards I made #3663

[gkatsev]

@cervengoc hey, would you be able to clean up this PR? I tried doing it locally and have the patch below but not sure if I messed something up and there's something missing.

diff --git a/src/css/components/menu/_menu.scss b/src/css/components/menu/_menu.scss
index 8ecd5ac..bb6496f 100644
--- a/src/css/components/menu/_menu.scss
+++ b/src/css/components/menu/_menu.scss
@@ -35,8 +35,8 @@
   text-transform: lowercase;
 }

-.vjs-menu li:focus,
-.vjs-menu li:hover {
+.vjs-menu li.vjs-menu-item:focus,
+.vjs-menu li.vjs-menu-item:hover {
   outline: 0;
   @include background-color-with-alpha($secondary-background-color, $secondary-background-transparency);
 }
diff --git a/src/js/control-bar/text-track-controls/chapters-button.js b/src/js/control-bar/text-track-controls/chapters-button.js
index 004be74..a6e1b4d 100644
--- a/src/js/control-bar/text-track-controls/chapters-button.js
+++ b/src/js/control-bar/text-track-controls/chapters-button.js
@@ -3,10 +3,7 @@
  */
 import TextTrackButton from './text-track-button.js';
 import Component from '../../component.js';
-import TextTrackMenuItem from './text-track-menu-item.js';
 import ChaptersTrackMenuItem from './chapters-track-menu-item.js';
-import Menu from '../../menu/menu.js';
-import * as Dom from '../../utils/dom.js';
 import toTitleCase from '../../utils/to-title-case.js';

 /**
@@ -37,108 +34,105 @@ class ChaptersButton extends TextTrackButton {
     return `vjs-chapters-button ${super.buildCSSClass()}`;
   }

-  /**
-   * Create a menu item for each text track
-   *
-   * @return {Array} Array of menu items
-   * @method createItems
-   */
-  createItems() {
-    const items = [];
-    const tracks = this.player_.textTracks();
+  update(event) {
+    if (!this.track_ || (event && (event.type === 'addtrack' || event.type === 'removetrack'))) {
+      this.setTrack(this.findChaptersTrack());
+    }
+    super.update();
+  }

-    if (!tracks) {
-      return items;
+  setTrack(track) {
+    if (this.track_ === track) {
+      return;
     }

-    for (let i = 0; i < tracks.length; i++) {
-      const track = tracks[i];
+    if (!this.updateHandler_) {
+      this.updateHandler_ = this.update.bind(this);
+    }

-      if (track.kind === this.kind_) {
-        items.push(new TextTrackMenuItem(this.player_, {track}));
+    // here this.track_ refers to the old track instance
+    if (this.track_) {
+      const remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(this.track_);
+
+      if (remoteTextTrackEl) {
+        remoteTextTrackEl.removeEventListener('load', this.updateHandler_);
       }
+
+      this.track_ = null;
     }

-    return items;
+    this.track_ = track;
+
+    // here this.track_ refers to the new track instance
+    if (this.track_) {
+      this.track_.mode = 'hidden';
+
+      const remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(this.track_);
+
+      if (remoteTextTrackEl) {
+        remoteTextTrackEl.addEventListener('load', this.updateHandler_);
+      }
+    }
   }

-  /**
-   * Create menu from chapter buttons
-   *
-   * @return {Menu} Menu of chapter buttons
-   * @method createMenu
-   */
-  createMenu() {
+  findChaptersTrack() {
     const tracks = this.player_.textTracks() || [];
-    let chaptersTrack;
-    let items = this.items || [];

     for (let i = tracks.length - 1; i >= 0; i--) {
-
       // We will always choose the last track as our chaptersTrack
       const track = tracks[i];

       if (track.kind === this.kind_) {
-        chaptersTrack = track;
-
-        break;
+        return track;
       }
     }
+  }

-    let menu = this.menu;
-
-    if (menu === undefined) {
-      menu = new Menu(this.player_);
-
-      const title = Dom.createEl('li', {
-        className: 'vjs-menu-title',
-        innerHTML: toTitleCase(this.kind_),
-        tabIndex: -1
-      });
-
-      menu.children_.unshift(title);
-      Dom.insertElFirst(title, menu.contentEl());
-    } else {
-      // We will empty out the menu children each time because we want a
-      // fresh new menu child list each time
-      items.forEach(item => menu.removeChild(item));
-      // Empty out the ChaptersButton menu items because we no longer need them
-      items = [];
+  getMenuCaption() {
+    if (this.track_ && this.track_.label) {
+      return this.track_.label;
     }
+    return this.localize(toTitleCase(this.kind_));
+  }

-    if (chaptersTrack && (chaptersTrack.cues === null || chaptersTrack.cues === undefined)) {
-      chaptersTrack.mode = 'hidden';
+  /**
+   * Create menu from chapter track
+   *
+   * @return {Menu} Menu of chapter buttons
+   * @method createMenu
+   */
+  createMenu() {
+    this.options_.title = this.getMenuCaption();
+    return super.createMenu();
+  }

-      const remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(chaptersTrack);
+  /**
+   * Create a menu item for each chapter cue
+   *
+   * @return {Array} Array of menu items
+   * @method createItems
+   */
+  createItems() {
+    const items = [];

-      if (remoteTextTrackEl) {
-        remoteTextTrackEl.addEventListener('load', (event) => this.update());
-      }
+    if (!this.track_) {
+      return items;
     }

-    if (chaptersTrack && chaptersTrack.cues && chaptersTrack.cues.length > 0) {
-      const cues = chaptersTrack.cues;
-
-      for (let i = 0, l = cues.length; i < l; i++) {
-        const cue = cues[i];
+    const cues = this.track_.cues;

-        const mi = new ChaptersTrackMenuItem(this.player_, {
-          cue,
-          track: chaptersTrack
-        });
+    if (!cues) {
+      return items;
+    }

-        items.push(mi);
+    for (let i = 0, l = cues.length; i < l; i++) {
+      const cue = cues[i];
+      const mi = new ChaptersTrackMenuItem(this.player_, { track: this.track_, cue });

-        menu.addChild(mi);
-      }
+      items.push(mi);
     }

-    if (items.length > 0) {
-      this.show();
-    }
-    // Assigning the value of items back to this.items for next iteration
-    this.items = items;
-    return menu;
+    return items;
   }
 }

diff --git a/src/js/control-bar/text-track-controls/chapters-track-menu-item.js b/src/js/control-bar/text-track-controls/chapters-track-menu-item.js
index 2291da4..b2b509f 100644
--- a/src/js/control-bar/text-track-controls/chapters-track-menu-item.js
+++ b/src/js/control-bar/text-track-controls/chapters-track-menu-item.js
@@ -21,6 +21,7 @@ class ChaptersTrackMenuItem extends MenuItem {
     const currentTime = player.currentTime();

     // Modify options for parent MenuItem class's init.
+    options.selectable = true;
     options.label = cue.text;
     options.selected = (cue.startTime <= currentTime && currentTime < cue.endTime);
     super(player, options);
diff --git a/test/unit/test-helpers.js b/test/unit/test-helpers.js
index 442de71..65429c7 100644
--- a/test/unit/test-helpers.js
+++ b/test/unit/test-helpers.js
@@ -130,6 +130,32 @@ const TestHelpers = {
       props.length;

     return run;
+  },
+
+  /**
+   * Triggers an event on a DOM node natively.
+   *
+   * @param  {Element} element
+   * @param  {string} eventType
+   */
+  triggerDomEvent(element, eventType) {
+    let event;
+
+    if (document.createEvent) {
+      event = document.createEvent('HTMLEvents');
+      event.initEvent(eventType, true, true);
+    } else {
+      event = document.createEventObject();
+      event.eventType = eventType;
+    }
+
+    event.eventName = eventType;
+
+    if (document.createEvent) {
+      element.dispatchEvent(event);
+    } else {
+      element.fireEvent('on' + event.eventType, event);
+    }
   }
 };

diff --git a/test/unit/tracks/text-track-controls.test.js b/test/unit/tracks/text-track-controls.test.js
index 569e80e..526694a 100644
--- a/test/unit/tracks/text-track-controls.test.js
+++ b/test/unit/tracks/text-track-controls.test.js
@@ -294,3 +294,154 @@ if (!browser.IS_IE8) {
     player.dispose();
   });
 }
+
+const chaptersTrack = {
+  kind: 'chapters',
+  label: 'Test Chapters'
+};
+
+test('chapters should not be displayed when text tracks list is empty', function() {
+  const player = TestHelpers.makePlayer();
+
+  ok(player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'control is not displayed');
+  equal(player.textTracks().length, 0, 'textTracks is empty');
+
+  player.dispose();
+});
+
+test('chapters should not be displayed when there is chapters track but no cues', function() {
+  const player = TestHelpers.makePlayer({
+    tracks: [chaptersTrack]
+  });
+
+  this.clock.tick(1000);
+
+  ok(player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is not displayed');
+  equal(player.textTracks().length, 1, 'textTracks contains one item');
+
+  player.dispose();
+});
+
+test('chapters should be displayed when cues added to initial track and button updated', function() {
+  const player = TestHelpers.makePlayer({
+    tracks: [chaptersTrack]
+  });
+
+  this.clock.tick(1000);
+
+  const chapters = player.textTracks()[0];
+
+  chapters.addCue({
+    startTime: 0,
+    endTime: 2,
+    text: 'Chapter 1'
+  });
+  chapters.addCue({
+    startTime: 2,
+    endTime: 4,
+    text: 'Chapter 2'
+  });
+  equal(chapters.cues.length, 2);
+
+  player.controlBar.chaptersButton.update();
+
+  ok(!player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is displayed');
+
+  const menuItems = player.controlBar.chaptersButton.items;
+
+  equal(menuItems.length, 2, 'menu contains two item');
+
+  player.dispose();
+});
+
+test('chapters should be displayed when a track and its cures added and button updated', function() {
+  const player = TestHelpers.makePlayer();
+
+  this.clock.tick(1000);
+
+  const chapters = player.addTextTrack('chapters', 'Test Chapters', 'en');
+
+  chapters.addCue({
+    startTime: 0,
+    endTime: 2,
+    text: 'Chapter 1'
+  });
+  chapters.addCue({
+    startTime: 2,
+    endTime: 4,
+    text: 'Chapter 2'
+  });
+  equal(chapters.cues.length, 2);
+
+  player.controlBar.chaptersButton.update();
+
+  ok(!player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is displayed');
+
+  const menuItems = player.controlBar.chaptersButton.items;
+
+  equal(menuItems.length, 2, 'menu contains two item');
+
+  player.dispose();
+});
+
+test('chapters menu should use track label as menu title', function() {
+  const player = TestHelpers.makePlayer({
+    tracks: [chaptersTrack]
+  });
+
+  this.clock.tick(1000);
+
+  const chapters = player.textTracks()[0];
+
+  chapters.addCue({
+    startTime: 0,
+    endTime: 2,
+    text: 'Chapter 1'
+  });
+  chapters.addCue({
+    startTime: 2,
+    endTime: 4,
+    text: 'Chapter 2'
+  });
+  equal(chapters.cues.length, 2);
+
+  player.controlBar.chaptersButton.update();
+
+  const menu = player.controlBar.chaptersButton.menu;
+  const menuTitle = menu.contentEl().firstChild.textContent;
+
+  equal(menuTitle, 'Test Chapters', 'menu gets track label as title');
+
+  player.dispose();
+});
+
+test('chapters should be displayed when remote track added and load event fired', function() {
+  const player = TestHelpers.makePlayer();
+
+  this.clock.tick(1000);
+
+  const chaptersEl = player.addRemoteTextTrack(chaptersTrack);
+
+  chaptersEl.track.addCue({
+    startTime: 0,
+    endTime: 2,
+    text: 'Chapter 1'
+  });
+  chaptersEl.track.addCue({
+    startTime: 2,
+    endTime: 4,
+    text: 'Chapter 2'
+  });
+
+  equal(chaptersEl.track.cues.length, 2);
+
+  TestHelpers.triggerDomEvent(chaptersEl, 'load');
+
+  ok(!player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is displayed');
+
+  const menuItems = player.controlBar.chaptersButton.items;
+
+  equal(menuItems.length, 2, 'menu contains two item');
+
+  player.dispose();
+});

[cervengoc]

I'll have a look next Monday

[gkatsev]

Thanks @cervengoc. We might not be able to get this into 5.13, but we'll juts do a 5.14 if necessary.

[gkatsev]

@cervengoc hey, any updates? Thanks!

[cervengoc]

@gkatsev sorry for the delay, had a lot of other work to do these days. I merged master, hope I didn't messed up anything.

By the way, how is it that these changes could not be simply automerged? I had to resolve 6-8 conflicts which were actually not conflicts at all, just simple additions to files, etc.? I'm still a bit unfamiliar with git, and use it only for these kind of free-time contributions, so for me this merging is a 1-hour nightmare :)

[gkatsev]

Yeah, git is sometimes a bit stupid. If both changes ended up working on the same line, it just gives up even if the only thing that changed is that the line moves down a bit.

Looks like a package.json change snuck in, but otherwise looks good.

[gkatsev]

we don't want this change.

[cervengoc]

Sorry about that, it was about midnight. Fixed.

[gkatsev]

No worries. Thanks for the help!