Issues with chapters

[cervengoc]

Hello guys, I had problems with chapters. I don't have time for the PR, but here are a list of issues which I've found and fixed locally.

• ChaptersButton listens to load event only when cues == null. In latest Chrome I have an empty cues property, so it seems like several browsers handle it differently. IMO the best would be to always subscribe to the load event and issue an update. Also, it would be needed to unsubscribe from it when player is disposed; when track is removed; etc. As a quick fix I refined the related if condition to !chaptersTrack.cues || !chaptersTrack.cues.length.
• ChaptersMenuItem should be set to selectable, otherwise it won't update it's selected style on cuechange.
• Chapters menu should take the chapters track's label as menu title and fall back to "Chapters" if not provided.
• This is just a refactoring: it seems like ChaptersButton's createItems is incorretct, and not even called (createMenu is used instead), so it should be removed.
• There are several styling isssues with chapters: font is not Arial because it inherits VideoJS font, so menu items and title look ugly; menu items have strange padding, the text is not aligned to the middle vertically.

Please someone handle these in the next release.

[gkatsev]

Thanks for the issue. We'll get around to it when we case. A PR is always greatly appreciated.
Some comments about each item below:

ChaptersButton listens to load event only when cues == null

It probably makes sense to always listen to load. When this was implemented, cues was always null before it was loaded but it's possible that things have changed by now.

ChaptersMenuItem should be set to selectable

Not sure that making the ChaptersMenuItem selectable is correct. Because we don't update update the "selected" item depending on the player's current time, I don't think it really makes sense for them to be selectable. Would the selected item but just the previously clicked item?
There definitely should be some visual cue that you've which item you've clicked on, though.

Chapters menu should take the chapters track's label as menu title and fall back to "Chapters"

That makes sense as an enhancement.

it seems like ChaptersButton's createItems is incorrect

Yup, createMenu probably needs to be broken up and a large portion moved into createItems.

font is not Arial because it inherits VideoJS font

The font-family was recently updated.

menu items have strange padding

Not sure what these means.

text is not aligned to the middle vertically

It looks aligned to me, but very possible it isn't perfect. Vertical alignment is hard in CSS.

[cervengoc]

@gkatsev I'll be able to work on it in the next weeks sometime, but as I've never contributed here yet, it will take time to learn your workflow, testing strategy, etc. That's why I suggested that someone should make this relatively small changes instead of me.

About selectable of chapter menu item. The correct way of handling chapters menu includes the automatic updates of menu items based on player time. And the good news is that it is actually implemented, I don't have time to find the line in the code to reference, but you can search for it easily. So the only missing point in this is that the menu item is not set to selectable, so the updates are basically (visually) ineffective.

[chemoish]

@cervengoc I chose to go with selectable (chemoish/videojs-chapter-thumbnails#20).

Looks like the outstanding PR also includes https://github.com/videojs/video.js/pull/3472/files#diff-9a53c3d0b5b8d51011b2ba56e434ea2cR24.

Have you ran into the issue with https://github.com/videojs/video.js/blob/master/src/js/menu/menu-item.js#L72?

this.controlText(', selected'); and this.controlText(' '); causes tooltips to popup where title attribute is supported.

Putting the controlText into the title attribute was added after I made that change. For menu items the "control text" is really the visible text, and I re-used the hidden, screen-reader accessible controlText for something that is really a "control state". But you're right, with the title change it doesn't really work any more.

• @OwenEdwards

@gkatsev Maybe we should handle? or create another ticket for this? If selectable is the right approach?

[gkatsev]

The PR has finally been merged.