Fix MenuButton for the case where multiple listeners are defined.

This is a minimal fix suitable for merging to M38. The underlying problem is that we create a new MenuButton instance (with its own click handler) for every connection. I recently changed the behaviour of MenuButton so that it toggles the 'active' class instead of adding it so that clicking on the button while a menu is open will dismiss it. However, this breaks if there an even number of MenuButton instances for a single DOM element. The correct fix is to make the tool-bar buttons owned by the tool-bar, which is a singleton. However, this minimal fix, which restores the M37 behaviour, is a better option for merging to M38. BUG=415410 Review URL: https://codereview.chromium.org/584693003 Cr-Commit-Position: refs/heads/master@{#295759}

Fix MenuButton for the case where multiple listeners are defined.
This is a minimal fix suitable for merging to M38. The underlying problem is that we create a new MenuButton instance (with its own click handler) for every connection. I recently changed the behaviour of MenuButton so that it toggles the 'active' class instead of adding it so that clicking on the button while a menu is open will dismiss it. However, this breaks if there an even number of MenuButton instances for a single DOM element. The correct fix is to make the tool-bar buttons owned by the tool-bar, which is a singleton. However, this minimal fix, which restores the M37 behaviour, is a better option for merging to M38. BUG=415410 Review URL: https://codereview.chromium.org/584693003 Cr-Commit-Position: refs/heads/master@{#295759}
2405b306 · jamiewalch · Commit bot · b35d075d · 2405b306 · 2405b306
Commit 2405b306 authored Sep 19, 2014 by jamiewalch Committed by Commit bot Sep 19, 2014
Hide whitespace changes
Inline Side-by-side

Showing with 4 additions and 1 deletion

remoting/webapp/menu_button.js remoting/webapp/menu_button.js +1 -1

remoting/webapp/unittests/menu_button_unittest.js remoting/webapp/unittests/menu_button_unittest.js +3 -0

No files found.
--- a/remoting/webapp/menu_button.js
+++ b/remoting/webapp/menu_button.js
@@ -70,7 +70,7 @@ remoting.MenuButton = function(container, opt_onShow, opt_onHide) {
    if (that.onShow_) {
      that.onShow_();
    }
-    that.button_.classList.toggle(remoting.MenuButton.BUTTON_ACTIVE_CLASS_);
+    that.button_.classList.add(remoting.MenuButton.BUTTON_ACTIVE_CLASS_);
    document.body.addEventListener('click', closeHandler, false);
    event.stopPropagation();
  };

--- a/remoting/webapp/unittests/menu_button_unittest.js
+++ b/remoting/webapp/unittests/menu_button_unittest.js
@@ -47,12 +47,15 @@ test('should dismiss when <body> is clicked', function() {
  ok(menu.offsetWidth == 0 && menu.offsetHeight == 0);
 });

+/*
+TODO(jamiewalch): Reinstate this once MenuButton is fixed properly.
 test('should dismiss when button is clicked', function() {
  var menu = menuButton.menu();
  menuButton.button().click();
  menuButton.button().click();
  ok(menu.offsetWidth == 0 && menu.offsetHeight == 0);
 });
+*/

 test('should dismiss when menu item is clicked', function() {
  var menu = menuButton.menu();