fix(chapters-button): Remove dead code and fix selected capability.
#3623
Conversation
| @@ -124,7 +98,8 @@ class ChaptersButton extends TextTrackButton { | |||
|
|
|||
| const mi = new ChaptersTrackMenuItem(this.player_, { | |||
| cue, | |||
| track: chaptersTrack | |||
| track: chaptersTrack, | |||
| selectable: true | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Perhaps "selectable" wasn't a clear way to describe this, but I don't think that chapters can be "selected". Think of selectable as being whether the item behaves like a checkbox or radio button - would you want it "checked" in the menu? For captions and descriptions, yes, you would. For the captions settings menu, you wouldn't.
Having a chapter "selected" only makes sense if you have a mechanism which updates the "selected" chapter as the video plays; otherwise it would be like leaving a checkmark on the progress bar of where you last clicked, even when playback has moved forward from that point.
There was a problem hiding this comment.
Okay, then we need to discuss the concept of "selected" some more.
I can see that chapter menu items do update as "selected" based on the timeline, but from an ARIA perspective, it's not strictly "selected". Again, think of it as a checkbox - if a checkbox is already checked, and you click on it, you don't expect the checkbox to stay checked. If it's a radio button you do, but then you don't expect it to have another effect.
So this chapters menu is a hybrid control, which is both a user control and a progress display. I understand what @chemoish wants to achieve visually, but it's not doing the correct thing logically by just making chapter items "selectable". (Having said that, using the controlText to indicate selected as text to a screen reader was a hack too! Let's fix this properly before we go too far down the road to fix it at all).
| * @return {Array} Array of menu items | ||
| * @method createItems | ||
| */ | ||
| createItems() { |
There was a problem hiding this comment.
Dead code never called. Item creation was all placed inside createMenu for some reason.
|
Yeah, I think I'm going to close this now as this is available as part of #3472 as well, though, we still want more updates there. Please review that PR if you get the chance. |
Description
Fixes #3033,
selectablewith minimal code change. This doesn't account for any code style or implementation for leveragingChaptersTrackMenuItemor extension ofTextTrackMenuItem.