Bug 1935432 - Fix customize panel not being in sync with prefs. r=sidebar-reviewers,kcochrane

This fixes the "actual" issue as we're leaning towards placing the refactoring bits on hold. It should be straightforward to rework SidebarCustomize in the future, should we decide that prefs ought to be read from a different module. Differential Revision: https://phabricator.services.mozilla.com/D234509
2025-01-23 13:15:15 +00:00
parent 5b20602824
commit 38276f5013
3 changed files with 107 additions and 50 deletions
--- a/browser/components/sidebar/sidebar-customize.mjs
+++ b/browser/components/sidebar/sidebar-customize.mjs
@@ -9,6 +9,10 @@ import { SidebarPage } from "./sidebar-page.mjs";
 // eslint-disable-next-line import/no-unassigned-import
 import "chrome://global/content/elements/moz-radio-group.mjs";

+const { XPCOMUtils } = ChromeUtils.importESModule(
+  "resource://gre/modules/XPCOMUtils.sys.mjs"
+);
+
 const l10nMap = new Map([
  ["viewGenaiChatSidebar", "sidebar-menu-genai-chat-label"],
  ["viewReviewCheckerSidebar", "sidebar-menu-review-checker-label"],
@@ -17,22 +21,53 @@ const l10nMap = new Map([
  ["viewBookmarksSidebar", "sidebar-menu-bookmarks-label"],
 ]);
 const VISIBILITY_SETTING_PREF = "sidebar.visibility";
+const POSITION_SETTING_PREF = "sidebar.position_start";
 const TAB_DIRECTION_SETTING_PREF = "sidebar.verticalTabs";

 export class SidebarCustomize extends SidebarPage {
  constructor() {
    super();
    this.activeExtIndex = 0;
-    this.visibility = Services.prefs.getStringPref(
+    XPCOMUtils.defineLazyPreferenceGetter(
+      this.#prefValues,
+      "visibility",
      VISIBILITY_SETTING_PREF,
-      "always-show"
+      "always-show",
+      (_aPreference, _previousValue, newValue) => {
+        this.visibility = newValue;
+      }
    );
+    XPCOMUtils.defineLazyPreferenceGetter(
+      this.#prefValues,
+      "isPositionStart",
+      POSITION_SETTING_PREF,
+      true,
+      (_aPreference, _previousValue, newValue) => {
+        this.isPositionStart = newValue;
+      }
+    );
+    XPCOMUtils.defineLazyPreferenceGetter(
+      this.#prefValues,
+      "isVerticalTabs",
+      TAB_DIRECTION_SETTING_PREF,
+      false,
+      (_aPreference, _previousValue, newValue) => {
+        this.isVerticalTabs = newValue;
+      }
+    );
+    this.visibility = this.#prefValues.visibility;
+    this.isPositionStart = this.#prefValues.isPositionStart;
+    this.isVerticalTabs = this.#prefValues.isVerticalTabs;
    this.boundObserve = (...args) => this.observe(...args);
  }

+  #prefValues = {};
+
  static properties = {
    activeExtIndex: { type: Number },
    visibility: { type: String },
+    isPositionStart: { type: Boolean },
+    isVerticalTabs: { type: Boolean },
  };

  static queries = {
@@ -48,7 +83,6 @@ export class SidebarCustomize extends SidebarPage {
    this.getWindow().addEventListener("SidebarItemAdded", this);
    this.getWindow().addEventListener("SidebarItemChanged", this);
    this.getWindow().addEventListener("SidebarItemRemoved", this);
-    Services.prefs.addObserver(VISIBILITY_SETTING_PREF, this.boundObserve);
  }

  disconnectedCallback() {
@@ -56,26 +90,6 @@ export class SidebarCustomize extends SidebarPage {
    this.getWindow().removeEventListener("SidebarItemAdded", this);
    this.getWindow().removeEventListener("SidebarItemChanged", this);
    this.getWindow().removeEventListener("SidebarItemRemoved", this);
-    Services.prefs.removeObserver(VISIBILITY_SETTING_PREF, this.boundObserve);
-  }
-
-  observe(subject, topic, prefName) {
-    switch (topic) {
-      case "nsPref:changed":
-        switch (prefName) {
-          case VISIBILITY_SETTING_PREF:
-            this.visibility = Services.prefs.getStringPref(
-              VISIBILITY_SETTING_PREF,
-              "always-show"
-            );
-            break;
-        }
-        break;
-    }
-  }
-
-  get sidebarLauncher() {
-    return this.getWindow().document.querySelector("sidebar-launcher");
  }

  getWindow() {
@@ -186,9 +200,7 @@ export class SidebarCustomize extends SidebarPage {
    SidebarController.reversePosition();
    Glean.sidebarCustomize.sidebarPosition.record({
      position:
-        SidebarController._positionStart !== this.getWindow().RTL_UI
-          ? "left"
-          : "right",
+        this.isPositionStart !== this.getWindow().RTL_UI ? "left" : "right",
    });
  }

@@ -245,6 +257,7 @@ export class SidebarCustomize extends SidebarPage {
          <moz-radio-group
            @change=${this.#handleVisibilityChange}
            name="visibility"
+            .value=${this.visibility}
            data-l10n-id="sidebar-customize-button-header"
          >
            <moz-radio
@@ -267,16 +280,13 @@ export class SidebarCustomize extends SidebarPage {
          <moz-radio-group
              @change=${this.reversePosition}
              name="position"
+              .value="${this.isPositionStart}"
              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
-              }
+              ?checked=${this.isPositionStart != this.getWindow().RTL_UI}
              iconsrc="chrome://browser/skin/sidebar-expanded.svg"
              data-l10n-id="sidebar-position-left"
            ></moz-radio>
@@ -284,11 +294,7 @@ export class SidebarCustomize extends SidebarPage {
              class="position-setting"
              id="position-right"
              value=${this.getWindow().RTL_UI}
-              ?checked=${
-                this.getWindow().RTL_UI
-                  ? this.getWindow().SidebarController._positionStart
-                  : !this.getWindow().SidebarController._positionStart
-              }
+              ?checked=${this.isPositionStart == this.getWindow().RTL_UI}
              iconsrc="chrome://browser/skin/sidebar-expanded-right.svg"
              data-l10n-id="sidebar-position-right"
            ></moz-radio>
@@ -298,14 +304,13 @@ export class SidebarCustomize extends SidebarPage {
          <moz-radio-group
              @change=${this.#handleTabDirectionChange}
              name="tabDirection"
+              .value=${this.isVerticalTabs}
              data-l10n-id="sidebar-customize-tabs-header">
            <moz-radio
              class="vertical-tabs-setting"
              id="vertical-tabs"
              value=${true}
-              ?checked=${
-                this.getWindow().SidebarController.sidebarVerticalTabsEnabled
-              }
+              ?checked=${this.isVerticalTabs}
              iconsrc="chrome://browser/skin/sidebar-collapsed.svg"
              data-l10n-id="sidebar-vertical-tabs"
            ></moz-radio>
@@ -313,10 +318,7 @@ export class SidebarCustomize extends SidebarPage {
              class="vertical-tabs-setting"
              id="horizontal-tabs"
              value=${false}
-              ?checked=${
-                this.getWindow().SidebarController
-                  .sidebarVerticalTabsEnabled === false
-              }
+              ?checked=${!this.isVerticalTabs}
              iconsrc="chrome://browser/skin/sidebar-horizontal-tabs.svg"
              data-l10n-id="sidebar-horizontal-tabs"
            ></moz-radio>
--- a/browser/components/sidebar/tests/browser/browser_customize_sidebar.js
+++ b/browser/components/sidebar/tests/browser/browser_customize_sidebar.js
@@ -6,8 +6,15 @@
 requestLongerTimeout(2);

 const SIDEBAR_VISIBILITY_PREF = "sidebar.visibility";
+const POSITION_SETTING_PREF = "sidebar.position_start";
 const TAB_DIRECTION_PREF = "sidebar.verticalTabs";

+registerCleanupFunction(() => {
+  Services.prefs.clearUserPref(SIDEBAR_VISIBILITY_PREF);
+  Services.prefs.clearUserPref(POSITION_SETTING_PREF);
+  Services.prefs.clearUserPref(TAB_DIRECTION_PREF);
+});
+
 async function showCustomizePanel(win) {
  await win.SidebarController.show("viewCustomizeSidebar");
  const document = win.SidebarController.browser.contentDocument;
@@ -292,3 +299,49 @@ add_task(async function test_keyboard_navigation_away_from_settings_link() {

  await BrowserTestUtils.closeWindow(win);
 });
+
+add_task(async function test_settings_synchronized_across_windows() {
+  const panel = await showCustomizePanel(window);
+  const { contentWindow } = SidebarController.browser;
+  const newWindow = await BrowserTestUtils.openNewBrowserWindow();
+  const newPanel = await showCustomizePanel(newWindow);
+
+  info("Update visibility settings.");
+  EventUtils.synthesizeMouseAtCenter(
+    panel.visibilityInputs[1],
+    {},
+    contentWindow
+  );
+  await newPanel.updateComplete;
+  ok(
+    newPanel.visibilityInputs[1].checked,
+    "New window shows the updated visibility setting."
+  );
+
+  info("Update position settings.");
+  EventUtils.synthesizeMouseAtCenter(
+    panel.positionInputs[1],
+    {},
+    contentWindow
+  );
+  await newPanel.updateComplete;
+  ok(
+    newPanel.positionInputs[1].checked,
+    "New window shows the updated position setting."
+  );
+
+  info("Update vertical tabs settings.");
+  EventUtils.synthesizeMouseAtCenter(
+    panel.verticalTabsInputs[0],
+    {},
+    contentWindow
+  );
+  await newPanel.updateComplete;
+  ok(
+    newPanel.verticalTabsInputs[0].checked,
+    "New window shows the vertical tabs setting."
+  );
+
+  SidebarController.hide();
+  await BrowserTestUtils.closeWindow(newWindow);
+});
--- a/browser/components/sidebar/tests/unit/test_sidebar_state.js
+++ b/browser/components/sidebar/tests/unit/test_sidebar_state.js
@@ -11,17 +11,19 @@ const { SidebarState } = ChromeUtils.importESModule(
  "resource:///modules/SidebarState.sys.mjs"
 );

-const mockElement = { toggleAttribute: sinon.stub() };
+const mockElement = {
+  setAttribute(name, value) {
+    this[name] = value;
+  },
+  style: { width: "200px" },
+  toggleAttribute: sinon.stub(),
+};
 const mockGlobal = {
  document: { getElementById: () => mockElement },
  gBrowser: { tabContainer: mockElement },
 };
-const mockPanel = {
-  setAttribute: (name, value) => (mockPanel[name] = value),
-  style: { width: "200px" },
-};
 const mockController = {
-  _box: mockPanel,
+  _box: mockElement,
  showInitially: sinon.stub(),
  sidebarContainer: { ownerGlobal: mockGlobal },
  sidebarMain: mockElement,