From 5fbb160ed8af0af503b40c0c84edfc669c6ab8ce Mon Sep 17 00:00:00 2001 From: Jonathan Sudiaman Date: Fri, 24 Jan 2025 17:14:01 +0000 Subject: [PATCH] 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 --- .../components/sidebar/sidebar-customize.mjs | 90 ++++++++++--------- .../browser/browser_customize_sidebar.js | 53 +++++++++++ .../sidebar/tests/unit/test_sidebar_state.js | 14 +-- 3 files changed, 107 insertions(+), 50 deletions(-) diff --git a/browser/components/sidebar/sidebar-customize.mjs b/browser/components/sidebar/sidebar-customize.mjs index e9315a4e2729..3808c41e9239 100644 --- 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 { @@ -267,16 +280,13 @@ export class SidebarCustomize extends SidebarPage { @@ -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" > @@ -298,14 +304,13 @@ export class SidebarCustomize extends SidebarPage { @@ -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" > diff --git a/browser/components/sidebar/tests/browser/browser_customize_sidebar.js b/browser/components/sidebar/tests/browser/browser_customize_sidebar.js index 5d7fc8f02ee3..c4d0c9527a51 100644 --- 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); +}); diff --git a/browser/components/sidebar/tests/unit/test_sidebar_state.js b/browser/components/sidebar/tests/unit/test_sidebar_state.js index 568c539d4bc2..8f7317b77c02 100644 --- 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,