Only select TextTrackMenuItem if unselected #4920
Conversation
| break; | ||
| } | ||
| } | ||
|
|
||
| this.selected(selected); | ||
| if (shouldBeSelected) { |
There was a problem hiding this comment.
this and and in text-text-menu-item, the two ifs can be collapsed into shouldBeSelected && !this.iSelected_
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, right, makes sense. What about moving the check into the this.selected method?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Nah, don't really have much preference one way or the other. @videojs/core-committers do you have any thoughts?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That sounds good to me
There was a problem hiding this comment.
Or even this:
if (shouldBeSelected !== this.isSelected_) {
this.selected(shouldBeSelected);
}
|
|
||
| // CaptionSettingsMenuItem does not need to handle track list change events because | ||
| // it has no concept of 'selected' | ||
| handleTracksChange(event) { |
There was a problem hiding this comment.
this hopefully shouldn't be necessary because we're setting selectable to false and selected already does a check for it.
There was a problem hiding this comment.
Ah yep. Overlooked the fact that the selectable check makes it a non-issue
gkatsev
added
patch
|
@ldayananda Made those changes. Much cleaner this way, thanks. @gkatsev Want to take another look? |
src/js/menu/menu-item.js
Outdated
| @@ -26,8 +26,9 @@ class MenuItem extends ClickableComponent { | |||
| super(player, options); | |||
|
|
|||
| this.selectable = options.selectable; | |||
| this.isSelected_ = options.selected; | |||
There was a problem hiding this comment.
looks like we need a default value here.
|
Just tested it as well |
Description
These changes address an issue where screen readers may repeatedly and redundantly read
TextTrackMenuItem's control text on everytexttrackchangeevent in some browsers.The source of the problem is in the
handleTracksChange()method ofTextTrackMenuItemand its subclassOffTextTrackMenuItem, in whichthis.selected(true/false)gets called even if the selected state has not changed since its previous invocation.Specific Changes proposed
isSelected_property for menu items.selected(true)inhandleTracksChange()if!isSelected_