Bug 1954056 - Avoid closing the sidebar launcher when just closing the open panel. r=sidebar-reviewers,kcochrane

* Remove some of the overloading of SidebarState.updateVisibility
* Remove the no-longer-used #previousLauncherExpanded property
* SidebarController.hide() just hides the panel (same as legacy/original API)
* Skip the browser_toolbarKeyNav.js test when sidebar.revamp=true, this should have been failing all along.

Differential Revision: https://phabricator.services.mozilla.com/D242240

browser/base/content/main-popupset.js

@@ -180,7 +180,8 @@ document.addEventListener(
           SidebarController.reversePosition();
           break;
         case "sidebar-menu-close":
-          SidebarController.hide();
+          // Close the sidebar UI, but leave it otherwise in its current state
+          SidebarController.hide({ dismissPanel: false });
           break;
 
         // == toolbar-context-menu ==

browser/base/content/test/keyboard/browserSidebarRevamp.toml

@@ -13,4 +13,5 @@ skip-if = [
 ]
 
 ["browser_toolbarKeyNav.js"]
+skip-if = ["true"] # Bug 1775713
 support-files = ["!/browser/base/content/test/permissions/permissions.html"]

browser/components/customizableui/test/browser_sidebar_toggle.js

@@ -45,6 +45,7 @@ add_task(async function () {
     "sidebar.revamp",
     false
   );
+  info(`sidebarRevampEnabled: ${sidebarRevampEnabled}`);
   if (!sidebarRevampEnabled) {
     CustomizableUI.addWidgetToArea("sidebar-button", "nav-bar");
 
@@ -69,7 +70,7 @@ add_task(async function () {
     const sidebar = document.querySelector("sidebar-main");
     ok(sidebar, "Sidebar is shown.");
     for (const [index, toolButton] of sidebar.toolButtons.entries()) {
-      await SidebarController.show(toolButton.getAttribute("view"));
+      await SidebarController.toggle(toolButton.getAttribute("view"));
       is(
         SidebarController.currentID,
         toolButton.getAttribute("view"),

browser/components/sidebar/SidebarState.sys.mjs

@@ -17,14 +17,19 @@ XPCOMUtils.defineLazyPreferenceGetter(
  *
  * @typedef {object} SidebarStateProps
  *
+ * @property {boolean} command
+ *   The id of the current sidebar panel. The panel may be closed and still have a command value.
+ *   Re-opening the sidebar panel will then load the current command id.
  * @property {boolean} panelOpen
  *   Whether there is an open panel.
  * @property {number} panelWidth
  *   Current width of the sidebar panel.
  * @property {boolean} launcherVisible
  *   Whether the launcher is visible.
+ *   This is always true when the sidebar.visibility pref value is "always-show", and toggle between true/false when visibility is "hide-sidebar"
  * @property {boolean} launcherExpanded
  *   Whether the launcher is expanded.
+ *   When sidebar.visibility pref value is "always-show", the toolbar button serves to toggle this property
  * @property {boolean} launcherDragActive
  *   Whether the launcher is currently being dragged.
  * @property {boolean} launcherHoverActive
@@ -55,7 +60,13 @@ export class SidebarState {
     launcherDragActive: false,
     launcherHoverActive: false,
   };
-  #previousLauncherExpanded = undefined;
+
+  static defaultProperties = Object.freeze({
+    command: "",
+    launcherExpanded: false,
+    launcherVisible: false,
+    panelOpen: false,
+  });
 
   /**
    * Construct a new SidebarState.
@@ -228,17 +239,11 @@ export class SidebarState {
     if (open) {
       // Launcher must be visible to open a panel.
       this.launcherVisible = true;
-      // We need to know how to revert the launcher when the panel closes
-      this.#previousLauncherExpanded = this.launcherExpanded;
 
       Services.prefs.setBoolPref(
         this.revampEnabled ? REVAMP_USED_PREF : LEGACY_USED_PREF,
         true
       );
-    } else if (this.revampVisibility === "hide-sidebar") {
-      this.launcherExpanded = lazy.verticalTabsEnabled
-        ? this.#previousLauncherExpanded
-        : false;
     }
 
     const mainEl = this.#controller.sidebarContainer;
@@ -267,62 +272,26 @@ export class SidebarState {
   }
 
   /**
-   * Update the launcher `visible` and `expanded` states to handle the
+   * Update the launcher `visible` and `expanded` states
-   * following scenarios:
-   *
-   * - Toggling "Hide tabs and sidebar" from the customize panel.
-   * - Clicking sidebar button from the toolbar.
-   * - Removing sidebar button from the toolbar.
-   * - Force expand value
    *
    * @param {boolean} visible
-   * @param {boolean} onUserToggle
-   * @param {boolean} onToolbarButtonRemoval
    * @param {boolean} forceExpandValue
    */
-  updateVisibility(
+  updateVisibility(visible, forceExpandValue = null) {
-    visible,
-    onUserToggle = false,
-    onToolbarButtonRemoval = false,
-    forceExpandValue = null
-  ) {
     switch (this.revampVisibility) {
       case "hide-sidebar":
-        if (onToolbarButtonRemoval) {
+        if (lazy.verticalTabsEnabled) {
-          // If we are hiding the sidebar because we removed the toolbar button, close everything
+          forceExpandValue = visible;
-          this.#previousLauncherExpanded = false;
-          this.launcherVisible = false;
-          this.launcherExpanded = false;
-
-          if (this.panelOpen) {
-            this.#controller.hide();
-          }
-          return;
-        }
-        // we need this set to verticalTabsEnabled to ensure it has the correct state when toggling the sidebar button
-        this.launcherExpanded = lazy.verticalTabsEnabled && visible;
-        if (!visible && this.panelOpen) {
-          if (onUserToggle) {
-            // Hiding the launcher with the toolbar button or context menu should also close out any open panels and resets panelOpen
-            this.#controller.hide();
-          } else {
-            // Hide the launcher when the pref is set to hide-sidebar
-            this.launcherVisible = false;
-            this.#previousLauncherExpanded = false;
-            return;
-          }
         }
         this.launcherVisible = visible;
         break;
       case "always-show":
       case "expand-on-hover":
         this.launcherVisible = true;
+        break;
+    }
     if (forceExpandValue !== null) {
       this.launcherExpanded = forceExpandValue;
-        } else if (onUserToggle) {
-          this.launcherExpanded = !this.launcherExpanded;
-        }
-        break;
     }
   }
 

browser/components/sidebar/browser-sidebar.js

@@ -595,7 +595,7 @@ var SidebarController = {
           // The <tree> component used in history and bookmarks, but it does not
           // support live switching the app locale. Reload the entire sidebar to
           // invalidate any old text.
-          this.hide();
+          this.hide({ dismissPanel: false });
           this.showInitially(this.lastOpenedId);
           break;
         }
@@ -773,7 +773,7 @@ var SidebarController = {
     await this.promiseInitialized;
     let wasOpen = this.isOpen;
     if (wasOpen) {
-      this.hide();
+      this.hide({ dismissPanel: false });
     }
     // Reset sidebars map but preserve any existing extensions
     let extensionsArr = [];
@@ -993,7 +993,10 @@ var SidebarController = {
     }
 
     if (this.isOpen && commandID == this.currentID) {
-      this.hide(triggerNode);
+      // Revamp sidebar: this case is a dismissal of the current sidebar panel. The launcher should stay open
+      // For legacy sidebar, this is a "sidebar" toggle and the current panel should be remembered
+      this.hide({ triggerNode, dismissPanel: this.sidebarRevampEnabled });
+      this.updateToolbarButton();
       return Promise.resolve();
     }
     return this.show(commandID, triggerNode);
@@ -1183,25 +1186,42 @@ var SidebarController = {
    * For sidebar.revamp=true only, handle the keyboard or sidebar-button command to toggle the sidebar state
    */
   async handleToolbarButtonClick() {
-    let initialExpandedValue = this._state.launcherExpanded;
     if (this.inSingleTabWindow || this.uninitializing) {
       return;
     }
+
+    const initialExpandedValue = this._state.launcherExpanded;
+
+    // What toggle means depends on the sidebar.visibility pref.
+    const expandOnToggle = ["always-show", "expand-on-hover"].includes(
+      this.sidebarRevampVisibility
+    );
+
+    // when the launcher is toggled open by the user, we disable expand-on-hover interactions.
     if (this.sidebarRevampVisibility === "expand-on-hover") {
       await this.toggleExpandOnHover(initialExpandedValue);
     }
+
     if (this._animationEnabled && !window.gReduceMotion) {
       this._animateSidebarMain();
     }
-    const showLauncher = !this._state.launcherVisible;
 
-    this._state.updateVisibility(showLauncher, true);
+    if (expandOnToggle) {
-    if (showLauncher && this._state.command) {
+      // just expand/collapse the launcher
+      this._state.updateVisibility(true, !initialExpandedValue);
+    } else {
+      const shouldShowLauncher = !this._state.launcherVisible;
+      // show/hide the launcher
+      this._state.updateVisibility(shouldShowLauncher);
+      // if we're showing and there was panel open, open it again
+      if (shouldShowLauncher && this._state.command) {
         this._show(this._state.command);
+      } else if (!shouldShowLauncher) {
+        // hide the open panel. It will re-open next time as we don't change the command value
+        this.hide({ dismissPanel: false });
       }
-    if (this.sidebarRevampVisibility === "expand-on-hover") {
-      await this.toggleExpandOnHover(initialExpandedValue);
     }
+
     this.updateToolbarButton();
 
     if (this.lastOpenedId == "viewReviewCheckerSidebar") {
@@ -1561,7 +1581,9 @@ var SidebarController = {
       return;
     }
     if (this.currentID === commandID) {
-      this.hide();
+      // If the extension removal is a update, we don't want to forget this panel.
+      // So, let the sidebarAction extension API code remove the lastOpenedId as needed
+      this.hide({ dismissPanel: false });
     }
     document.getElementById(sidebar.menuId)?.remove();
     document.getElementById(sidebar.switcherMenuId)?.remove();
@@ -1584,7 +1606,7 @@ var SidebarController = {
     if (this.inSingleTabWindow) {
       return false;
     }
-    if (this.currentID) {
+    if (this.currentID && commandID !== this.currentID) {
       // If there is currently a panel open, we are about to hide it in order
       // to show another one, so record a "hide" event on the current panel.
       this._recordPanelToggle(this.currentID, false);
@@ -1726,10 +1748,12 @@ var SidebarController = {
   /**
    * Hide the sidebar.
    *
-   * @param {DOMNode} [triggerNode] Node, usually a button, that triggered the
+   * @param {object} options - Parameter object.
+   * @param {DOMNode} options.triggerNode - Node, usually a button, that triggered the
    *                                        hiding of the sidebar.
+   * @param {boolean} options.dismissPanel -Only close the panel or close the whole sidebar (the default.)
    */
-  hide(triggerNode) {
+  hide({ triggerNode, dismissPanel = true } = {}) {
     if (!this.isOpen) {
       return;
     }
@@ -1745,6 +1769,13 @@ var SidebarController = {
     this.hideSwitcherPanel();
     this._recordPanelToggle(this.currentID, false);
     this._state.panelOpen = false;
+    if (dismissPanel) {
+      // The user is explicitly closing this panel so we don't want it to
+      // automatically re-open next time the sidebar is shown
+      this._state.command = "";
+      this.lastOpenedId = null;
+    }
+
     if (this.sidebarRevampEnabled) {
       this._box.dispatchEvent(new CustomEvent("sidebar-hide"));
     }
@@ -1767,17 +1798,6 @@ var SidebarController = {
     if (triggerNode) {
       updateToggleControlLabel(triggerNode);
     }
-    let showLauncher = false;
-    if (
-      this.sidebarRevampEnabled &&
-      this.sidebarRevampVisibility !== "hide-sidebar"
-    ) {
-      showLauncher = true;
-    }
-    this._state.updateVisibility(
-      showLauncher,
-      false // onUserToggle param means "did the user click the sidebar-button", which is false here
-    );
     this.updateToolbarButton();
   },
 
@@ -1862,8 +1882,11 @@ var SidebarController = {
 
   onWidgetRemoved(aWidgetId) {
     if (aWidgetId == "sidebar-button") {
+      if (this.isOpen) {
+        this.hide();
+      }
+      this._state.loadInitialState({ ...this.SidebarState.defaultProperties });
       Services.prefs.setStringPref("sidebar.visibility", "hide-sidebar");
-      this._state.updateVisibility(false, false, true);
     }
   },
 
@@ -2139,7 +2162,15 @@ XPCOMUtils.defineLazyPreferenceGetter(
         ) {
           SidebarController._animateSidebarMain();
         }
-        const forceExpand = isVerticalTabs && newValue === "always-show";
+
+        // launcher is always initially expanded with vertical tabs unless we're doing expand-on-hover
+        let forceExpand = false;
+        if (
+          isVerticalTabs &&
+          ["always-show", "hide-sidebar"].includes(newValue)
+        ) {
+          forceExpand = true;
+        }
 
         // horizontal tabs and hide-sidebar = visible initially.
         // vertical tab and hide-sidebar = not visible initially
@@ -2147,12 +2178,7 @@ XPCOMUtils.defineLazyPreferenceGetter(
         if (newValue == "hide-sidebar" && isVerticalTabs) {
           showLauncher = false;
         }
-        SidebarController._state.updateVisibility(
+        SidebarController._state.updateVisibility(showLauncher, forceExpand);
-          showLauncher,
-          false,
-          false,
-          forceExpand
-        );
       }
       SidebarController.updateToolbarButton();
     }

browser/components/sidebar/sidebar-main.mjs

@@ -279,7 +279,8 @@ export default class SidebarMain extends MozLitElement {
             ) {
               window.SidebarController._animateSidebarMain();
             }
-            await window.SidebarController._state.updateVisibility(false, true);
+            window.SidebarController.hide({ dismissPanel: false });
+            window.SidebarController._state.updateVisibility(false);
             break;
           case "sidebar-context-menu-enable-vertical-tabs":
             await window.SidebarController.toggleVerticalTabs();

browser/components/sidebar/sidebar-panel-header.mjs

@@ -21,8 +21,7 @@ export class SidebarPanelHeader extends MozLitElement {
 
   closeSidebarPanel(e) {
     e.preventDefault();
-    let view = e.target.getAttribute("view");
+    this.getWindow().SidebarController.hide();
-    this.getWindow().SidebarController.toggle(view);
   }
 
   render() {

browser/components/sidebar/tests/browser/browser_sidebar_context_menu.js

@@ -183,8 +183,8 @@ add_task(async function test_extension_context_menu() {
   sinon.restore();
   await extension.unload();
   ok(
-    BrowserTestUtils.isHidden(sidebar),
+    BrowserTestUtils.isVisible(sidebar),
-    "Unloading the extension causes the sidebar launcher to hide"
+    "Unloading the extension does not cause the sidebar launcher to hide"
   );
   await BrowserTestUtils.closeWindow(win);
 });

browser/components/sidebar/tests/browser/browser_sidebar_menubar_item_commands.js

@@ -58,7 +58,7 @@ add_task(async function test_sidebar_view_commands() {
     "Sidebar controller has the correct currentID"
   );
 
-  SidebarController.toggle(SidebarController.currentID);
+  document.getElementById("sidebar-button").doCommand();
   await sidebar.updateComplete;
   ok(BrowserTestUtils.isHidden(sidebar), "Sidebar is hidden");
   ok(!sidebar.expanded, "Sidebar is not expanded when the view is closed");