Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;r=mikedeboer draft
authorBrian Grinstead <bgrinstead@mozilla.com>
Wed, 24 May 2017 09:54:34 -0700
changeset 583799 13a3281744ca1d16b478bc6269ce4f189e8b22b4
parent 583335 96e18bec9fc8a5ce623c16167c12756bbe190d73
child 630207 78f61def4d4403038fc827e3abf0d28b4f016b49
push id60551
push userbgrinstead@mozilla.com
push dateWed, 24 May 2017 17:06:13 +0000
reviewersmikedeboer
bugs1355331
milestone55.0a1
Bug 1355331 - Create an option to move sidebar between the left and right sides of the window;r=mikedeboer MozReview-Commit-ID: 79ts9djMC3e
browser/app/profile/firefox.js
browser/base/content/browser-sidebar.js
browser/base/content/browser.xul
browser/base/content/test/sidebar/browser.ini
browser/base/content/test/sidebar/browser_sidebar_move.js
browser/base/content/test/sidebar/browser_sidebar_switcher.js
browser/locales/en-US/chrome/browser/browser.properties
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1267,16 +1267,19 @@ pref("pdfjs.firstRun", true);
 pref("pdfjs.previousHandler.preferredAction", 0);
 pref("pdfjs.previousHandler.alwaysAskBeforeHandling", false);
 
 // The maximum amount of decoded image data we'll willingly keep around (we
 // might keep around more than this, but we'll try to get down to this value).
 // (This is intentionally on the high side; see bug 746055.)
 pref("image.mem.max_decoded_image_kb", 256000);
 
+// Is the sidebar positioned ahead of the content browser
+pref("sidebar.position_start", true);
+
 // Activation from inside of share panel is possible if activationPanelEnabled
 // is true. Pref'd off for release while usage testing is done through beta.
 pref("social.share.activationPanelEnabled", true);
 pref("social.shareDirectory", "https://activations.cdn.mozilla.net/sharePanel.html");
 
 // Block insecure active content on https pages
 pref("security.mixed_content.block_active_content", true);
 
--- a/browser/base/content/browser-sidebar.js
+++ b/browser/base/content/browser-sidebar.js
@@ -16,27 +16,34 @@
  *                 display on the sidebar.
  *  - checked      indicates whether the sidebar is currently displayed.
  *                 Note that toggleSidebar updates this attribute when
  *                 it changes the sidebar's visibility.
  *  - group        this attribute must be set to "sidebar".
  */
 var SidebarUI = {
   browser: null,
+  POSITION_START_PREF: "sidebar.position_start",
 
   _box: null,
   _title: null,
   _splitter: null,
+  _icon: null,
+  _reversePositionButton: null,
+  _switcherPanel: null,
+  _switcherTarget: null,
+  _switcherArrow: null,
 
   init() {
     this._box = document.getElementById("sidebar-box");
     this.browser = document.getElementById("sidebar");
     this._title = document.getElementById("sidebar-title");
     this._splitter = document.getElementById("sidebar-splitter");
     this._icon = document.getElementById("sidebar-icon");
+    this._reversePositionButton = document.getElementById("sidebar-reverse-position");
     this._switcherPanel = document.getElementById("sidebarMenu-popup");
     this._switcherTarget = document.getElementById("sidebar-switcher-target");
     this._switcherArrow = document.getElementById("sidebar-switcher-arrow");
 
     this._switcherTarget.addEventListener("command", () => {
       this.toggleSwitcherPanel();
     });
   },
@@ -67,16 +74,23 @@ var SidebarUI = {
     this._switcherPanel.hidePopup();
   },
 
   showSwitcherPanel() {
     this._ensureShortcutsShown();
     this._switcherPanel.addEventListener("popuphiding", () => {
       this._switcherTarget.classList.remove("active");
     }, {once: true});
+
+    // Combine start/end position with ltr/rtl to set the label in the popup appropriately.
+    let label = this._positionStart == this.isRTL ?
+                  gNavigatorBundle.getString("sidebar.moveToLeft") :
+                  gNavigatorBundle.getString("sidebar.moveToRight");
+    this._reversePositionButton.setAttribute("label", label);
+
     this._switcherPanel.hidden = false;
     this._switcherPanel.openPopup(this._icon);
     this._switcherTarget.classList.add("active");
   },
 
   _addedShortcuts: false,
   _ensureShortcutsShown() {
     if (this._addedShortcuts) {
@@ -89,16 +103,47 @@ var SidebarUI = {
       if (!key) {
         continue;
       }
       button.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(key));
     }
   },
 
   /**
+   * Change the pref that will trigger a call to setPosition
+   */
+  reversePosition() {
+    Services.prefs.setBoolPref(this.POSITION_START_PREF, !this._positionStart);
+  },
+
+  /**
+   * Read the positioning pref and position the sidebar and the splitter
+   * appropriately within the browser container.
+   */
+  setPosition() {
+    // First reset all ordinals to match DOM ordering.
+    let browser = document.getElementById("browser");
+    [...browser.childNodes].forEach((node, i) => {
+      node.ordinal = i + 1;
+    });
+
+    if (!this._positionStart) {
+      // DOM ordering is:     |  sidebar-box  | splitter |   appcontent  |
+      // Want to display as:  |   appcontent  | splitter |  sidebar-box  |
+      // So we just swap box and appcontent ordering
+      let appcontent = document.getElementById("appcontent");
+      let boxOrdinal = this._box.ordinal;
+      this._box.ordinal = appcontent.ordinal;
+      appcontent.ordinal = boxOrdinal;
+    }
+
+    this.hideSwitcherPanel();
+  },
+
+  /**
    * Try and adopt the status of the sidebar from another window.
    * @param {Window} sourceWindow - Window to use as a source for sidebar status.
    * @return true if we adopted the state, or false if the caller should
    * initialize the state itself.
    */
   adoptFromWindow(sourceWindow) {
     // If the opener had a sidebar, open the same sidebar in our window.
     // The opener can be the hidden window too, if we're coming from the state
@@ -127,18 +172,17 @@ var SidebarUI = {
     this._box.setAttribute("width", sourceUI._box.boxObject.width);
 
     this._box.setAttribute("sidebarcommand", commandID);
     // Note: we're setting 'src' on this._box, which is a <vbox>, not on
     // the <browser id="sidebar">. This lets us delay the actual load until
     // delayedStartup().
     this._box.setAttribute("src", sourceUI.browser.getAttribute("src"));
 
-    this._box.hidden = false;
-    this._splitter.hidden = false;
+    this._setVisibility(true);
     commandElem.setAttribute("checked", "true");
     this.browser.setAttribute("src", this._box.getAttribute("src"));
     return true;
   },
 
   windowPrivacyMatches(w1, w2) {
     return PrivateBrowsingUtils.isWindowPrivate(w1) === PrivateBrowsingUtils.isWindowPrivate(w2);
   },
@@ -165,18 +209,17 @@ var SidebarUI = {
     // If we're not adopting settings from a parent window, set them now.
     let commandID = this._box.getAttribute("sidebarcommand");
     if (!commandID) {
       return;
     }
 
     let command = document.getElementById(commandID);
     if (command) {
-      this._box.hidden = false;
-      this._splitter.hidden = false;
+      this._setVisibility(true);
       command.setAttribute("checked", "true");
       this.browser.setAttribute("src", this._box.getAttribute("src"));
     } else {
       // Remove the |sidebarcommand| attribute, because the element it
       // refers to no longer exists, so we should assume this sidebar
       // panel has been uninstalled. (249883)
       this._box.removeAttribute("sidebarcommand");
     }
@@ -215,16 +258,29 @@ var SidebarUI = {
     return this._title.value;
   },
 
   set title(value) {
     this._title.value = value;
   },
 
   /**
+   * Internal helper to show/hide the box and splitter elements.
+   *
+   * @param {bool} visible
+   */
+  _setVisibility(visible) {
+    this._box.hidden = !visible;
+    this._splitter.hidden = !visible;
+    if (visible) {
+      this.setPosition();
+    }
+  },
+
+  /**
    * Toggle the visibility of the sidebar. If the sidebar is hidden or is open
    * with a different commandID, then the sidebar will be opened using the
    * specified commandID. Otherwise the sidebar will be hidden.
    *
    * @param {string} commandID ID of the xul:broadcaster element to use.
    * @return {Promise}
    */
   toggle(commandID = this.currentID) {
@@ -263,18 +319,17 @@ var SidebarUI = {
 
         if (broadcaster != sidebarBroadcaster) {
           broadcaster.removeAttribute("checked");
         } else {
           sidebarBroadcaster.setAttribute("checked", "true");
         }
       }
 
-      this._box.hidden = false;
-      this._splitter.hidden = false;
+      this._setVisibility(true);
 
       this.hideSwitcherPanel();
 
       this._box.setAttribute("sidebarcommand", sidebarBroadcaster.id);
       this.lastOpenedId = sidebarBroadcaster.id;
 
       let title = sidebarBroadcaster.getAttribute("sidebartitle");
       if (!title) {
@@ -342,28 +397,35 @@ var SidebarUI = {
     // until about:blank has loaded (which does not happen as long as the
     // element is hidden).
     this.browser.setAttribute("src", "about:blank");
     this.browser.docShell.createAboutBlankContentViewer(null);
 
     sidebarBroadcaster.removeAttribute("checked");
     this._box.setAttribute("sidebarcommand", "");
     this._title.value = "";
-    this._box.hidden = true;
-    this._splitter.hidden = true;
+    this._setVisibility(false);
 
     let selBrowser = gBrowser.selectedBrowser;
     selBrowser.focus();
     selBrowser.messageManager.sendAsyncMessage("Sidebar:VisibilityChange",
       {commandID, isOpen: false}
     );
     BrowserUITelemetry.countSidebarEvent(commandID, "hide");
   },
 };
 
+// Add getters related to the position here, since we will want them
+// available for both startDelayedLoad and init.
+XPCOMUtils.defineLazyPreferenceGetter(SidebarUI, "_positionStart",
+  SidebarUI.POSITION_START_PREF, true, SidebarUI.setPosition.bind(SidebarUI));
+XPCOMUtils.defineLazyGetter(SidebarUI, "isRTL", () => {
+  return getComputedStyle(document.documentElement).direction == "rtl";
+});
+
 /**
  * This exists for backwards compatibility - it will be called once a sidebar is
  * ready, following any request to show it.
  *
  * @deprecated
  */
 function fireSidebarFocusedEvent() {}
 
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -316,16 +316,20 @@
       <toolbarbutton id="sidebar-switcher-tabs"
                      label="&syncedTabs.sidebar.label;"
                      class="subviewbutton subviewbutton-iconic"
                      observes="viewTabsSidebar"
                      oncommand="SidebarUI.show('viewTabsSidebar');">
          <observes element="viewTabsSidebar" attribute="checked"/>
        </toolbarbutton>
       <toolbarseparator/>
+      <toolbarbutton id="sidebar-reverse-position"
+                     class="subviewbutton"
+                     oncommand="SidebarUI.reversePosition()"/>
+      <toolbarseparator/>
       <toolbarbutton label="&sidebarMenuClose.label;"
                      class="subviewbutton"
                      oncommand="SidebarUI.hide()"/>
     </panel>
 
     <menupopup id="toolbar-context-menu"
                onpopupshowing="onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator'));">
       <menuitem oncommand="gCustomizeMode.addToPanel(document.popupNode)"
--- a/browser/base/content/test/sidebar/browser.ini
+++ b/browser/base/content/test/sidebar/browser.ini
@@ -1,4 +1,5 @@
 [DEFAULT]
 
 [browser_bug409481.js]
+[browser_sidebar_move.js]
 [browser_sidebar_switcher.js]
new file mode 100644
--- /dev/null
+++ b/browser/base/content/test/sidebar/browser_sidebar_move.js
@@ -0,0 +1,54 @@
+
+registerCleanupFunction(() => {
+  Services.prefs.clearUserPref("sidebar.position_start");
+  SidebarUI.hide();
+});
+
+const EXPECTED_START_ORDINALS = [
+  ["browser-border-start", 1],
+  ["sidebar-box", 2],
+  ["sidebar-splitter", 3],
+  ["appcontent", 4],
+  ["browser-border-end", 5],
+];
+
+const EXPECTED_END_ORDINALS = [
+  ["browser-border-start", 1],
+  ["sidebar-box", 4],
+  ["sidebar-splitter", 3],
+  ["appcontent", 2],
+  ["browser-border-end", 5],
+];
+
+function getBrowserChildrenWithOrdinals() {
+  let browser = document.getElementById("browser");
+  return [...browser.childNodes].map(node => {
+    return [node.id, node.ordinal];
+  });
+}
+
+add_task(function* () {
+  yield SidebarUI.show("viewBookmarksSidebar");
+  SidebarUI.showSwitcherPanel();
+
+  let reversePositionButton = document.getElementById("sidebar-reverse-position");
+  let originalLabel = reversePositionButton.getAttribute("label");
+
+  // Default (position: left)
+  Assert.deepEqual(getBrowserChildrenWithOrdinals(),
+    EXPECTED_START_ORDINALS, "Correct ordinal (start)");
+
+  // Moved to right
+  SidebarUI.reversePosition();
+  SidebarUI.showSwitcherPanel();
+  Assert.deepEqual(getBrowserChildrenWithOrdinals(),
+    EXPECTED_END_ORDINALS, "Correct ordinal (end)");
+  isnot(reversePositionButton.getAttribute("label"), originalLabel, "Label changed");
+
+  // Moved to back to left
+  SidebarUI.reversePosition();
+  SidebarUI.showSwitcherPanel();
+  Assert.deepEqual(getBrowserChildrenWithOrdinals(),
+    EXPECTED_START_ORDINALS, "Correct ordinal (start)");
+  is(reversePositionButton.getAttribute("label"), originalLabel, "Label is back to normal");
+});
--- a/browser/base/content/test/sidebar/browser_sidebar_switcher.js
+++ b/browser/base/content/test/sidebar/browser_sidebar_switcher.js
@@ -1,8 +1,12 @@
+
+registerCleanupFunction(() => {
+  SidebarUI.hide();
+});
 
 function showSwitcherPanelPromise() {
   return new Promise(resolve => {
     SidebarUI._switcherPanel.addEventListener("popupshown", () => {
       resolve();
     }, {once: true});
     SidebarUI.showSwitcherPanel();
   });
--- a/browser/locales/en-US/chrome/browser/browser.properties
+++ b/browser/locales/en-US/chrome/browser/browser.properties
@@ -585,16 +585,20 @@ fullscreenButton.tooltip=Display the win
 service.toolbarbutton.label=Services
 service.toolbarbutton.tooltiptext=Services
 
 # LOCALIZATION NOTE (social.install.description): %1$S is the hostname of the social provider, %2$S is brandShortName (e.g. Firefox)
 service.install.description=Would you like to enable services from %1$S to display in your %2$S toolbar and sidebar?
 service.install.ok.label=Enable Services
 service.install.ok.accesskey=E
 
+# These are visible when opening the popup inside the bookmarks sidebar
+sidebar.moveToLeft=Move sidebar to left
+sidebar.moveToRight=Move sidebar to right
+
 # LOCALIZATION NOTE (social.markpageMenu.label): %S is the name of the social provider
 social.markpageMenu.label=Save Page to %S
 # LOCALIZATION NOTE (social.marklinkMenu.label): %S is the name of the social provider
 social.marklinkMenu.label=Save Link to %S
 
 # LOCALIZATION NOTE (social.error.message): %1$S is brandShortName (e.g. Firefox), %2$S is the name of the social provider
 social.error.message=%1$S is unable to connect with %2$S right now.
 social.error.tryAgain.label=Try Again