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.

Sign up for GitHub

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

fix(chapters-button): Remove dead code and fix selected capability. #3623

Closed
wants to merge 1 commit into from
Closed

fix(chapters-button): Remove dead code and fix selected capability. #3623

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 2 additions & 27 deletions src/js/control-bar/text-track-controls/chapters-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
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';
Expand Down Expand Up @@ -37,31 +36,6 @@ 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();

if (!tracks) {
return items;
}

for (let i = 0; i < tracks.length; i++) {
const track = tracks[i];

if (track.kind === this.kind_) {
items.push(new TextTrackMenuItem(this.player_, {track}));
}
}

return items;
}

/**
* Create menu from chapter buttons
*
Expand Down Expand Up @@ -124,7 +98,8 @@ class ChaptersButton extends TextTrackButton {

const mi = new ChaptersTrackMenuItem(this.player_, {
cue,
track: chaptersTrack
track: chaptersTrack,
selectable: true
Copy link
Member Author

Choose a reason for hiding this comment

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

Exact implementation of https://github.com/videojs/video.js/blob/master/src/js/control-bar/text-track-controls/text-track-button.js#L49.

Copy link
Member

@OwenEdwards OwenEdwards Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

They actually are supposed to get updated as mentioned in #3472 and #3447.

Copy link
Member

@OwenEdwards OwenEdwards Sep 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a plan. We should either see if we can get #3472 updated since it has a bunch of other good fixes or merge in #3472 and then fix it forward in a separate PR before releasing.

});

items.push(mi);
Expand Down