fix(MenuButton) Implement unified method of showing/hiding MenuButton #3993

justinanastos · 2017-01-27T23:00:17Z

Description

When remote subtitle text tracks are removed and there are none left, the subtitle button button persists in the menu bar. This also applies to the descriptions button as seen in #3994.

Reduced test case created by @mctep: http://jsbin.com/navoruk/1/edit?html,js,output

Addresses #3890, #3994

Specific Changes proposed

Implement a getHideThreshold function in menu-button and override it in captions-button.
Remove menu-button descendants' update methods

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

OwenEdwards · 2017-01-31T23:55:53Z

src/js/menu/menu-button.js

@@ -58,7 +59,7 @@ class MenuButton extends ClickableComponent {
    this.buttonPressed_ = false;
    this.el_.setAttribute('aria-expanded', 'false');

-    if (this.items && this.items.length === 0) {
+    if (this.items && (this.items.length === 0 || (this.items.length === 1 && this.items[0] instanceof OffTextTrackMenuItem))) {


I don't this should be handled in the MenuButton component; it should be in the sub-component CaptionsButton, using the update method which explicitly understands the different threshold if the OffTextTrackMenuItem is included.

Oh, my mistake - I was thinking of the CaptionsSettings menu item, not the OffTextTrackMenuItem.

justinanastos · 2017-02-08T18:09:31Z

@OwenEdwards Any more feedback?

OwenEdwards · 2017-02-17T23:02:03Z

@justinanastos I'm still confused why this happens on the subtitles menu button, and not on the captions menu button, but you're fixing it on the generic menuButton. Can you give me a little more explanation of why that is?

justinanastos · 2017-02-21T16:41:55Z

@OwenEdwards In trying to explain why I implemented this the way I did, I realized there was a smarter way of handing it.

First, when using emulated text tracks, video.js will add a "captions settings" menu item, which is an instance of OffTextTrackMenuItem:

When removing the caption button, video.js will use a threshold for how many menu items exist for showing or hiding the button in control-bar/text-track-controls/subtitles-button:

/**
 * Update caption menu items
 *
 * @param {EventTarget~Event} [event]
 *        The `addtrack` or `removetrack` event that caused this function to be
 *        called.
 *
 * @listens TextTrackList#addtrack
 * @listens TextTrackList#removetrack
 */
update(event) {
  let threshold = 2;

  super.update();

  // if native, then threshold is 1 because no settings button
  if (this.player().tech_ && this.player().tech_.featuresNativeTextTracks) {
    threshold = 1;
  }
  if (this.items && this.items.length > threshold) {
    this.show();
  } else {
    this.hide();
  }
}

A more appropriate solution would be to add a similar function in control-bar/text-track-controls/subtitles-button

This PR has been updated to add an update function to control-bar/text-track-controls/subtitles-button. A "subtitles settings" menu item is never added for native or emulated subtitles, so the threshold will always be 1.

OwenEdwards · 2017-02-21T23:13:46Z

@justinanastos okay, that seems like a cleaner fix. The problem is, this also affect the Descriptions button (see #3994). If you look at the update method in MenuButton, you'll see that it hides the menu if there are zero menu items, shows the menu if there are more than one items, but doesn't explicitly do anything if there is just one item!

So you need to look at which other menus extend the MenuButton (TrackButton, TextTrackButton, etc.), and see how far up the hierarchy you can push this change so that it affects all the menus which need fixing (e.g. does the AudioTrackButton need fixing?) without you having to put this fix in multiple places. @gkatsev may have some thoughts on that, too.

gkatsev · 2017-02-21T23:20:18Z

In #3890, I was thinking that we should make the threshold a generic thing for text track buttons and maybe track buttons in general. That way, captions button can set its own threshold and subtitles could use the default. And then a value of -1 or something could disable this hiding.

This PR solves the specific issue with subtitles, so, maybe it might be worth pulling it in anyway, but I think we should make it generic that way we don't have the same code in the various track buttons.

gkatsev · 2017-02-21T23:37:05Z

Given that @mister-ben did a similar thing in #4028, I'd be inclined to pull this in, and then making the more general fix later.

OwenEdwards · 2017-02-21T23:39:47Z

Hmm, I'm just concerned that the Descriptions button, which is essentially identical to the subtitles button except with a different track kind and button controlText is going to be the orphaned control if we don't fix the identical issue in both of them by moving this fix up the hierarchy.

gkatsev · 2017-02-21T23:40:58Z

I don't disagree. However, we have a fix for subtitles, so, why not get it fixed?

OwenEdwards · 2017-02-21T23:43:24Z

I guess I'm a fan if "do it right the first time" :-) If we pull this patch, we'll either want to undo it later and apply it higher up the hierarchy to fix #3994, or it'll get copied into DescriptionsButton, and that way lies madness ;-)

OwenEdwards · 2017-02-21T23:52:38Z

@gkatsev I guess another way to look at this is - was it intentional, or is it a bug, that in the update method for the core component MenuButton, it hides if there are no items, shows if there are more than one items, but does nothing if there is exactly one item? If it's a bug, it should be fixed (I'm having a hard time imagining a use case for a menu with just one item, but I guess it's possible), no?

gkatsev · 2017-02-22T00:02:39Z

The default menu button should probably should for one and more and then TextTracks buttons need the threshold fix applied to it as a general solution, which each type of text track specifying if it needs a higher threshold (like captions button sometimes needing a threshold of 2). AudioTrackButton could be shown or not with only one item.

Looks like MenuButton has been around like that for a long time.

justinanastos · 2017-02-22T16:43:55Z

@gkatsev I didn't fully understand what you meant in #3890 until now. This makes total sense and would be pretty simple to implement. Let me give it a shot.

gkatsev · 2017-02-22T16:56:14Z

Cool, that would be awesome, @justinanastos. Let me know if you have any questions, feel free to drop by our slack (http://slack.videojs.com), too.

justinanastos · 2017-02-22T17:01:17Z

@gkatsev I did not implement the -1 behavior because, as far as I can tell, it won't be used anywhere. All menus appear to have a title, so we'll never actually want to show the menu with no content, right?

gkatsev

This looks simple enough, I'll take it for a spin after the change is made.

Thanks @justinanastos!

gkatsev · 2017-02-22T18:52:15Z

src/js/menu/menu-button.js

+   * to 1 beacuse all menus have a title.
+   * @type {Number}
+   */
+  get hideThreshold() {


these should be regular methods rather than getters.

Updated @gkatsev .

OwenEdwards · 2017-02-22T22:56:02Z

src/js/menu/menu-button.js

@@ -37,6 +37,16 @@ class MenuButton extends ClickableComponent {
  }

  /**
+   * Hide the menu if the number of items is less than or equal to this threshold. This defaults
+   * to 1 beacuse all menus have a title.


Not all menus have a title - the Subtitles menu and the Descriptions menu have an "off" button, which should be hidden if it is the only item. <Captain Pedantic ;-) >

Updated @OwenEdwards

OwenEdwards · 2017-02-23T01:23:36Z

src/js/menu/menu-button.js

@@ -37,6 +37,16 @@ class MenuButton extends ClickableComponent {
  }

  /**
+   * Hide the menu if the number of items is less than or equal to this threshold. This defaults


Ultimately I'd love to see each menu component actually set their threshold as they add items to the menu which can be hidden (title, off button, caption settings button, etc), but this looks like it will do for now, and covers both #3890 and #3994.

You're right. Maybe just making it be a private instance variable would be easier in the long run.
At least should be a private method.

@justinanastos thoughts?

That would avoid the multiple uses of if (this.player().tech_ && this.player().tech_.featuresNativeTextTracks) { in this component - just increment the threshold count at the same time it adds the Captions Settings Menu Item.

I'll give this idea a shot.

justinanastos · 2017-02-27T14:10:09Z

@OwenEdwards and @gkatsev : I updated the logic to use a private instance variable that is incremented when menus are added.

gkatsev

Thanks @justinanastos!

Tests are failing, saying some of the menu buttons aren't shown correctly.
Moving the variable init might fix those tests.

gkatsev · 2017-02-27T15:55:35Z

src/js/menu/menu-button.js

+     *
+     * @type {Number}
+     */
+    this.hideThreshold_ = 0;


this should probably move above this.update(). createItems gets called as part of createMenu which is called in update, so, we need to make sure that this threshold is initialized properly.

gkatsev · 2017-02-27T15:58:20Z

src/js/menu/menu-button.js

+     * Hide the menu if the number of items is less than or equal to this threshold. This defaults
+     * to 0 and whenever we add items to the menu we'll increment it.
+     *
+     * @type {Number}


does this need to be marked @private?

Yes it does @gkatsev

Correction @gkatsev : It should be @protected because descendants should be allowed access.

Ah, didn't realize that existed.

Me neither, I googled where to put the @private tag and found it.

OwenEdwards · 2017-02-27T19:01:35Z

This looks much better! Once you've made the changes @gkatsev requested, and fixed the tests, it'll be a really clean solution to the various problems.

OwenEdwards · 2017-02-28T19:21:30Z

src/js/menu/menu-button.js

@@ -29,6 +29,14 @@ class MenuButton extends ClickableComponent {

    this.update();

+    /**
+     * Hide the menu if the number of items is less than or equal to this threshold. This defaults
+     * to 0 and whenever we add items to the menu we'll increment it.


If I'm going to be pedantic, this ought to be "... and whenever we add items which can be hidden to the menu ..." I don't want future additions to the code to increment this when they add non-hideable items!

Implement a `hideThreshold` property that defaults to 1 so descendants can override it if necessary. Right now the only descendant that will override will be `CaptionsButton` because video.js adds a "captions settings" for emulated text tracks.

gkatsev · 2017-03-02T20:24:53Z

@justinanastos can we get a version of this against master as well? If you don't have a chance to do it, let us know and we can port it over for you.

justinanastos · 2017-03-02T20:36:10Z

Sure thing @gkatsev

justinanastos mentioned this pull request Jan 27, 2017

Subtitles Button does not hide after remove subtitles tracks #3890

Closed

OwenEdwards reviewed Jan 31, 2017

View reviewed changes

justinanastos force-pushed the fix-subtitle-button branch from 8a5b8c3 to 2c196da Compare February 21, 2017 16:31

justinanastos changed the base branch from master to 5.x February 21, 2017 16:32

justinanastos force-pushed the fix-subtitle-button branch from 2c196da to 9aec7d0 Compare February 22, 2017 16:59

justinanastos changed the title ~~fix(SubtitlesButton) Hide when the only item is OffTextTrackMenuItem~~ fix(MenuButton) Implement unified method of showing/hiding MenuButton Feb 22, 2017

gkatsev requested changes Feb 22, 2017

View reviewed changes

justinanastos force-pushed the fix-subtitle-button branch from 9aec7d0 to f1dbe72 Compare February 22, 2017 18:58

OwenEdwards reviewed Feb 22, 2017

View reviewed changes

justinanastos force-pushed the fix-subtitle-button branch from f1dbe72 to 585debe Compare February 22, 2017 23:11

OwenEdwards approved these changes Feb 23, 2017

View reviewed changes

justinanastos force-pushed the fix-subtitle-button branch from 585debe to 6f90fa6 Compare February 27, 2017 14:09

gkatsev requested changes Feb 27, 2017

View reviewed changes

gkatsev added the 5.x label Feb 27, 2017

OwenEdwards reviewed Feb 28, 2017

View reviewed changes

justinanastos force-pushed the fix-subtitle-button branch 2 times, most recently from 45f4546 to f84246f Compare February 28, 2017 19:39

justinanastos force-pushed the fix-subtitle-button branch from f84246f to 3c1316f Compare February 28, 2017 19:40

gkatsev approved these changes Feb 28, 2017

View reviewed changes

gkatsev merged commit 4367c69 into videojs:5.x Mar 2, 2017

justinanastos mentioned this pull request Mar 2, 2017

fix(MenuButton) Unify behavior of showing/hiding #4157

Merged

fix(MenuButton) Implement unified method of showing/hiding MenuButton #3993

fix(MenuButton) Implement unified method of showing/hiding MenuButton #3993

Conversation

justinanastos commented Jan 27, 2017 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

OwenEdwards Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos commented Feb 8, 2017

OwenEdwards commented Feb 17, 2017

justinanastos commented Feb 21, 2017

OwenEdwards commented Feb 21, 2017

gkatsev commented Feb 21, 2017

gkatsev commented Feb 21, 2017

OwenEdwards commented Feb 21, 2017

gkatsev commented Feb 21, 2017

OwenEdwards commented Feb 21, 2017

OwenEdwards commented Feb 21, 2017

gkatsev commented Feb 22, 2017

justinanastos commented Feb 22, 2017

gkatsev commented Feb 22, 2017

justinanastos commented Feb 22, 2017

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos commented Feb 27, 2017

gkatsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinanastos Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

OwenEdwards commented Feb 27, 2017

Choose a reason for hiding this comment

gkatsev commented Mar 2, 2017

justinanastos commented Mar 2, 2017

justinanastos commented Jan 27, 2017 •

edited

Loading

OwenEdwards Jan 31, 2017 •

edited

Loading

justinanastos Feb 28, 2017 •

edited

Loading