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

Only select TextTrackMenuItem if unselected #4920

Merged
merged 10 commits into from
Feb 9, 2018
Merged

Only select TextTrackMenuItem if unselected #4920

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b95387c
wip
alex-barstow Jan 31, 2018
e460dd0
more experimentation, logging
alex-barstow Feb 1, 2018
7250409
remove logging, clean upcomments
alex-barstow Feb 1, 2018
4fcf5eb
remove event arg used for testing
alex-barstow Feb 1, 2018
abf8bf4
add comment
alex-barstow Feb 1, 2018
a626b17
set value of isSelected_ in selected() method
alex-barstow Feb 1, 2018
e5eb6cd
better comment
alex-barstow Feb 1, 2018
8995ad9
remove empty handleTracksChange method from caption settings menu item
alex-barstow Feb 2, 2018
52277b4
simplify conditional, rename variable
alex-barstow Feb 5, 2018
343510a
add default value for isSelected_
alex-barstow Feb 5, 2018
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
14 changes: 11 additions & 3 deletions src/js/control-bar/text-track-controls/off-text-track-menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,26 @@ class OffTextTrackMenuItem extends TextTrackMenuItem {
*/
handleTracksChange(event) {
const tracks = this.player().textTracks();
let selected = true;
let shouldBeSelected = true;

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

if ((this.options_.kinds.indexOf(track.kind) > -1) && track.mode === 'showing') {
selected = false;
shouldBeSelected = false;
break;
}
}

this.selected(selected);
if (shouldBeSelected) {
Copy link
Member

Choose a reason for hiding this comment

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

this and and in text-text-menu-item, the two ifs can be collapsed into shouldBeSelected && !this.iSelected_

Copy link
Contributor Author

@alex-barstow alex-barstow Feb 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand correctly, that would create scenarios where shouldBeSelected is true, isSelected_ is true, yet we would deselect the menu item. In that case where both those values are true, we should just do nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, makes sense. What about moving the check into the this.selected method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I considered it. It was kind of a toss up, but in the end I decided I found it clearer to conditionally call .selected() rather than always call it and conditionally have it do nothing, if that makes sense. I don't have a particularly strong preference, so if you find it clearer to have the logic in the selected method itself I'm happy to move it.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, don't really have much preference one way or the other. @videojs/core-committers do you have any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're making this change, it seems unnecessary to allow redundant selected(false) calls as well. How about shouldBeSelected && shouldBeSelected !== this.isSelected_) for the first if and !shouldBeSelected && shouldBeSelected !== this.isSelected_ for the second?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even this:

if (shouldBeSelected !== this.isSelected_) {
  this.selected(shouldBeSelected);
}

// Prevent redundant selected(true) calls because they may cause
// screen readers to read the appended control text unnecessarily
if (!this.isSelected_) {
this.selected(true);
}
} else {
this.selected(false);
}
}

handleSelectedLanguageChange(event) {
Expand Down
12 changes: 11 additions & 1 deletion src/js/control-bar/text-track-controls/text-track-menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,17 @@ class TextTrackMenuItem extends MenuItem {
* @listens TextTrackList#change
*/
handleTracksChange(event) {
this.selected(this.track.mode === 'showing');
const trackModeIsShowing = this.track.mode === 'showing';

if (trackModeIsShowing) {
// Prevent redundant selected(true) calls because they may cause
// screen readers to read the appended control text unnecessarily
if (!this.isSelected_) {
this.selected(true);
}
} else {
this.selected(false);
}
}

handleSelectedLanguageChange(event) {
Expand Down
5 changes: 4 additions & 1 deletion src/js/menu/menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ class MenuItem extends ClickableComponent {
super(player, options);

this.selectable = options.selectable;
this.isSelected_ = options.selected;
Copy link
Member

Choose a reason for hiding this comment

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

looks like we need a default value here.


this.selected(options.selected);
this.selected(this.isSelected_);

if (this.selectable) {
// TODO: May need to be either menuitemcheckbox or menuitemradio,
Expand Down Expand Up @@ -93,11 +94,13 @@ class MenuItem extends ClickableComponent {
// aria-checked isn't fully supported by browsers/screen readers,
// so indicate selected state to screen reader in the control text.
this.controlText(', selected');
this.isSelected_ = true;
} else {
this.removeClass('vjs-selected');
this.el_.setAttribute('aria-checked', 'false');
// Indicate un-selected state to screen reader
this.controlText('');
this.isSelected_ = false;
}
}
}
Expand Down