From b95387c5a916d52f8f8c5774175b267fb2273c76 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Wed, 31 Jan 2018 14:51:22 -0500 Subject: [PATCH 01/10] wip --- .../text-track-controls/text-track-menu-item.js | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/js/control-bar/text-track-controls/text-track-menu-item.js b/src/js/control-bar/text-track-controls/text-track-menu-item.js index 248973636b..b1f8ff9f50 100644 --- a/src/js/control-bar/text-track-controls/text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/text-track-menu-item.js @@ -129,7 +129,20 @@ class TextTrackMenuItem extends MenuItem { * @listens TextTrackList#change */ handleTracksChange(event) { - this.selected(this.track.mode === 'showing'); + // determine if the menu item *should* be selected at this time + const trackModeIsShowing = this.track.mode === 'showing'; + + // menu item should either be selected, or remain selected if it already was + if (trackModeIsShowing) { + if (!this.isSelected_) { + this.selected(true); + this.isSelected_ = true; + } + // menu item should not be selected + } else { + this.selected(false); + this.isSelected_ = false; + } } handleSelectedLanguageChange(event) { From e460dd0edf972864aaeb9c3130ee2bea3b9f631c Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Thu, 1 Feb 2018 14:12:50 -0500 Subject: [PATCH 02/10] more experimentation, logging --- .../caption-settings-menu-item.js | 4 ++++ .../off-text-track-menu-item.js | 15 ++++++++++++--- .../text-track-controls/text-track-menu-item.js | 4 +++- src/js/menu/menu-item.js | 7 ++++++- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js index 095d8895a7..8c2d4aaed5 100644 --- a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js +++ b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js @@ -54,6 +54,10 @@ class CaptionSettingsMenuItem extends TextTrackMenuItem { handleClick(event) { this.player().getChild('textTrackSettings').open(); } + + handleTracksChange(event) { + + } } Component.registerComponent('CaptionSettingsMenuItem', CaptionSettingsMenuItem); diff --git a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js index 69556b7104..49bded7017 100644 --- a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js @@ -54,19 +54,28 @@ class OffTextTrackMenuItem extends TextTrackMenuItem { * The event that caused this function to run */ handleTracksChange(event) { + // console.log('handleTracksChange() called', event); // eslint-disable-line 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) { + if (!this.isSelected_) { + this.selected(true); + this.isSelected_ = true; + } + } else { + this.selected(false); + this.isSelected_ = false; + } } handleSelectedLanguageChange(event) { diff --git a/src/js/control-bar/text-track-controls/text-track-menu-item.js b/src/js/control-bar/text-track-controls/text-track-menu-item.js index b1f8ff9f50..45f726f231 100644 --- a/src/js/control-bar/text-track-controls/text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/text-track-menu-item.js @@ -33,7 +33,8 @@ class TextTrackMenuItem extends MenuItem { super(player, options); this.track = track; - const changeHandler = (...args) => { + const changeHandler = (event, ...args) => { + // console.log('changeHandler called on:', event) // eslint-disable-line this.handleTracksChange.apply(this, args); }; const selectedLanguageChangeHandler = (...args) => { @@ -129,6 +130,7 @@ class TextTrackMenuItem extends MenuItem { * @listens TextTrackList#change */ handleTracksChange(event) { + console.log('handleTracksChange() called with track:', this.track); // eslint-disable-line // determine if the menu item *should* be selected at this time const trackModeIsShowing = this.track.mode === 'showing'; diff --git a/src/js/menu/menu-item.js b/src/js/menu/menu-item.js index 21c873665c..b24228f8a8 100644 --- a/src/js/menu/menu-item.js +++ b/src/js/menu/menu-item.js @@ -29,6 +29,9 @@ class MenuItem extends ClickableComponent { this.selected(options.selected); + // I ADDED THIS + this.isSelected_ = options.selected; + if (this.selectable) { // TODO: May need to be either menuitemcheckbox or menuitemradio, // and may need logical grouping of menu items. @@ -77,6 +80,7 @@ class MenuItem extends ClickableComponent { */ handleClick(event) { this.selected(true); + this.isSelected_ = true; } /** @@ -86,13 +90,14 @@ class MenuItem extends ClickableComponent { * if the menu item is selected or not */ selected(selected) { + console.log(`selected(${selected}) called on`, this); // eslint-disable-line if (this.selectable) { if (selected) { this.addClass('vjs-selected'); this.el_.setAttribute('aria-checked', 'true'); // 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.controlText('alex'); } else { this.removeClass('vjs-selected'); this.el_.setAttribute('aria-checked', 'false'); From 72504094f8ed318997fc883afcf3d47ca20269e3 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Thu, 1 Feb 2018 14:46:56 -0500 Subject: [PATCH 03/10] remove logging, clean upcomments --- .../text-track-controls/off-text-track-menu-item.js | 3 ++- .../text-track-controls/text-track-menu-item.js | 7 ++----- src/js/menu/menu-item.js | 9 +++------ 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js index 49bded7017..4ce2414f5e 100644 --- a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js @@ -54,7 +54,6 @@ class OffTextTrackMenuItem extends TextTrackMenuItem { * The event that caused this function to run */ handleTracksChange(event) { - // console.log('handleTracksChange() called', event); // eslint-disable-line const tracks = this.player().textTracks(); let shouldBeSelected = true; @@ -68,6 +67,8 @@ class OffTextTrackMenuItem extends TextTrackMenuItem { } if (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); this.isSelected_ = true; diff --git a/src/js/control-bar/text-track-controls/text-track-menu-item.js b/src/js/control-bar/text-track-controls/text-track-menu-item.js index 45f726f231..812e4b04e8 100644 --- a/src/js/control-bar/text-track-controls/text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/text-track-menu-item.js @@ -34,7 +34,6 @@ class TextTrackMenuItem extends MenuItem { this.track = track; const changeHandler = (event, ...args) => { - // console.log('changeHandler called on:', event) // eslint-disable-line this.handleTracksChange.apply(this, args); }; const selectedLanguageChangeHandler = (...args) => { @@ -130,17 +129,15 @@ class TextTrackMenuItem extends MenuItem { * @listens TextTrackList#change */ handleTracksChange(event) { - console.log('handleTracksChange() called with track:', this.track); // eslint-disable-line - // determine if the menu item *should* be selected at this time const trackModeIsShowing = this.track.mode === 'showing'; - // menu item should either be selected, or remain selected if it already was 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); this.isSelected_ = true; } - // menu item should not be selected } else { this.selected(false); this.isSelected_ = false; diff --git a/src/js/menu/menu-item.js b/src/js/menu/menu-item.js index b24228f8a8..0835d34f12 100644 --- a/src/js/menu/menu-item.js +++ b/src/js/menu/menu-item.js @@ -26,12 +26,10 @@ class MenuItem extends ClickableComponent { super(player, options); this.selectable = options.selectable; - - this.selected(options.selected); - - // I ADDED THIS this.isSelected_ = options.selected; + this.selected(this.isSelected_); + if (this.selectable) { // TODO: May need to be either menuitemcheckbox or menuitemradio, // and may need logical grouping of menu items. @@ -90,14 +88,13 @@ class MenuItem extends ClickableComponent { * if the menu item is selected or not */ selected(selected) { - console.log(`selected(${selected}) called on`, this); // eslint-disable-line if (this.selectable) { if (selected) { this.addClass('vjs-selected'); this.el_.setAttribute('aria-checked', 'true'); // aria-checked isn't fully supported by browsers/screen readers, // so indicate selected state to screen reader in the control text. - this.controlText('alex'); + this.controlText(', selected'); } else { this.removeClass('vjs-selected'); this.el_.setAttribute('aria-checked', 'false'); From 4fcf5eb79d127a4cea5f9c54f365fa1b69ee3ec9 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Thu, 1 Feb 2018 14:57:32 -0500 Subject: [PATCH 04/10] remove event arg used for testing --- src/js/control-bar/text-track-controls/text-track-menu-item.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/control-bar/text-track-controls/text-track-menu-item.js b/src/js/control-bar/text-track-controls/text-track-menu-item.js index 812e4b04e8..ef31432a3a 100644 --- a/src/js/control-bar/text-track-controls/text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/text-track-menu-item.js @@ -33,7 +33,7 @@ class TextTrackMenuItem extends MenuItem { super(player, options); this.track = track; - const changeHandler = (event, ...args) => { + const changeHandler = (...args) => { this.handleTracksChange.apply(this, args); }; const selectedLanguageChangeHandler = (...args) => { From abf8bf4d629b2e82f7921d0c65b649006d66ac80 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Thu, 1 Feb 2018 15:08:55 -0500 Subject: [PATCH 05/10] add comment --- .../text-track-controls/caption-settings-menu-item.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js index 8c2d4aaed5..84dd5296e5 100644 --- a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js +++ b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js @@ -55,6 +55,7 @@ class CaptionSettingsMenuItem extends TextTrackMenuItem { this.player().getChild('textTrackSettings').open(); } + // No operation is needed following text track list changes handleTracksChange(event) { } From a626b178d7d364cf40c2f9f707740d4f851cf6d5 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Thu, 1 Feb 2018 17:14:52 -0500 Subject: [PATCH 06/10] set value of isSelected_ in selected() method --- .../text-track-controls/off-text-track-menu-item.js | 2 -- src/js/control-bar/text-track-controls/text-track-menu-item.js | 2 -- src/js/menu/menu-item.js | 3 ++- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js index 4ce2414f5e..8284ee7135 100644 --- a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js @@ -71,11 +71,9 @@ class OffTextTrackMenuItem extends TextTrackMenuItem { // screen readers to read the appended control text unnecessarily if (!this.isSelected_) { this.selected(true); - this.isSelected_ = true; } } else { this.selected(false); - this.isSelected_ = false; } } diff --git a/src/js/control-bar/text-track-controls/text-track-menu-item.js b/src/js/control-bar/text-track-controls/text-track-menu-item.js index ef31432a3a..3475401a9f 100644 --- a/src/js/control-bar/text-track-controls/text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/text-track-menu-item.js @@ -136,11 +136,9 @@ class TextTrackMenuItem extends MenuItem { // screen readers to read the appended control text unnecessarily if (!this.isSelected_) { this.selected(true); - this.isSelected_ = true; } } else { this.selected(false); - this.isSelected_ = false; } } diff --git a/src/js/menu/menu-item.js b/src/js/menu/menu-item.js index 0835d34f12..d09e3a614d 100644 --- a/src/js/menu/menu-item.js +++ b/src/js/menu/menu-item.js @@ -78,7 +78,6 @@ class MenuItem extends ClickableComponent { */ handleClick(event) { this.selected(true); - this.isSelected_ = true; } /** @@ -95,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; } } } From e5eb6cd7a9257cf270033a6546bc1993812c627b Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Thu, 1 Feb 2018 17:24:01 -0500 Subject: [PATCH 07/10] better comment --- .../text-track-controls/caption-settings-menu-item.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js index 84dd5296e5..b9d716c94b 100644 --- a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js +++ b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js @@ -55,7 +55,8 @@ class CaptionSettingsMenuItem extends TextTrackMenuItem { this.player().getChild('textTrackSettings').open(); } - // No operation is needed following text track list changes + // CaptionSettingsMenuItem does not need to handle track list change events because + // it has no concept of 'selected' handleTracksChange(event) { } From 8995ad9924340e3f176e0094f4f10b8fb62112a2 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Fri, 2 Feb 2018 11:34:09 -0500 Subject: [PATCH 08/10] remove empty handleTracksChange method from caption settings menu item --- .../text-track-controls/caption-settings-menu-item.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js index b9d716c94b..095d8895a7 100644 --- a/src/js/control-bar/text-track-controls/caption-settings-menu-item.js +++ b/src/js/control-bar/text-track-controls/caption-settings-menu-item.js @@ -54,12 +54,6 @@ class CaptionSettingsMenuItem extends TextTrackMenuItem { handleClick(event) { this.player().getChild('textTrackSettings').open(); } - - // CaptionSettingsMenuItem does not need to handle track list change events because - // it has no concept of 'selected' - handleTracksChange(event) { - - } } Component.registerComponent('CaptionSettingsMenuItem', CaptionSettingsMenuItem); From 52277b4d87210fb9a33bc934580c01fe0abb8e01 Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Mon, 5 Feb 2018 11:28:22 -0500 Subject: [PATCH 09/10] simplify conditional, rename variable --- .../off-text-track-menu-item.js | 12 ++++-------- .../text-track-controls/text-track-menu-item.js | 14 +++++--------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js index 8284ee7135..175db373ef 100644 --- a/src/js/control-bar/text-track-controls/off-text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/off-text-track-menu-item.js @@ -66,14 +66,10 @@ class OffTextTrackMenuItem extends TextTrackMenuItem { } } - if (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); + // Prevent redundant selected() calls because they may cause + // screen readers to read the appended control text unnecessarily + if (shouldBeSelected !== this.isSelected_) { + this.selected(shouldBeSelected); } } diff --git a/src/js/control-bar/text-track-controls/text-track-menu-item.js b/src/js/control-bar/text-track-controls/text-track-menu-item.js index 3475401a9f..3775e29d03 100644 --- a/src/js/control-bar/text-track-controls/text-track-menu-item.js +++ b/src/js/control-bar/text-track-controls/text-track-menu-item.js @@ -129,16 +129,12 @@ class TextTrackMenuItem extends MenuItem { * @listens TextTrackList#change */ handleTracksChange(event) { - const trackModeIsShowing = this.track.mode === 'showing'; + const shouldBeSelected = 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); + // Prevent redundant selected() calls because they may cause + // screen readers to read the appended control text unnecessarily + if (shouldBeSelected !== this.isSelected_) { + this.selected(shouldBeSelected); } } From 343510aeb40abfc5c5180e8c6131abd23befb30c Mon Sep 17 00:00:00 2001 From: Alex Barstow Date: Mon, 5 Feb 2018 12:22:28 -0500 Subject: [PATCH 10/10] add default value for isSelected_ --- src/js/menu/menu-item.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/menu/menu-item.js b/src/js/menu/menu-item.js index d09e3a614d..c89e88b636 100644 --- a/src/js/menu/menu-item.js +++ b/src/js/menu/menu-item.js @@ -26,7 +26,7 @@ class MenuItem extends ClickableComponent { super(player, options); this.selectable = options.selectable; - this.isSelected_ = options.selected; + this.isSelected_ = options.selected || false; this.selected(this.isSelected_);