Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu. r=mak
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Wed, 11 Apr 2018 12:42:18 +0100
changeset 413058 91cfe8e76dd18e360ef9e79166abac11c94e3f9e
parent 413057 4d8757b2fc9e75b536b20fe3d2726b3a4093f655
child 413059 da809ecceaf3a8ada0aa2d7115822d39d0439654
child 413122 dce413699c31023dd641d3438aadd55df81cb864
push id33832
push userrgurzau@mozilla.com
push dateThu, 12 Apr 2018 23:09:18 +0000
treeherdermozilla-central@da809ecceaf3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1391948
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu. r=mak MozReview-Commit-ID: 2P3yV6xHLxi
browser/base/content/browser-places.js
browser/base/content/browser.js
browser/base/content/browser.xul
browser/components/customizableui/content/panelUI.inc.xul
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -1284,27 +1284,16 @@ var BookmarkingUI = {
   _getFormattedTooltip(strId) {
     let args = [];
     let shortcut = document.getElementById(this.BOOKMARK_BUTTON_SHORTCUT);
     if (shortcut)
       args.push(ShortcutUtils.prettifyShortcut(shortcut));
     return gNavigatorBundle.getFormattedString(strId, args);
   },
 
-  /**
-   * The popup contents must be updated when the user customizes the UI, or
-   * changes the personal toolbar collapsed status.  In such a case, any needed
-   * change should be handled in the popupshowing helper, for performance
-   * reasons.
-   */
-  _popupNeedsUpdate: true,
-  onToolbarVisibilityChange: function BUI_onToolbarVisibilityChange() {
-    this._popupNeedsUpdate = true;
-  },
-
   onPopupShowing: function BUI_onPopupShowing(event) {
     // Don't handle events for submenus.
     if (event.target != event.currentTarget)
       return;
 
     // On non-photon, this code should never be reached. However, if you click
     // the outer button's border, some cpp code for the menu button's XBL
     // binding decides to open the popup even though the dropmarker is invisible.
@@ -1326,33 +1315,31 @@ var BookmarkingUI = {
       event.preventDefault();
       widget.node.removeAttribute("closemenu");
       PlacesCommandHook.showPlacesOrganizer("BookmarksMenu");
       return;
     }
 
     this._initMobileBookmarks(document.getElementById("BMB_mobileBookmarks"));
 
-    if (!this._popupNeedsUpdate)
-      return;
-    this._popupNeedsUpdate = false;
+    this.selectLabel("BMB_viewBookmarksSidebar",
+                     SidebarUI.currentID == "viewBookmarksSidebar");
+    this.selectLabel("BMB_viewBookmarksToolbar",
+                     !document.getElementById("PersonalToolbar").collapsed);
+  },
 
-    let popup = event.target;
-    let getPlacesAnonymousElement =
-      aAnonId => document.getAnonymousElementByAttribute(popup.parentNode,
-                                                         "placesanonid",
-                                                         aAnonId);
+  selectLabel(elementId, visible) {
+    let element = document.getElementById(elementId);
+    element.setAttribute("label", element.getAttribute(visible ? "label-hide"
+                                                               : "label-show"));
+  },
 
-    let viewToolbarMenuitem = getPlacesAnonymousElement("view-toolbar");
-    if (viewToolbarMenuitem) {
-      // Update View bookmarks toolbar checkbox menuitem.
-      viewToolbarMenuitem.classList.add("subviewbutton");
-      let personalToolbar = document.getElementById("PersonalToolbar");
-      viewToolbarMenuitem.setAttribute("checked", !personalToolbar.collapsed);
-    }
+  toggleBookmarksToolbar() {
+    CustomizableUI.setToolbarVisibility("PersonalToolbar",
+      document.getElementById("PersonalToolbar").collapsed);
   },
 
   attachPlacesView(event, node) {
     // If the view is already there, bail out early.
     if (node.parentNode._placesView)
       return;
 
     new PlacesMenu(event, "place:folder=BOOKMARKS_MENU", {
@@ -1435,17 +1422,16 @@ var BookmarkingUI = {
     if (!this._isCustomizing) {
       this._uninitView();
     }
   },
 
   onCustomizeEnd: function BUI_customizeEnd(aWindow) {
     if (aWindow == window) {
       this._isCustomizing = false;
-      this.onToolbarVisibilityChange();
     }
   },
 
   init() {
     CustomizableUI.addListener(this);
 
     if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
       let starButtonBox = document.getElementById("star-button-box");
@@ -1646,50 +1632,24 @@ var BookmarkingUI = {
 
   onPanelMenuViewHiding: function BUI_onViewHiding(aEvent) {
     this._panelMenuView.uninit();
     delete this._panelMenuView;
     aEvent.target.removeEventListener("ViewHiding", this);
   },
 
   showBookmarkingTools(triggerNode) {
-    const panelID = "PanelUI-bookmarkingTools";
-    let viewNode = document.getElementById(panelID);
-    for (let button of [...viewNode.getElementsByTagName("toolbarbutton")]) {
-      let update = true;
-      switch (button.id) {
-        case "panelMenu_toggleBookmarksMenu":
-          let placement = CustomizableUI.getPlacementOfWidget(this.BOOKMARK_BUTTON_ID);
-          button.setAttribute("checked", !!placement && placement.area == CustomizableUI.AREA_NAVBAR);
-          break;
-        case "panelMenu_viewBookmarksSidebar":
-          button.setAttribute("checked", SidebarUI.currentID == "viewBookmarksSidebar");
-          break;
-        case "panelMenu_viewBookmarksToolbar":
-          let toolbar = document.getElementById("PersonalToolbar");
-          // This is an actual toolbarbutton[type=checkbox], and its checked
-          // attribute will get added/removed by the binding when clicked.
-          // Setting the attribute to 'false' breaks showing the toolbar,
-          // because the binding removes the attribute instead of setting it
-          // to 'true' when clicked.
-          if (toolbar.getAttribute("collapsed") != "true") {
-            button.setAttribute("checked", "true");
-          } else {
-            button.removeAttribute("checked");
-          }
-          break;
-        default:
-          update = false;
-          break;
-      }
-      if (update) {
-        updateToggleControlLabel(button);
-      }
-    }
-    PanelUI.showSubView(panelID, triggerNode);
+    let placement = CustomizableUI.getPlacementOfWidget(this.BOOKMARK_BUTTON_ID);
+    this.selectLabel("panelMenu_toggleBookmarksMenu",
+                     placement && placement.area == CustomizableUI.AREA_NAVBAR);
+    this.selectLabel("panelMenu_viewBookmarksSidebar",
+                     SidebarUI.currentID == "viewBookmarksSidebar");
+    this.selectLabel("panelMenu_viewBookmarksToolbar",
+                     !document.getElementById("PersonalToolbar").collapsed);
+    PanelUI.showSubView("PanelUI-bookmarkingTools", triggerNode);
   },
 
   toggleMenuButtonInToolbar(triggerNode) {
     let placement = CustomizableUI.getPlacementOfWidget(this.BOOKMARK_BUTTON_ID);
     const area = CustomizableUI.AREA_NAVBAR;
     if (!placement) {
       // Button is in the palette, so we can move it to the navbar.
       let pos;
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -5600,18 +5600,16 @@ function setToolbarVisibility(toolbar, i
     detail: {
       visible: isVisible
     },
     bubbles: true
   };
   let event = new CustomEvent("toolbarvisibilitychange", eventParams);
   toolbar.dispatchEvent(event);
 
-  BookmarkingUI.onToolbarVisibilityChange();
-
   if (toolbar.getAttribute("type") == "menubar" && CustomizationHandler.isCustomizing()) {
     gCustomizeMode._updateDragSpaceCheckbox();
   }
 }
 
 function updateToggleControlLabel(control) {
   if (!control.hasAttribute("label-checked")) {
     return;
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -1105,21 +1105,19 @@
                    onmouseup="BookmarksEventHandler.onMouseUp(event);"
                    oncommand="BookmarksEventHandler.onCommand(event);"
                    onclick="BookmarksEventHandler.onClick(event, this.parentNode._placesView);"
                    onpopupshowing="BookmarkingUI.onPopupShowing(event);
                                    BookmarkingUI.attachPlacesView(event, this);"
                    tooltip="bhTooltip" popupsinherittooltip="true">
           <menuitem id="BMB_viewBookmarksSidebar"
                     class="subviewbutton"
-                    label="&viewBookmarksSidebar2.label;"
-                    type="checkbox"
-                    oncommand="SidebarUI.toggle('viewBookmarksSidebar');">
-            <observes element="viewBookmarksSidebar" attribute="checked"/>
-          </menuitem>
+                    label-show="&viewBookmarksSidebar2.label;"
+                    label-hide="&hideBookmarksSidebar.label;"
+                    oncommand="SidebarUI.toggle('viewBookmarksSidebar');"/>
           <!-- NB: temporary solution for bug 985024, this should go away soon. -->
           <menuitem id="BMB_bookmarksShowAllTop"
                     class="menuitem-iconic subviewbutton"
                     label="&showAllBookmarks2.label;"
                     command="Browser:ShowAllBookmarks"
                     key="manBookmarkKb"/>
           <menuseparator/>
           <menu id="BMB_bookmarksToolbar"
@@ -1128,21 +1126,20 @@
                 container="true">
             <menupopup id="BMB_bookmarksToolbarPopup"
                        placespopup="true"
                        context="placesContext"
                        onpopupshowing="if (!this.parentNode._placesView)
                                          new PlacesMenu(event, 'place:folder=TOOLBAR',
                                                         PlacesUIUtils.getViewForNode(this.parentNode.parentNode).options);">
               <menuitem id="BMB_viewBookmarksToolbar"
-                        placesanonid="view-toolbar"
-                        toolbarId="PersonalToolbar"
-                        type="checkbox"
-                        oncommand="onViewToolbarCommand(event)"
-                        label="&viewBookmarksToolbar.label;"/>
+                        class="subviewbutton"
+                        label-show="&viewBookmarksToolbar.label;"
+                        label-hide="&hideBookmarksToolbar.label;"
+                        oncommand="BookmarkingUI.toggleBookmarksToolbar();"/>
               <menuseparator/>
               <!-- Bookmarks toolbar items -->
             </menupopup>
           </menu>
           <menu id="BMB_unsortedBookmarks"
                 class="menu-iconic bookmark-item subviewbutton"
                 label="&bookmarksMenuButton.other.label;"
                 container="true">
--- a/browser/components/customizableui/content/panelUI.inc.xul
+++ b/browser/components/customizableui/content/panelUI.inc.xul
@@ -655,36 +655,31 @@
           <!-- Recent Highlights will go here -->
         </toolbaritem>
       </vbox>
     </panelview>
 
     <panelview id="PanelUI-bookmarkingTools" class="PanelUI-subView">
       <vbox class="panel-subview-body">
         <toolbarbutton id="panelMenu_toggleBookmarksMenu"
-                       label="&addBookmarksMenu.label;"
-                       label-checked="&removeBookmarksMenu.label;"
                        class="subviewbutton subviewbutton-iconic"
-                       oncommand="BookmarkingUI.toggleMenuButtonInToolbar(this); PanelUI.hide();"/>
+                       label-show="&addBookmarksMenu.label;"
+                       label-hide="&removeBookmarksMenu.label;"
+                       oncommand="BookmarkingUI.toggleMenuButtonInToolbar(this);"/>
         <toolbarbutton id="panelMenu_viewBookmarksSidebar"
-                       label="&viewBookmarksSidebar2.label;"
-                       label-checked="&hideBookmarksSidebar.label;"
                        class="subviewbutton subviewbutton-iconic"
+                       label-show="&viewBookmarksSidebar2.label;"
+                       label-hide="&hideBookmarksSidebar.label;"
                        key="viewBookmarksSidebarKb"
-                       oncommand="SidebarUI.toggle('viewBookmarksSidebar', this); PanelUI.hide();">
-          <observes element="viewBookmarksSidebar" attribute="checked"/>
-        </toolbarbutton>
+                       oncommand="SidebarUI.toggle('viewBookmarksSidebar', this);"/>
         <toolbarbutton id="panelMenu_viewBookmarksToolbar"
                        class="subviewbutton subviewbutton-iconic"
-                       placesanonid="view-toolbar"
-                       toolbarId="PersonalToolbar"
-                       type="checkbox"
-                       oncommand="onViewToolbarCommand(event)"
-                       label="&viewBookmarksToolbar.label;"
-                       label-checked="&hideBookmarksToolbar.label;"/>
+                       label-show="&viewBookmarksToolbar.label;"
+                       label-hide="&hideBookmarksToolbar.label;"
+                       oncommand="BookmarkingUI.toggleBookmarksToolbar();"/>
       </vbox>
     </panelview>
   </panelmultiview>
 </panel>
 
 <panel id="downloads-button-autohide-panel"
        role="group"
        type="arrow"