Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring chapters button handling and fixing several issues #3472

Merged
merged 13 commits into from
Nov 23, 2016
4 changes: 2 additions & 2 deletions src/css/components/menu/_menu.scss
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
text-transform: lowercase;
}

.vjs-menu li:focus,
.vjs-menu li:hover {
.vjs-menu li.vjs-menu-item:focus,
Copy link
Member

Choose a reason for hiding this comment

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

why increase specificity? Can we drop li in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

.vjs-menu li.vjs-menu-item:hover {
outline: 0;
@include background-color-with-alpha($secondary-background-color, $secondary-background-transparency);
}
Expand Down
138 changes: 61 additions & 77 deletions src/js/control-bar/text-track-controls/chapters-button.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ import window from 'global/window';
*/
class ChaptersButton extends TextTrackButton {

constructor(player, options, ready){
constructor(player, options, ready) {
super(player, options, ready);
this.updateHandler_ = Fn.bind(this, this.update);
this.el_.setAttribute('aria-label','Chapters Menu');
}

Expand All @@ -39,107 +40,90 @@ 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() {
let items = [];
update(event) {
if (this.track_ === undefined || (event && (event.type === 'addtrack' || event.type === 'removetrack'))) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this.setTrack(this.findChaptersTrack());
}
super.update();
}

setTrack(track) {
if (this.track_ === track) return;

if (this.track_) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

let remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(this.track_);
if (remoteTextTrackEl) {
remoteTextTrackEl.removeEventListener('load', this.updateHandler_);
}
this.track_ = null;
}

this.track_ = track;

let tracks = this.player_.textTracks();
if (this.track_) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this.track_['mode'] = 'hidden';

if (!tracks) {
return items;
let remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(this.track_);
if (remoteTextTrackEl) {
remoteTextTrackEl.addEventListener('load', this.updateHandler_);
}
}
}

for (let i = 0; i < tracks.length; i++) {
findChaptersTrack() {
let tracks = this.player_.textTracks() || [];

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

if (track['kind'] === this.kind_) {
items.push(new TextTrackMenuItem(this.player_, {
'track': track
}));
return track;
}
}
}

return items;
getMenuCaption() {
if (this.track_ && this.track_.label)
return this.track_.label;
return this.localize(toTitleCase(this.kind_));
}

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

for (let i = tracks.length - 1; i >= 0; i--) {
this.options_.title = this.getMenuCaption();
return super.createMenu();
}

// We will always choose the last track as our chaptersTrack
let track = tracks[i];
/**
* Create a menu item for each chapter cue
*
* @return {Array} Array of menu items
* @method createItems
*/
createItems() {
Copy link
Member

Choose a reason for hiding this comment

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

sometimes the diffs are so hard to read. I'll do another more thorough code review next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was too big rewrite so diffs don't make too much sense here. Thank you, have a nice weekend.

let items = [];
if (!this.track_) return items;

if (track['kind'] === this.kind_) {
chaptersTrack = track;
let cues = this.track_['cues'], cue;
Copy link
Member

Choose a reason for hiding this comment

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

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.


break;
}
}
for (let i = 0, l = cues.length; i < l; i++) {
cue = cues[i];

let menu = this.menu;
if (menu === undefined) {
menu = new Menu(this.player_);
let title = Dom.createEl('li', {
className: 'vjs-menu-title',
innerHTML: toTitleCase(this.kind_),
tabIndex: -1
let mi = new ChaptersTrackMenuItem(this.player_, {
'track': this.track_,
'cue': cue
});
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 = [];
}

if (chaptersTrack && chaptersTrack.cues == null) {
chaptersTrack['mode'] = 'hidden';

let remoteTextTrackEl = this.player_.remoteTextTrackEls().getTrackElementByTrack_(chaptersTrack);

if (remoteTextTrackEl) {
remoteTextTrackEl.addEventListener('load', (event) => this.update());
}
items.push(mi);
}

if (chaptersTrack && chaptersTrack.cues && chaptersTrack.cues.length > 0) {
let cues = chaptersTrack['cues'], cue;

for (let i = 0, l = cues.length; i < l; i++) {
cue = cues[i];

let mi = new ChaptersTrackMenuItem(this.player_, {
'track': chaptersTrack,
'cue': cue
});

items.push(mi);

menu.addChild(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;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ChaptersTrackMenuItem extends MenuItem {
let 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);
Expand Down
26 changes: 26 additions & 0 deletions test/unit/test-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,32 @@ var TestHelpers = {
props.length;

return run;
},

/**
* Triggers an event on a DOM node natively.
*
* @param {Element} element
* @param {string} eventType
*/
triggerDomEvent: function (element, eventType) {
var 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);
}
}
};

Expand Down
149 changes: 149 additions & 0 deletions test/unit/tracks/text-track-controls.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,3 +262,152 @@ if (!browser.IS_IE8) {
player.dispose();
});
}


let chaptersTrack = {
kind: 'chapters',
label: 'Test Chapters'
};

test('chapters should not be displayed when text tracks list is empty', function() {
let 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() {
let 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() {
let player = TestHelpers.makePlayer({
tracks: [chaptersTrack]
});

this.clock.tick(1000);

var 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();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


ok(!player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is displayed');

var 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() {
let player = TestHelpers.makePlayer();

this.clock.tick(1000);

var 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');

var 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() {
let player = TestHelpers.makePlayer({
tracks: [chaptersTrack]
});

this.clock.tick(1000);

var 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();

var menu = player.controlBar.chaptersButton.menu;
var 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() {
let player = TestHelpers.makePlayer();

this.clock.tick(1000);

var 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');
Copy link
Member

Choose a reason for hiding this comment

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

I think we need another version of this test that uses the emulated text tracks.

let player = TestHelpers.makePlayer({
  techOrder: ['html5'],
  html5: {
    nativeTextTracks: false
  }
});

Then we can just call chaptersEl.trigger('load') here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the setup above the players tech_ property gets undefined somehow.


ok(!player.controlBar.chaptersButton.hasClass('vjs-hidden'), 'chapters menu is displayed');

var menuItems = player.controlBar.chaptersButton.items;

equal(menuItems.length, 2, 'menu contains two item');

player.dispose();
});