Bug 1391948 - Don't use checkboxes for the "View Bookmarks Toolbar" and "View Bookmarks Sidebar" items in the Bookmarks button menu. r=mak draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Wed, 11 Apr 2018 12:42:18 +0100
changeset 780420 5d9441fecb8f99e8b460a8cd8b7a8f2d09f558ea
parent 779146 30d72755b1749953d438199456f1524ce84ab5e5
child 780423 cb927878fa66a52090aaeef2c675cee4bec92b3a
push id105992
push userpaolo.mozmail@amadzone.org
push dateWed, 11 Apr 2018 11:46:22 +0000
reviewersmak
bugs1391948
milestone61.0a1
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
@@ -1281,27 +1281,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.
@@ -1323,33 +1312,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", {
@@ -1432,17 +1419,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");
@@ -1643,50 +1629,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
@@ -5598,18 +5598,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"