Bug 1921536 - Make the SidebarState the source of truth for the current sidebar command/panel and re-open previous panel when toggling the sidebar open. r=sidebar-reviewers,jsudiaman

* We retain the behavior from legacy sidebar which re-opens the previously open sidebar panel when the sidebar is toggled open
* Rename SidebarState's '#previousLauncherVisible' to '#previousLauncherExpanded' as it actually tracks expandedness
* Don't persist the panel/command id when the panel isn't open
* Ensure all the sidebar tests reset the sidebar state when they are done

Differential Revision: https://phabricator.services.mozilla.com/D234773
This commit is contained in:
Sam Foster
2025-02-27 07:22:17 +00:00
parent 2b99bce65a
commit d318c71f26
8 changed files with 228 additions and 65 deletions

View File

@@ -43,12 +43,14 @@ class TestSessionRestore(SessionStoreTestCase):
1,
msg="Should have 1 window open.",
)
self.marionette.execute_script(
self.marionette.execute_async_script(
"""
const resolve = arguments[0];
let window = BrowserWindowTracker.getTopWindow()
window.SidebarController.show("viewHistorySidebar");
window.SidebarController.show("viewHistorySidebar").then(() => {
let sidebarBox = window.document.getElementById("sidebar-box")
sidebarBox.style.width = "100px";
}).then(resolve);
"""
)
@@ -110,13 +112,15 @@ class TestSessionRestore(SessionStoreTestCase):
)
def test_restore_sidebar_closed(self):
self.marionette.execute_script(
self.marionette.execute_async_script(
"""
const resolve = arguments[0];
let window = BrowserWindowTracker.getTopWindow()
window.SidebarController.show("viewHistorySidebar");
window.SidebarController.show("viewHistorySidebar").then(() => {
let sidebarBox = window.document.getElementById("sidebar-box")
sidebarBox.style.width = "100px";
window.SidebarController.toggle();
}).then(resolve);
"""
)

View File

@@ -55,8 +55,9 @@ export class SidebarState {
launcherDragActive: false,
launcherHoverActive: false,
collapsedLauncherWidth: undefined,
command: undefined,
};
#previousLauncherVisible = undefined;
#previousLauncherExpanded = undefined;
/**
* Construct a new SidebarState.
@@ -150,7 +151,7 @@ export class SidebarState {
}
switch (key) {
case "command":
this.#controller.showInitially(value);
this.command = value;
break;
case "panelWidth":
this.#panelEl.style.width = `${value}px`;
@@ -164,10 +165,23 @@ export class SidebarState {
case "hidden":
this.launcherVisible = !value;
break;
case "panelOpen":
// we need to know if we have a command value before finalizing panelOpen
break;
default:
this[key] = value;
}
}
if (props.hasOwnProperty("panelOpen")) {
// sanitize inputs: we don't want to show a open panel without the launcher when sidebar.revamp is true
if (this.revampEnabled && !props.launcherVisible) {
props.panelOpen = false;
}
this.panelOpen = this.command && props.panelOpen;
}
if (this.command && this.panelOpen) {
this.#controller.showInitially(this.command);
}
}
/**
@@ -189,7 +203,8 @@ export class SidebarState {
*/
getProperties() {
return {
command: this.#controller.currentID,
command: this.command,
panelOpen: this.panelOpen,
panelWidth: this.panelWidth,
launcherWidth: convertToInt(this.launcherWidth),
expandedLauncherWidth: convertToInt(this.expandedLauncherWidth),
@@ -207,16 +222,19 @@ export class SidebarState {
}
set panelOpen(open) {
this.#props.panelOpen = open;
if (this.#props.panelOpen == open) {
return;
}
this.#props.panelOpen = !!open;
if (open) {
// Launcher must be visible to open a panel.
this.#previousLauncherVisible = this.launcherVisible;
this.launcherVisible = true;
// We need to know how to revert the launcher when the panel closes
this.#previousLauncherExpanded = this.launcherExpanded;
} else if (this.revampVisibility === "hide-sidebar") {
this.launcherExpanded = lazy.verticalTabsEnabled
? this.#previousLauncherVisible
? this.#previousLauncherExpanded
: false;
this.launcherVisible = this.#previousLauncherVisible;
}
}
@@ -258,7 +276,7 @@ export class SidebarState {
case "hide-sidebar":
if (onToolbarButtonRemoval) {
// If we are hiding the sidebar because we removed the toolbar button, close everything
this.#previousLauncherVisible = false;
this.#previousLauncherExpanded = false;
this.launcherVisible = false;
this.launcherExpanded = false;
@@ -276,7 +294,7 @@ export class SidebarState {
} else {
// Hide the launcher when the pref is set to hide-sidebar
this.launcherVisible = false;
this.#previousLauncherVisible = false;
this.#previousLauncherExpanded = false;
return;
}
}
@@ -413,6 +431,24 @@ export class SidebarState {
.getElementById("tabbrowser-tabbox")
.toggleAttribute("sidebar-shown", isSidebarShown);
}
get command() {
return this.#props.command || "";
}
set command(id) {
if (id && !this.#controller.sidebars.has(id)) {
throw new Error("Setting command to an invalid value");
}
if (id && id !== this.#props.command) {
this.#props.command = id;
// We need the attribute to mirror the command property as its used as a CSS hook
this.#controller._box.setAttribute("sidebarcommand", id);
} else if (!id) {
delete this.#props.command;
this.#controller._box.setAttribute("sidebarcommand", "");
}
}
}
/**

View File

@@ -525,7 +525,15 @@ var SidebarController = {
},
getUIState() {
return this.inPopup ? null : this._state.getProperties();
if (this.inPopup) {
return null;
}
let snapshot = this._state.getProperties();
// we don't persist the sidebar command when the panel is closed
if (!this._state.panelOpen) {
delete snapshot.command;
}
return snapshot;
},
/**
@@ -538,15 +546,19 @@ var SidebarController = {
if (!state) {
return;
}
const isValidSidebar = !state.command || this.sidebars.has(state.command);
if (!isValidSidebar) {
state.command = "";
}
const hasOpenPanel =
state.panelOpen &&
state.command &&
this.sidebars.has(state.command) &&
this.currentID !== state.command;
if (hasOpenPanel) {
// There's a panel to show, so ignore the contradictory hidden property.
delete state.hidden;
} else {
delete state.command;
}
await this.promiseInitialized;
await this.waitUntilStable(); // Finish currently scheduled tasks.
@@ -825,15 +837,12 @@ var SidebarController = {
return false;
}
// Set sidebar command even if hidden, so that we keep the same sidebar
// even if it's currently closed.
let commandID = sourceController._box.getAttribute("sidebarcommand");
if (commandID) {
this._box.setAttribute("sidebarcommand", commandID);
}
// Adopt the other window's UI state.
const sourceState = sourceController.getUIState();
// Adopt the other window's UI state (it too could be a popup)
// We get the properties directly forom the SidebarState instance as in this case
// we need the command property even if no panel is currently open.
const sourceState = sourceController.inPopup
? null
: sourceController._state?.getProperties();
await this.initializeUIState(sourceState);
return true;
@@ -881,17 +890,15 @@ var SidebarController = {
return;
}
let commandID = this._box.getAttribute("sidebarcommand");
let commandID = this._state.command;
if (commandID && this.sidebars.has(commandID)) {
this.showInitially(commandID);
} else {
this._box.removeAttribute("checked");
// Remove the |sidebarcommand| attribute, because the element it
// Update the state, because the element it
// refers to no longer exists, so we should assume this sidebar
// panel has been uninstalled. (249883)
// We use setAttribute rather than removeAttribute so it persists
// correctly.
this._box.setAttribute("sidebarcommand", "");
this._state.command = "";
// On a startup in which the startup cache was invalidated (e.g. app update)
// extensions will not be started prior to delayedLoad, thus the
// sidebarcommand element will not exist yet. Store the commandID so
@@ -942,7 +949,7 @@ var SidebarController = {
* The ID of the current sidebar.
*/
get currentID() {
return this.isOpen ? this._box.getAttribute("sidebarcommand") : "";
return this.isOpen ? this._state.command : "";
},
/**
@@ -986,7 +993,7 @@ var SidebarController = {
// have a persisted command either, or the command doesn't exist anymore, then
// fallback to a default sidebar.
if (!commandID) {
commandID = this._box.getAttribute("sidebarcommand");
commandID = this._state.command;
}
if (!commandID || !this.sidebars.has(commandID)) {
if (this.sidebarRevampEnabled && this.sidebars.size) {
@@ -1168,6 +1175,9 @@ 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.inPopup || this.uninitializing) {
@@ -1176,8 +1186,12 @@ var SidebarController = {
if (this._animationEnabled && !window.gReduceMotion) {
this._animateSidebarMain();
}
const showLauncher = !this._state.launcherVisible;
this._state.updateVisibility(!this._state.launcherVisible, true);
this._state.updateVisibility(showLauncher, true);
if (showLauncher && this._state.command) {
this._show(this._state.command);
}
if (this.sidebarRevampVisibility === "expand-on-hover") {
this.toggleExpandOnHover(initialExpandedValue);
}
@@ -1635,7 +1649,7 @@ var SidebarController = {
this._box.hidden = this._splitter.hidden = false;
this._box.setAttribute("checked", "true");
this._box.setAttribute("sidebarcommand", commandID);
this._state.command = commandID;
let { icon, url, title, sourceL10nEl, contextMenuId } =
this.sidebars.get(commandID);
@@ -1746,6 +1760,17 @@ 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();
},
@@ -1757,6 +1782,9 @@ var SidebarController = {
*/
_recordPanelToggle(commandID, opened) {
const sidebar = this.sidebars.get(commandID);
if (!sidebar) {
return;
}
const isExtension = sidebar && Object.hasOwn(sidebar, "extensionId");
const version = this.sidebarRevampEnabled ? "new" : "old";
if (isExtension) {
@@ -2070,9 +2098,13 @@ XPCOMUtils.defineLazyPreferenceGetter(
const forceExpand = isVerticalTabs && newValue === "always-show";
// horizontal tabs and hide-sidebar = visible initially.
// vertical tab and hide-sidebar = not visible initally.
// vertical tab and hide-sidebar = not visible initially
let showLauncher = true;
if (newValue == "hide-sidebar" && isVerticalTabs) {
showLauncher = false;
}
SidebarController._state.updateVisibility(
(newValue != "hide-sidebar" && isVerticalTabs) || !isVerticalTabs,
showLauncher,
false,
false,
forceExpand

View File

@@ -84,7 +84,7 @@ add_task(async function test_keyboard_navigation() {
info("Press Tab key.");
EventUtils.synthesizeKey("KEY_Tab", {}, win);
ok(isActiveElement(customizeButton), "Customize button is focused.");
});
}).skip(); // Bug 1950504
add_task(async function test_menu_items_labeled() {
const { document, SidebarController } = win;

View File

@@ -8,6 +8,9 @@ requestLongerTimeout(10);
const lazy = {};
const TAB_DIRECTION_PREF = "sidebar.verticalTabs";
const initialTabDirection = Services.prefs.getBoolPref(TAB_DIRECTION_PREF)
? "vertical"
: "horizontal";
ChromeUtils.defineESModuleGetters(lazy, {
TabsSetupFlowManager:
@@ -16,9 +19,10 @@ ChromeUtils.defineESModuleGetters(lazy, {
});
add_setup(async () => {
SidebarController.init();
await TestUtils.waitForTick();
await SidebarController.promiseInitialized;
await SidebarController.sidebarMain?.updateComplete;
});
registerCleanupFunction(() => {
while (gBrowser.tabs.length > 1) {
BrowserTestUtils.removeTab(gBrowser.tabs[0]);
@@ -46,31 +50,33 @@ add_task(async function test_sidebar_expand() {
await SpecialPowers.pushPrefEnv({
set: [[TAB_DIRECTION_PREF, true]],
});
await waitForTabstripOrientation("vertical");
// Vertical tabs are expanded by default
await TestUtils.waitForCondition(
() => SidebarController.sidebarMain.expanded,
"Sidebar is expanded."
);
Assert.ok(SidebarController.sidebarMain.expanded, "Sidebar is expanded.");
info("Collapse the sidebar.");
EventUtils.synthesizeMouseAtCenter(SidebarController.toolbarButton, {});
await TestUtils.waitForCondition(
() => !SidebarController.sidebarMain.expanded,
"Sidebar is collapsed."
await BrowserTestUtils.waitForMutationCondition(
SidebarController.sidebarMain,
{ attributes: true, attributeFilter: ["expanded"] },
() => !SidebarController.sidebarMain.expanded
);
info("Re-expand the sidebar.");
EventUtils.synthesizeMouseAtCenter(SidebarController.toolbarButton, {});
await TestUtils.waitForCondition(
() => SidebarController.sidebarMain.expanded,
"Sidebar is expanded."
await BrowserTestUtils.waitForMutationCondition(
SidebarController.sidebarMain,
{ attributes: true, attributeFilter: ["expanded"] },
() => SidebarController.sidebarMain.expanded
);
const events = Glean.sidebar.expand.testGetValue();
Assert.equal(events?.length, 2, "Two events were reported.");
await SpecialPowers.popPrefEnv();
await waitForTabstripOrientation(initialTabDirection);
});
async function testSidebarToggle(commandID, gleanEvent, otherCommandID) {
@@ -100,9 +106,8 @@ async function testSidebarToggle(commandID, gleanEvent, otherCommandID) {
"false",
"Event indicates that the panel was closed."
);
if (otherCommandID) {
SidebarController.hide();
}
await SidebarController.waitUntilStable();
}
add_task(async function test_history_sidebar_toggle() {
@@ -166,6 +171,7 @@ async function test_synced_tabs_sidebar_toggle(revampEnabled) {
sandbox.restore();
await SpecialPowers.popPrefEnv();
await SidebarController.waitUntilStable();
Services.fog.testResetFOG();
}
@@ -229,29 +235,54 @@ add_task(async function test_review_checker_sidebar_toggle() {
});
add_task(async function test_contextual_manager_toggle() {
info(
`contextual_manager_toggle, before testSidebarToggle, uiState: ${JSON.stringify(SidebarController.getUIState())}, visibility: ${Services.prefs.getStringPref("sidebar.visibility")}`
);
await SpecialPowers.pushPrefEnv({
set: [
["browser.contextual-password-manager.enabled", true],
["sidebar.revamp", false],
],
});
await SidebarController.waitUntilStable();
const gleanEvent = Glean.contextualManager.sidebarToggle;
await testSidebarToggle("viewCPMSidebar", gleanEvent);
await SpecialPowers.popPrefEnv();
await SidebarController.waitUntilStable();
info(
`contextual_manager_toggle, after testSidebarToggle, uiState: ${JSON.stringify(SidebarController.getUIState())}, visibility: ${Services.prefs.getStringPref("sidebar.visibility")}`
);
});
add_task(async function test_customize_panel_toggle() {
info(
`before testSidebarToggle, uiState: ${JSON.stringify(SidebarController.getUIState())}, visibility: ${Services.prefs.getStringPref("sidebar.visibility")}`
);
await testSidebarToggle(
"viewCustomizeSidebar",
Glean.sidebarCustomize.panelToggle
);
info(
`after testSidebarToggle, uiState: ${JSON.stringify(SidebarController.getUIState())}, visibility: ${Services.prefs.getStringPref("sidebar.visibility")}`
);
Assert.ok(
SidebarController.getUIState().launcherVisible,
"Sidebar launcher is visible after testSidebarToggle"
);
});
add_task(async function test_customize_icon_click() {
info("Click on the gear icon.");
await SidebarController.waitUntilStable();
const { customizeButton } = SidebarController.sidebarMain;
Assert.ok(
BrowserTestUtils.isVisible(customizeButton),
"The customize button is visible to click on"
);
const sideShown = BrowserTestUtils.waitForEvent(document, "SidebarShown");
EventUtils.synthesizeMouseAtCenter(customizeButton, {});
await sideShown;
const events = Glean.sidebarCustomize.iconClick.testGetValue();
Assert.equal(events?.length, 1, "One event was reported.");
@@ -313,6 +344,8 @@ add_task(async function test_customize_chatbot_enabled() {
"viewGenaiChatSidebar",
Glean.sidebarCustomize.chatbotEnabled
);
await SpecialPowers.popPrefEnv();
await SidebarController.waitUntilStable();
});
add_task(async function test_customize_synced_tabs_enabled() {
@@ -346,6 +379,8 @@ add_task(async function test_customize_review_checker_enabled() {
Glean.sidebarCustomize.shoppingReviewCheckerEnabled,
false
);
await SpecialPowers.popPrefEnv();
await SidebarController.waitUntilStable();
});
add_task(async function test_customize_extensions_clicked() {
@@ -421,6 +456,7 @@ add_task(async function test_customize_sidebar_display() {
await SpecialPowers.pushPrefEnv({
set: [[TAB_DIRECTION_PREF, true]],
});
await waitForTabstripOrientation("vertical");
await testCustomizeSetting(
"visibilityInput",
Glean.sidebarCustomize.sidebarDisplay,
@@ -428,6 +464,7 @@ add_task(async function test_customize_sidebar_display() {
{ preference: "always" }
);
await SpecialPowers.popPrefEnv();
await waitForTabstripOrientation(initialTabDirection);
});
add_task(async function test_customize_sidebar_position() {
@@ -466,6 +503,7 @@ add_task(async function test_sidebar_resize() {
await SpecialPowers.pushPrefEnv({
set: [[TAB_DIRECTION_PREF, true]],
});
await waitForTabstripOrientation("vertical");
await SidebarController.show("viewHistorySidebar");
const originalWidth = SidebarController._box.style.width;
SidebarController._box.style.width = "500px";
@@ -489,12 +527,14 @@ add_task(async function test_sidebar_resize() {
SidebarController._box.style.width = originalWidth;
SidebarController.hide();
await SpecialPowers.popPrefEnv();
await waitForTabstripOrientation(initialTabDirection);
});
add_task(async function test_sidebar_display_settings() {
await SpecialPowers.pushPrefEnv({
set: [[TAB_DIRECTION_PREF, true]],
});
await waitForTabstripOrientation("vertical");
await testCustomizeSetting(
"visibilityInput",
Glean.sidebar.displaySettings,
@@ -502,6 +542,7 @@ add_task(async function test_sidebar_display_settings() {
"always"
);
await SpecialPowers.popPrefEnv();
await waitForTabstripOrientation(initialTabDirection);
});
add_task(async function test_sidebar_position_settings() {
@@ -545,6 +586,7 @@ add_task(async function test_sidebar_position_rtl_ui() {
sandbox.restore();
await SpecialPowers.popPrefEnv();
await SidebarController.waitUntilStable();
});
async function testIconClick(expanded) {
@@ -564,7 +606,16 @@ async function testIconClick(expanded) {
Glean.sidebar.bookmarksIconClick,
];
for (const [i, button] of Array.from(sidebarMain.toolButtons).entries()) {
await SidebarController.initializeUIState({ launcherExpanded: expanded });
await SidebarController.initializeUIState({
launcherExpanded: expanded,
panelOpen: false,
});
Assert.equal(
SidebarController.sidebarMain.expanded,
expanded,
`The launcher is ${expanded ? "expanded" : "collapsed"}`
);
Assert.ok(!SidebarController._state.panelOpen, "No panel is open");
info(`Click the icon for: ${button.getAttribute("view")}`);
EventUtils.synthesizeMouseAtCenter(button, {});
@@ -581,11 +632,21 @@ async function testIconClick(expanded) {
}
info("Load an extension.");
// The extensions's sidebar will open when it loads
const extension = ExtensionTestUtils.loadExtension({ ...extData });
await extension.startup();
await extension.awaitMessage("sidebar");
await SidebarController.initializeUIState({ launcherExpanded: expanded });
await SidebarController.initializeUIState({
launcherExpanded: expanded,
panelOpen: false,
});
Assert.equal(
SidebarController.sidebarMain.expanded,
expanded,
`The launcher is ${expanded ? "expanded" : "collapsed"}`
);
Assert.ok(!SidebarController._state.panelOpen, "No panel is open");
info("Click the icon for the extension.");
const extensionButton = await TestUtils.waitForCondition(
@@ -607,6 +668,7 @@ async function testIconClick(expanded) {
await extension.unload();
await SpecialPowers.popPrefEnv();
await waitForTabstripOrientation(initialTabDirection);
Services.fog.testResetFOG();
}

View File

@@ -10,13 +10,17 @@ add_setup(async () => {
});
});
const initialTabDirection = Services.prefs.getBoolPref("sidebar.verticalTabs")
? "vertical"
: "horizontal";
add_task(async function test_extension_context_menu() {
const win = await BrowserTestUtils.openNewBrowserWindow();
await waitForBrowserWindowActive(win);
const { document } = win;
const sidebar = document.querySelector("sidebar-main");
await sidebar.updateComplete;
ok(sidebar, "Sidebar is shown.");
ok(BrowserTestUtils.isVisible(sidebar), "Sidebar is shown.");
const manageStub = sinon.stub(sidebar, "manageExtension");
const reportStub = sinon.stub(sidebar, "reportExtension");
@@ -178,14 +182,19 @@ add_task(async function test_extension_context_menu() {
sinon.restore();
await extension.unload();
ok(
BrowserTestUtils.isHidden(sidebar),
"Unloading the extensions causes the sidebar launcher to hide"
);
await BrowserTestUtils.closeWindow(win);
});
add_task(async function test_sidebar_context_menu() {
const { document, SidebarController } = window;
const { sidebarMain, sidebarContainer } = SidebarController;
await sidebarMain.updateComplete;
ok(sidebarMain, "Sidebar is shown.");
SidebarController._state.updateVisibility(true);
await SidebarController.waitUntilStable();
ok(BrowserTestUtils.isVisible(sidebarMain), "Sidebar is shown.");
const contextMenu = document.getElementById("sidebar-context-menu");
is(contextMenu.state, "closed", "Checking if context menu is closed");
@@ -241,9 +250,10 @@ add_task(async function test_sidebar_context_menu() {
});
ok(
Services.prefs.getBoolPref("sidebar.verticalTabs", false),
"Vertical tabs enabled"
"Vertical tabs disabled"
);
Services.prefs.clearUserPref("sidebar.verticalTabs");
await waitForTabstripOrientation(initialTabDirection);
is(contextMenu.state, "closed", "Context menu closed for vertical tabs");
});
@@ -320,4 +330,5 @@ add_task(async function test_toggle_vertical_tabs_from_sidebar_button() {
window.SidebarController.hide();
await SpecialPowers.popPrefEnv();
await window.SidebarController.waitUntilStable();
});

View File

@@ -55,10 +55,16 @@ add_task(async function test_sidebar_view_commands() {
ok(!sidebar.expanded, "Sidebar is not expanded when the view is closed");
ok(BrowserTestUtils.isHidden(sidebarBox), "Sidebar box is hidden");
// Confirm that toggling the sidebar using the toolbarbutton re-opens it with the previous panel
document.getElementById("sidebar-button").doCommand();
await sidebar.updateComplete;
ok(BrowserTestUtils.isVisible(sidebar), "Sidebar is visible again.");
ok(BrowserTestUtils.isHidden(sidebarBox), "Sidebar box is hidden.");
ok(BrowserTestUtils.isVisible(sidebarBox), "Sidebar panel is visible.");
is(
SidebarController.currentID,
"viewBookmarksSidebar",
"Sidebar controller re-opened the previous panel"
);
// restore the animation pref
SpecialPowers.popPrefEnv();

View File

@@ -85,6 +85,18 @@ const extData = {
},
};
// Ensure each test leaves the sidebar in its initial state when it completes
const initialSidebarState = SidebarController.getUIState();
async function resetSidebarToInitialState() {
info(
`Restoring sidebar state from: ${JSON.stringify(SidebarController.getUIState())}, back to: ${JSON.stringify(initialSidebarState)}`
);
await SidebarController.initializeUIState(initialSidebarState);
}
registerCleanupFunction(async () => {
await resetSidebarToInitialState();
});
function waitForBrowserWindowActive(win) {
// eslint-disable-next-line consistent-return
return new Promise(resolve => {