Bug 1939917 - Update Sidebar visibility settings and settings order r=sidebar-reviewers,fluent-reviewers,desktop-theme-reviewers,bolsson,sclements,hjones

- change order of settings as per spec
- only show visibilty settings if vertical tabs enabled

Depends on D234592

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

browser/components/sidebar/SidebarState.sys.mjs

@@ -239,7 +239,7 @@ export class SidebarState {
           return;
         }
         // we need this set to verticalTabsEnabled to ensure it has the correct state when toggling the sidebar button
-        this.launcherExpanded = lazy.verticalTabsEnabled;
+        this.launcherExpanded = lazy.verticalTabsEnabled && visible;
 
         if (!visible && this.panelOpen) {
           // Hiding the launcher should also close out any open panels and resets panelOpen

browser/components/sidebar/browser-sidebar.js

@@ -1827,7 +1827,6 @@ XPCOMUtils.defineLazyPreferenceGetter(
   "always-show",
   (_aPreference, _previousValue, newValue) => {
     if (!SidebarController.uninitializing) {
-      SidebarController.updateToolbarButton();
       SidebarController.recordVisibilitySetting(newValue);
       SidebarController._state.revampVisibility = newValue;
       SidebarController._state.updateVisibility(
@@ -1835,6 +1834,7 @@ XPCOMUtils.defineLazyPreferenceGetter(
           SidebarController.sidebarVerticalTabsEnabled) ||
           !SidebarController.sidebarVerticalTabsEnabled
       );
+      SidebarController.updateToolbarButton();
     }
   }
 );
@@ -1845,11 +1845,11 @@ XPCOMUtils.defineLazyPreferenceGetter(
   false,
   (_aPreference, _previousValue, newValue) => {
     if (!SidebarController.uninitializing) {
+      SidebarController.recordTabsLayoutSetting(newValue);
       Services.prefs.setStringPref(
         SidebarController.VISIBILITY_PREF,
         newValue ? "always-show" : "hide-sidebar"
       );
-      SidebarController.recordTabsLayoutSetting(newValue);
     }
   }
 );

browser/components/sidebar/sidebar-customize.css

@@ -14,6 +14,10 @@
   }
 }
 
+.medium-top-margin {
+  margin-block-start: var(--space-medium);
+}
+
 .customize-header {
   display: flex;
   justify-content: space-between;
@@ -61,7 +65,6 @@
   font-weight: var(--font-weight-bold);
 }
 
-moz-radio-group,
 moz-fieldset {
   display: block;
 
@@ -75,7 +78,6 @@ moz-fieldset {
   }
 }
 
-moz-radio,
 moz-checkbox {
   padding-block: var(--space-medium);
 
@@ -83,6 +85,13 @@ moz-checkbox {
     border-bottom: 0.5px solid var(--panel-separator-color);
   }
 
+  > moz-checkbox:last-of-type {
+    padding-block: 0;
+  }
+}
+
+.no-label::part(label) {
+  display: none;
 }
 
 #manage-settings {
@@ -92,6 +101,6 @@ moz-checkbox {
   padding-inline-start: var(--space-medium);
 }
 
-.customize-group {
+.customize-group:not(.no-end-margin) {
   margin-block-end: var(--space-xlarge);
 }

browser/components/sidebar/sidebar-customize.mjs

@@ -6,9 +6,6 @@ import { html, when } from "chrome://global/content/vendor/lit.all.mjs";
 
 import { SidebarPage } from "./sidebar-page.mjs";
 
-// eslint-disable-next-line import/no-unassigned-import
-import "chrome://global/content/elements/moz-radio-group.mjs";
-
 const lazy = {};
 
 ChromeUtils.defineESModuleGetters(lazy, {
@@ -44,11 +41,11 @@ export class SidebarCustomize extends SidebarPage {
   };
 
   static queries = {
-    toolInputs: { all: ".customize-group moz-checkbox" },
+    toolInputs: { all: ".tool" },
     extensionLinks: { all: ".extension-link" },
-    positionInputs: { all: ".position-setting" },
+    positionInput: "#position",
-    visibilityInputs: { all: ".visibility-setting" },
+    visibilityInput: "#hide-sidebar",
-    verticalTabsInputs: { all: ".vertical-tabs-setting" },
+    verticalTabsInput: "#vertical-tabs",
   };
 
   connectedCallback() {
@@ -94,6 +91,13 @@ export class SidebarCustomize extends SidebarPage {
     return this.getWindow().document.querySelector("sidebar-launcher");
   }
 
+  get fluentStrings() {
+    if (!this._fluentStrings) {
+      this._fluentStrings = new Localization(["browser/sidebar.ftl"], true);
+    }
+    return this._fluentStrings;
+  }
+
   getWindow() {
     return window.browsingContext.embedderWindowGlobal.browsingContext.window;
   }
@@ -158,6 +162,7 @@ export class SidebarCustomize extends SidebarPage {
     }
     return html`
       <moz-checkbox
+        class="tool"
         type="checkbox"
         id=${tool.view}
         name=${tool.view}
@@ -242,6 +247,42 @@ export class SidebarCustomize extends SidebarPage {
       <div class="sidebar-panel">
         <sidebar-panel-header data-l10n-id="sidebar-menu-customize-header" data-l10n-attrs="heading" view="viewCustomizeSidebar">
         </sidebar-panel-header>
+        <moz-fieldset class="customize-group no-end-margin" data-l10n-id="sidebar-settings">
+          <moz-checkbox
+            type="checkbox"
+            id="vertical-tabs"
+            name="verticalTabs"
+            iconsrc="chrome://browser/skin/sidebar-collapsed.svg"
+            data-l10n-id="sidebar-vertical-tabs"
+            @change=${this.#handleTabDirectionChange}
+            ?checked=${this.verticalTabsEnabled}
+          >
+            ${when(
+              this.verticalTabsEnabled,
+              () => html`
+                <moz-checkbox
+                  slot="nested"
+                  type="checkbox"
+                  id="hide-sidebar"
+                  name="hideSidebar"
+                  data-l10n-id="sidebar-hide-tabs-and-sidebar"
+                  @change=${this.#handleVisibilityChange}
+                  ?checked=${this.visibility == "hide-sidebar"}
+                ></moz-checkbox>
+              `
+            )}
+          </moz-checkbox>
+        </moz-fieldset>
+        <moz-fieldset class="customize-group medium-top-margin no-label">
+          <moz-checkbox
+            type="checkbox"
+            id="position"
+            name="position"
+            data-l10n-id=${document.dir == "rtl" ? "sidebar-show-on-the-left" : "sidebar-show-on-the-right"}
+            @change=${this.reversePosition}
+            ?checked=${!this.getWindow().SidebarController._positionStart}
+        ></moz-checkbox>
+        </moz-fieldset>
         <moz-fieldset class="customize-group" data-l10n-id="sidebar-customize-firefox-tools-header">
           ${this.getWindow()
             .SidebarController.getTools()
@@ -262,91 +303,6 @@ export class SidebarCustomize extends SidebarPage {
               </div>
             </div>`
         )}
-        ${when(
-          this.verticalTabsEnabled,
-          () =>
-            html`<div class="customize-group">
-              <moz-radio-group
-                @change=${this.#handleVisibilityChange}
-                name="visibility"
-                data-l10n-id="sidebar-customize-button-header"
-              >
-                <moz-radio
-                  class="visibility-setting"
-                  value="always-show"
-                  ?checked=${this.visibility === "always-show"}
-                  iconsrc="chrome://browser/skin/sidebar-expanded.svg"
-                  data-l10n-id="sidebar-visibility-setting-always-show"
-                ></moz-radio>
-                <moz-radio
-                  class="visibility-setting"
-                  value="hide-sidebar"
-                  ?checked=${this.visibility === "hide-sidebar"}
-                  iconsrc="chrome://browser/skin/sidebar-hidden.svg"
-                  data-l10n-id="sidebar-visibility-setting-hide-sidebar"
-                ></moz-radio>
-              </moz-radio-group>
-            </div>`
-        )}
-        <div class="customize-group">
-          <moz-radio-group
-              @change=${this.reversePosition}
-              name="position"
-              data-l10n-id="sidebar-customize-position-header">
-            <moz-radio
-              class="position-setting"
-              id="position-left"
-              value=${!this.getWindow().RTL_UI}
-              ?checked=${
-                this.getWindow().RTL_UI
-                  ? !this.getWindow().SidebarController._positionStart
-                  : this.getWindow().SidebarController._positionStart
-              }
-              iconsrc="chrome://browser/skin/sidebar-expanded.svg"
-              data-l10n-id="sidebar-position-left"
-            ></moz-radio>
-            <moz-radio
-              class="position-setting"
-              id="position-right"
-              value=${this.getWindow().RTL_UI}
-              ?checked=${
-                this.getWindow().RTL_UI
-                  ? this.getWindow().SidebarController._positionStart
-                  : !this.getWindow().SidebarController._positionStart
-              }
-              iconsrc="chrome://browser/skin/sidebar-expanded-right.svg"
-              data-l10n-id="sidebar-position-right"
-            ></moz-radio>
-          </moz-radio-group>
-        </div>
-        <div class="customize-group">
-          <moz-radio-group
-              @change=${this.#handleTabDirectionChange}
-              name="tabDirection"
-              data-l10n-id="sidebar-customize-tabs-header">
-            <moz-radio
-              class="vertical-tabs-setting"
-              id="vertical-tabs"
-              value=${true}
-              ?checked=${
-                this.getWindow().SidebarController.sidebarVerticalTabsEnabled
-              }
-              iconsrc="chrome://browser/skin/sidebar-collapsed.svg"
-              data-l10n-id="sidebar-vertical-tabs"
-            ></moz-radio>
-            <moz-radio
-              class="vertical-tabs-setting"
-              id="horizontal-tabs"
-              value=${false}
-              ?checked=${
-                this.getWindow().SidebarController
-                  .sidebarVerticalTabsEnabled === false
-              }
-              iconsrc="chrome://browser/skin/sidebar-horizontal-tabs.svg"
-              data-l10n-id="sidebar-horizontal-tabs"
-            ></moz-radio>
-          </moz-radio-group>
-        </div>
         <div id="manage-settings">
           <img src="chrome://browser/skin/preferences/category-general.svg" class="icon" role="presentation" />
           <a
@@ -361,16 +317,20 @@ export class SidebarCustomize extends SidebarPage {
     `;
   }
 
-  #handleVisibilityChange({ target: { value } }) {
+  #handleVisibilityChange(e) {
-    this.visibility = value;
+    e.stopPropagation();
-    Services.prefs.setStringPref(VISIBILITY_SETTING_PREF, value);
+    this.visibility = e.target.checked ? "hide-sidebar" : "always-show";
+    Services.prefs.setStringPref(
+      VISIBILITY_SETTING_PREF,
+      e.target.checked ? "hide-sidebar" : "always-show"
+    );
     Glean.sidebarCustomize.sidebarDisplay.record({
-      preference: value === "always-show" ? "always" : "hide",
+      preference: e.target.checked ? "hide" : "always",
     });
   }
 
-  #handleTabDirectionChange({ target: { value } }) {
+  #handleTabDirectionChange({ target: { checked } }) {
-    const verticalTabsEnabled = value === "true";
+    const verticalTabsEnabled = checked;
     Services.prefs.setBoolPref(TAB_DIRECTION_SETTING_PREF, verticalTabsEnabled);
     Glean.sidebarCustomize.tabsLayout.record({
       orientation: verticalTabsEnabled ? "vertical" : "horizontal",

browser/components/sidebar/tests/browser/browser_customize_sidebar.js

@@ -13,7 +13,11 @@ async function showCustomizePanel(win) {
   const document = win.SidebarController.browser.contentDocument;
   return TestUtils.waitForCondition(async () => {
     const component = document.querySelector("sidebar-customize");
-    if (!component?.positionInputs || !component?.visibilityInputs) {
+    if (
+      !component?.positionInput ||
+      (win.SidebarController.sidebarVerticalTabsEnabled &&
+        !component?.visibilityInput)
+    ) {
       return false;
     }
     return component;
@@ -170,19 +174,22 @@ add_task(async function test_customize_position_setting() {
     () => BrowserTestUtils.isVisible(sidebarBox),
     "Sidebar panel is visible"
   );
-  const [positionLeft, positionRight] = panel.positionInputs;
+
-  ok(positionLeft.checked, "The sidebar positioned on the left by default.");
+  ok(
+    !panel.positionInput.checked,
+    "The sidebar positioned on the left by default."
+  );
   is(
     sidebarBox.style.order,
     "3",
     "Sidebar box should have an order of 3 when on the left"
   );
   EventUtils.synthesizeMouseAtCenter(
-    positionRight,
+    panel.positionInput,
     {},
     win.SidebarController.browser.contentWindow
   );
-  ok(positionRight.checked, "Sidebar is positioned on the right");
+  ok(panel.positionInput.checked, "Sidebar is positioned on the right");
 
   const newWin = await BrowserTestUtils.openNewBrowserWindow();
   const newPanel = await showCustomizePanel(newWin);
@@ -191,8 +198,8 @@ add_task(async function test_customize_position_setting() {
     () => BrowserTestUtils.isVisible(newSidebarBox),
     "Sidebar panel is visible"
   );
-  const [, newPositionRight] = newPanel.positionInputs;
+
-  ok(newPositionRight.checked, "Position setting persists.");
+  ok(newPanel.positionInput.checked, "Position setting persists.");
   is(
     newSidebarBox.style.order,
     "5",
@@ -217,22 +224,20 @@ add_task(async function test_customize_visibility_setting() {
 
   const win = await BrowserTestUtils.openNewBrowserWindow();
   const panel = await showCustomizePanel(win);
-  const [showInput, hideInput] = panel.visibilityInputs;
+  ok(!panel.visibilityInput.checked, "Always show is enabled by default.");
-  ok(showInput.checked, "Always show is enabled by default.");
   EventUtils.synthesizeMouseAtCenter(
-    hideInput,
+    panel.visibilityInput,
     {},
     win.SidebarController.browser.contentWindow
   );
-  ok(hideInput.checked, "Hide sidebar is enabled.");
+  ok(panel.visibilityInput.checked, "Hide sidebar is enabled.");
   await deferredPrefChange.promise;
   const newPrefValue = Services.prefs.getStringPref(SIDEBAR_VISIBILITY_PREF);
   is(newPrefValue, "hide-sidebar", "Visibility preference updated.");
 
   const newWin = await BrowserTestUtils.openNewBrowserWindow();
   const newPanel = await showCustomizePanel(newWin);
-  const [, newHideInput] = newPanel.visibilityInputs;
+  ok(newPanel.visibilityInput.checked, "Visibility setting persists.");
-  ok(newHideInput.checked, "Visibility setting persists.");
 
   await BrowserTestUtils.closeWindow(win);
   await BrowserTestUtils.closeWindow(newWin);
@@ -251,22 +256,23 @@ add_task(async function test_vertical_tabs_setting() {
 
   const win = await BrowserTestUtils.openNewBrowserWindow();
   const panel = await showCustomizePanel(win);
-  const [verticalTabs, horizontalTabs] = panel.verticalTabsInputs;
+  ok(
-  ok(horizontalTabs.checked, "Horizontal tabs is enabled by default.");
+    !panel.verticalTabsInput.checked,
+    "Horizontal tabs is enabled by default."
+  );
   EventUtils.synthesizeMouseAtCenter(
-    verticalTabs,
+    panel.verticalTabsInput,
     {},
     win.SidebarController.browser.contentWindow
   );
-  ok(verticalTabs.checked, "Vertical tabs is enabled.");
+  ok(panel.verticalTabsInput.checked, "Vertical tabs is enabled.");
   await deferredPrefChange.promise;
   const newPrefValue = Services.prefs.getBoolPref(TAB_DIRECTION_PREF);
   is(newPrefValue, true, "Vertical tabs pref updated.");
 
   const newWin = await BrowserTestUtils.openNewBrowserWindow();
   const newPanel = await showCustomizePanel(newWin);
-  const [newVerticalTabs] = newPanel.verticalTabsInputs;
+  ok(newPanel.verticalTabsInput.checked, "Vertical tabs setting persists.");
-  ok(newVerticalTabs.checked, "Vertical tabs setting persists.");
 
   await BrowserTestUtils.closeWindow(win);
   await BrowserTestUtils.closeWindow(newWin);

browser/components/sidebar/tests/browser/browser_glean_sidebar.js

@@ -366,21 +366,18 @@ add_task(async function test_customize_extensions_clicked() {
 });
 
 async function testCustomizeSetting(
-  inputs,
+  input,
   gleanEventOrMetric,
   firstExpected,
-  secondExpected,
+  secondExpected
-  reverseInputsOrder
 ) {
   await SidebarController.show("viewCustomizeSidebar");
-  const { contentDocument, contentWindow } = SidebarController.browser;
+  const { contentDocument } = SidebarController.browser;
   const component = contentDocument.querySelector("sidebar-customize");
-  const [firstInput, secondInput] = reverseInputsOrder
-    ? [...component[inputs]].reverse()
-    : component[inputs];
 
-  info(`Toggle the setting for ${inputs}.`);
+  info(`Toggle the setting for ${input}.`);
-  EventUtils.synthesizeMouseAtCenter(firstInput, {}, contentWindow);
+  // Use click as the rect in synthesizeMouseAtCenter can change for the vertical tabs toggle
+  component[input].click();
   await TestUtils.waitForTick();
   let value = gleanEventOrMetric.testGetValue();
   if (Array.isArray(value)) {
@@ -390,7 +387,8 @@ async function testCustomizeSetting(
     Assert.equal(value, firstExpected);
   }
 
-  EventUtils.synthesizeMouseAtCenter(secondInput, {}, contentWindow);
+  info(`Toggle the setting for ${input}.`);
+  component[input].click();
   await TestUtils.waitForTick();
   value = gleanEventOrMetric.testGetValue();
   if (Array.isArray(value)) {
@@ -408,32 +406,29 @@ add_task(async function test_customize_sidebar_display() {
     set: [[TAB_DIRECTION_PREF, true]],
   });
   await testCustomizeSetting(
-    "visibilityInputs",
+    "visibilityInput",
     Glean.sidebarCustomize.sidebarDisplay,
     { preference: "hide" },
-    { preference: "always" },
+    { preference: "always" }
-    true
   );
   await SpecialPowers.popPrefEnv();
 });
 
 add_task(async function test_customize_sidebar_position() {
   await testCustomizeSetting(
-    "positionInputs",
+    "positionInput",
     Glean.sidebarCustomize.sidebarPosition,
     { position: "right" },
-    { position: "left" },
+    { position: "left" }
-    true
   );
 });
 
 add_task(async function test_customize_tabs_layout() {
   await testCustomizeSetting(
-    "verticalTabsInputs",
+    "verticalTabsInput",
     Glean.sidebarCustomize.tabsLayout,
     { orientation: "vertical" },
-    { orientation: "horizontal" },
+    { orientation: "horizontal" }
-    false
   );
 });
 
@@ -484,32 +479,29 @@ add_task(async function test_sidebar_display_settings() {
     set: [[TAB_DIRECTION_PREF, true]],
   });
   await testCustomizeSetting(
-    "visibilityInputs",
+    "visibilityInput",
     Glean.sidebar.displaySettings,
     "hide",
-    "always",
+    "always"
-    true
   );
   await SpecialPowers.popPrefEnv();
 });
 
 add_task(async function test_sidebar_position_settings() {
   await testCustomizeSetting(
-    "positionInputs",
+    "positionInput",
     Glean.sidebar.positionSettings,
     "right",
-    "left",
+    "left"
-    true
   );
 });
 
 add_task(async function test_sidebar_tabs_layout() {
   await testCustomizeSetting(
-    "verticalTabsInputs",
+    "verticalTabsInput",
     Glean.sidebar.tabsLayout,
     "vertical",
-    "horizontal",
+    "horizontal"
-    false
   );
 });
 
@@ -522,13 +514,13 @@ add_task(async function test_sidebar_position_rtl_ui() {
   // When RTL is enabled, sidebar is shown on the right by default.
   // Toggle position setting to move it to the left, then back to the right.
   await testCustomizeSetting(
-    "positionInputs",
+    "positionInput",
     Glean.sidebarCustomize.sidebarPosition,
     { position: "left" },
     { position: "right" }
   );
   await testCustomizeSetting(
-    "positionInputs",
+    "positionInput",
     Glean.sidebar.positionSettings,
     "left",
     "right"

browser/locales/en-US/browser/sidebar.ftl

@@ -48,24 +48,8 @@ sidebar-customize-extensions-header = Sidebar extensions
 sidebar-customize-firefox-tools-header =
   .label = { -brand-product-name } tools
 sidebar-customize-firefox-settings = Manage { -brand-short-name } settings
-sidebar-position-left =
-  .label = Show on the left
-sidebar-position-right =
-  .label = Show on the right
 sidebar-vertical-tabs =
   .label = Vertical tabs
-sidebar-horizontal-tabs =
-  .label = Horizontal tabs
-sidebar-customize-tabs-header =
-  .label = Tab settings
-sidebar-customize-button-header =
-  .label = Sidebar button
-sidebar-customize-position-header =
-  .label = Sidebar position
-sidebar-visibility-setting-always-show =
-  .label = Expand and collapse sidebar
-sidebar-visibility-setting-hide-sidebar =
-  .label = Show and hide sidebar
 sidebar-settings =
   .label = Sidebar settings
 sidebar-hide-tabs-and-sidebar =

browser/themes/shared/icons/sidebar-hidden.svg

@@ -1,6 +0,0 @@
-<!-- This Source Code Form is subject to the terms of the Mozilla Public
-   - License, v. 2.0. If a copy of the MPL was not distributed with this
-   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
-<svg width="16" height="16" viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="context-fill" fill-opacity="context-fill-opacity">
-<path fill-rule="evenodd" clip-rule="evenodd" d="M2 3.5C1.72421 3.5 1.5 3.72421 1.5 4V12C1.5 12.2758 1.72421 12.5 2 12.5H14C14.2758 12.5 14.5 12.2758 14.5 12V4C14.5 3.72421 14.2758 3.5 14 3.5H2ZM0 4C0 2.89579 0.895786 2 2 2H14C15.1042 2 16 2.89579 16 4V12C16 13.1042 15.1042 14 14 14H2C0.895786 14 0 13.1042 0 12V4Z"/>
-</svg>

browser/themes/shared/icons/sidebar-horizontal-tabs.svg

@@ -1,6 +0,0 @@
-<!-- This Source Code Form is subject to the terms of the Mozilla Public
-   - License, v. 2.0. If a copy of the MPL was not distributed with this
-   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
-<svg width="16" height="16" fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg">
-<path fill-rule="evenodd" clip-rule="evenodd" d="M2 2a2 2 0 0 0-2 2v8a2 2 0 0 0 2 2h12a2 2 0 0 0 2-2V4a2 2 0 0 0-2-2H2zm-.5 2a.5.5 0 0 1 .5-.5h3.5v2A.5.5 0 0 0 6 6h8.5v6a.5.5 0 0 1-.5.5H2a.5.5 0 0 1-.5-.5V4z"/>
-</svg>

browser/themes/shared/jar.inc.mn

@@ -201,8 +201,6 @@
   skin/classic/browser/sidebar-collapsed-right.svg    (../shared/icons/sidebar-collapsed-right.svg)
   skin/classic/browser/sidebar-expanded.svg           (../shared/icons/sidebar-expanded.svg)
   skin/classic/browser/sidebar-expanded-right.svg     (../shared/icons/sidebar-expanded-right.svg)
-  skin/classic/browser/sidebar-hidden.svg             (../shared/icons/sidebar-hidden.svg)
-  skin/classic/browser/sidebar-horizontal-tabs.svg    (../shared/icons/sidebar-horizontal-tabs.svg)
   skin/classic/browser/sidebars.svg                   (../shared/icons/sidebars.svg)
   skin/classic/browser/sidebars-right.svg             (../shared/icons/sidebars-right.svg)
 #ifndef MOZ_WIDGET_GTK