From 1965263344a3b9fe0d588ebf550719c037c4439b Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Fri, 19 Apr 2024 18:06:50 -0500 Subject: [PATCH 1/6] #8228: show sidebar in top-level frame --- .../messenger/strict/registration.ts | 6 +- src/contentScript/sidebarController.tsx | 66 ++++++++++++++----- src/utils/sidePanelUtils.ts | 12 ++-- 3 files changed, 59 insertions(+), 25 deletions(-) diff --git a/src/contentScript/messenger/strict/registration.ts b/src/contentScript/messenger/strict/registration.ts index fba970c353..cb9c1abfd6 100644 --- a/src/contentScript/messenger/strict/registration.ts +++ b/src/contentScript/messenger/strict/registration.ts @@ -19,7 +19,7 @@ import { hideMv2SidebarInTopFrame, - showSidebar, + showSidebarInTopFrame, sidebarWasLoaded, updateSidebar, removeExtensions as removeSidebars, @@ -53,7 +53,7 @@ declare global { FORM_CANCEL: typeof cancelForm; UPDATE_SIDEBAR: typeof updateSidebar; SIDEBAR_WAS_LOADED: typeof sidebarWasLoaded; - SHOW_SIDEBAR: typeof showSidebar; + SHOW_SIDEBAR: typeof showSidebarInTopFrame; HIDE_SIDEBAR: typeof hideMv2SidebarInTopFrame; REMOVE_SIDEBARS: typeof removeSidebars; HANDLE_MENU_ACTION: typeof handleMenuAction; @@ -82,7 +82,7 @@ export default function registerMessenger(): void { FORM_CANCEL: cancelForm, UPDATE_SIDEBAR: updateSidebar, SIDEBAR_WAS_LOADED: sidebarWasLoaded, - SHOW_SIDEBAR: showSidebar, + SHOW_SIDEBAR: showSidebarInTopFrame, HIDE_SIDEBAR: hideMv2SidebarInTopFrame, REMOVE_SIDEBARS: removeSidebars, HANDLE_MENU_ACTION: handleMenuAction, diff --git a/src/contentScript/sidebarController.tsx b/src/contentScript/sidebarController.tsx index 76fd19aa87..7cb12ba73b 100644 --- a/src/contentScript/sidebarController.tsx +++ b/src/contentScript/sidebarController.tsx @@ -19,10 +19,12 @@ import reportEvent from "@/telemetry/reportEvent"; import { Events } from "@/telemetry/events"; import { expectContext } from "@/utils/expectContext"; import sidebarInThisTab from "@/sidebar/messenger/api"; +import * as contentScriptApi from "@/contentScript/messenger/strict/api"; import { isEmpty, throttle } from "lodash"; import { signalFromEvent } from "abort-utils"; import { SimpleEventTarget } from "@/utils/SimpleEventTarget"; import * as sidebarMv2 from "@/contentScript/sidebarDomControllerLite"; +import { getSidebarElement } from "@/contentScript/sidebarDomControllerLite"; import { type Except } from "type-fest"; import { type RunArgs, RunReason } from "@/types/runtimeTypes"; import { type UUID } from "@/types/stringTypes"; @@ -30,8 +32,8 @@ import { type RegistryId } from "@/types/registryTypes"; import { type ModComponentRef } from "@/types/modComponentTypes"; import type { ActivatePanelOptions, - ModActivationPanelEntry, FormPanelEntry, + ModActivationPanelEntry, PanelEntry, PanelPayload, TemporaryPanelEntry, @@ -41,18 +43,29 @@ import { getFormPanelSidebarEntries } from "@/platform/forms/formController"; import { memoizeUntilSettled } from "@/utils/promiseUtils"; import { getTimedSequence } from "@/types/helpers"; import { isMV3 } from "@/mv3/api"; -import { getErrorMessage } from "@/errors/errorHelpers"; import { focusCaptureDialog } from "@/contentScript/focusCaptureDialog"; import { isLoadedInIframe } from "@/utils/iframeUtils"; import { showMySidePanel } from "@/background/messenger/strict/api"; -import { getSidebarElement } from "@/contentScript/sidebarDomControllerLite"; import focusController from "@/utils/focusController"; import selectionController from "@/utils/selectionController"; -import { messenger } from "webext-messenger"; -import { getSidebarTargetForCurrentTab } from "@/utils/sidePanelUtils"; +import { getTopLevelFrame, messenger } from "webext-messenger"; +import { + getSidebarTargetForCurrentTab, + isUserGestureRequiredError, +} from "@/utils/sidePanelUtils"; const HIDE_SIDEBAR_EVENT_NAME = "pixiebrix:hideSidebar"; +/** + * Event listeners triggered when the sidebar shows and is ready to receive messages. + */ +export const sidebarShowEvents = new SimpleEventTarget(); + +// eslint-disable-next-line local-rules/persistBackgroundData -- Unused there +const panels: PanelEntry[] = []; + +let modActivationPanelEntry: ModActivationPanelEntry | null = null; + /* * Only one check at a time * Cannot throttle because subsequent checks need to be able to be made immediately @@ -88,30 +101,33 @@ const pingSidebar = memoizeUntilSettled( }, 1000) as () => Promise, ); -/** - * Event listeners triggered when the sidebar shows and is ready to receive messages. - */ -export const sidebarShowEvents = new SimpleEventTarget(); - export function sidebarWasLoaded(): void { sidebarShowEvents.emit({ reason: RunReason.MANUAL }); } -// eslint-disable-next-line local-rules/persistBackgroundData -- Unused there -const panels: PanelEntry[] = []; - -let modActivationPanelEntry: ModActivationPanelEntry | null = null; - /** - * Attach the sidebar to the page if it's not already attached. Then re-renders all panels. + * Content script handler for showing the sidebar in the top-level frame. Regular callers should call + * showSidebar instead, which handles calls from iframes. + * + * - Resolves when the sidebar is initialized (responds to a ping) + * - Shows focusCaptureDialog if a user gesture is required + * + * @see showSidebar + * @throws Error if the sidebar ping fails, e.g. it does not respond in time */ -export async function showSidebar(): Promise { +export const showSidebarInTopFrame = memoizeUntilSettled(async () => { reportEvent(Events.SIDEBAR_SHOW); + + if (isLoadedInIframe()) { + console.warn("showSidebarInTopFrame should not be called in an iframe"); + } + + // Defensively handle accidental calls from iframes if (isMV3() || isLoadedInIframe()) { try { await showMySidePanel(); } catch (error) { - if (!getErrorMessage(error).includes("user gesture")) { + if (!isUserGestureRequiredError(error)) { throw error; } @@ -135,6 +151,20 @@ export async function showSidebar(): Promise { } catch (error) { throw new Error("The sidebar did not respond in time", { cause: error }); } +}); + +/** + * Attach the sidebar to the page if it's not already attached. Safe to call from any frame. Resolves when the + * sidebar is initialized. + */ +export async function showSidebar(): Promise { + if (isLoadedInIframe()) { + const topLevelFrame = await getTopLevelFrame(); + await contentScriptApi.showSidebar(topLevelFrame); + } else { + // Call directly for performance + await showSidebarInTopFrame(); + } } /** diff --git a/src/utils/sidePanelUtils.ts b/src/utils/sidePanelUtils.ts index 63b35dfb63..d16d7024f6 100644 --- a/src/utils/sidePanelUtils.ts +++ b/src/utils/sidePanelUtils.ts @@ -24,6 +24,13 @@ import { type PageTarget, messenger, getThisFrame } from "webext-messenger"; import { isContentScript } from "webext-detect-page"; import { showSidebar } from "@/contentScript/messenger/strict/api"; +/** + * Returns true if an error showing sidebar is due to a missing user gesture. + */ +export function isUserGestureRequiredError(error: unknown): boolean { + return getErrorMessage(error).includes("user gesture"); +} + export async function openSidePanel(tabId: number): Promise { if (isBrowserSidebar()) { console.warn( @@ -60,10 +67,7 @@ async function openSidePanelMv3(tabId: number): Promise { // it's still part of a user gesture. // If it's not, it will throw an error *even if the side panel is already open*. // The following code silences that error iff the side panel is already open. - if ( - getErrorMessage(error).includes("user gesture") && - (await isSidePanelOpen(tabId)) - ) { + if (isUserGestureRequiredError(error) && (await isSidePanelOpen(tabId))) { // The `openSidePanel` call was not required in the first place, the error can be silenced // TODO: After switching to MV3, verify whether we drop that `openSidePanel` call return; From 10185956855a9ca89dd3f7a9d23c52edbc822eae Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Fri, 19 Apr 2024 18:31:07 -0500 Subject: [PATCH 2/6] #8228: add test for opening sidebar from frame --- .../tests/runtime/sidebarController.spec.ts | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 end-to-end-tests/tests/runtime/sidebarController.spec.ts diff --git a/end-to-end-tests/tests/runtime/sidebarController.spec.ts b/end-to-end-tests/tests/runtime/sidebarController.spec.ts new file mode 100644 index 0000000000..839444b81d --- /dev/null +++ b/end-to-end-tests/tests/runtime/sidebarController.spec.ts @@ -0,0 +1,50 @@ +/* + * Copyright (C) 2024 PixieBrix, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +import { test, expect } from "../../fixtures/extensionBase"; +import { ActivateModPage } from "../../pageObjects/extensionConsole/modsPage"; +// @ts-expect-error -- https://youtrack.jetbrains.com/issue/AQUA-711/Provide-a-run-configuration-for-Playwright-tests-in-specs-with-fixture-imports-only +import { type Page, test as base } from "@playwright/test"; +import { getSidebarPage, isSidebarOpen } from "../../utils"; + +test.describe("sidebar controller", () => { + test("toggle sidebar brick", async ({ page, extensionId }) => { + const modId = "@pixies/test/frame-sidebar-actions"; + + const modActivationPage = new ActivateModPage(page, extensionId, modId); + await modActivationPage.goto(); + await modActivationPage.clickActivateAndWaitForModsPageRedirect(); + + await page.goto("/frames-builder.html"); + + const frame = page.frameLocator("iframe"); + await frame.getByRole("link", { name: "Show Sidebar Immediately" }).click(); + + // Will error if page/frame not available + const sidebarPage = await getSidebarPage(page, extensionId); + + await expect( + sidebarPage.getByRole("tab", { name: "Mods Close" }), + ).toBeVisible(); + + await frame.getByRole("link", { name: "Hide Sidebar" }).click(); + + await expect(() => { + expect(isSidebarOpen(page, extensionId)).toBe(false); + }).toPass({ timeout: 5000 }); + }); +}); From fdb5fbe644ac07dec362f51de8431ee22a7e02a9 Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Fri, 19 Apr 2024 19:59:15 -0500 Subject: [PATCH 3/6] #8228: add test for sidebar with wait --- .../tests/runtime/sidebarController.spec.ts | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/end-to-end-tests/tests/runtime/sidebarController.spec.ts b/end-to-end-tests/tests/runtime/sidebarController.spec.ts index 839444b81d..f987b2592f 100644 --- a/end-to-end-tests/tests/runtime/sidebarController.spec.ts +++ b/end-to-end-tests/tests/runtime/sidebarController.spec.ts @@ -20,9 +20,10 @@ import { ActivateModPage } from "../../pageObjects/extensionConsole/modsPage"; // @ts-expect-error -- https://youtrack.jetbrains.com/issue/AQUA-711/Provide-a-run-configuration-for-Playwright-tests-in-specs-with-fixture-imports-only import { type Page, test as base } from "@playwright/test"; import { getSidebarPage, isSidebarOpen } from "../../utils"; +import { MV } from "../../env"; test.describe("sidebar controller", () => { - test("toggle sidebar brick", async ({ page, extensionId }) => { + test("show sidebar uses top-level frame", async ({ page, extensionId }) => { const modId = "@pixies/test/frame-sidebar-actions"; const modActivationPage = new ActivateModPage(page, extensionId, modId); @@ -46,5 +47,23 @@ test.describe("sidebar controller", () => { await expect(() => { expect(isSidebarOpen(page, extensionId)).toBe(false); }).toPass({ timeout: 5000 }); + + // Mod waits 5 seconds before running Show Sidebar brick to for user gesture dialog to show + await frame.getByRole("link", { name: "Show Sidebar after Wait" }).click(); + await page.waitForTimeout(5000); + + // Expect the focus dialog to be visible on the top-level frame + if (MV === "3") { + // FIXME: why aren't we getting a dialog here when running locally? + // Should be on the top-level frame + await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); + + // Should not be on the frame. Check after checking the top-level frame because it's a positive check for + // the dialog being shown. + await expect(frame.getByRole("button", { name: "OK" })).not.toBeVisible(); + } + + // Will error if page/frame not available + await getSidebarPage(page, extensionId); }); }); From f0320de644d8f6aad9e7b16fde1d894f139ab55e Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Sat, 20 Apr 2024 10:23:54 -0500 Subject: [PATCH 4/6] Clarify method --- src/contentScript/sidebarController.tsx | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/contentScript/sidebarController.tsx b/src/contentScript/sidebarController.tsx index 7cb12ba73b..eae83f83ef 100644 --- a/src/contentScript/sidebarController.tsx +++ b/src/contentScript/sidebarController.tsx @@ -113,9 +113,13 @@ export function sidebarWasLoaded(): void { * - Shows focusCaptureDialog if a user gesture is required * * @see showSidebar - * @throws Error if the sidebar ping fails, e.g. it does not respond in time + * @see pingSidebar + * @throws Error if the sidebar ping fails or does not respond in time */ -export const showSidebarInTopFrame = memoizeUntilSettled(async () => { +// Don't memoizeUntilSettled this method. focusCaptureDialog is memoized which prevents this method from showing +// the focus dialog from multiple times. By allowing multiple concurrent calls to showSidebarInTopFrame, +// a subsequent call might succeed, which will then automatically close the focusCaptureDialog (via it's abort signal) +export async function showSidebarInTopFrame() { reportEvent(Events.SIDEBAR_SHOW); if (isLoadedInIframe()) { @@ -151,20 +155,18 @@ export const showSidebarInTopFrame = memoizeUntilSettled(async () => { } catch (error) { throw new Error("The sidebar did not respond in time", { cause: error }); } -}); +} /** * Attach the sidebar to the page if it's not already attached. Safe to call from any frame. Resolves when the * sidebar is initialized. + * @see showSidebarInTopFrame */ export async function showSidebar(): Promise { - if (isLoadedInIframe()) { - const topLevelFrame = await getTopLevelFrame(); - await contentScriptApi.showSidebar(topLevelFrame); - } else { - // Call directly for performance - await showSidebarInTopFrame(); - } + // Could consider explicitly calling showSidebarInTopFrame directly if we're already in the top frame. + // But the messenger will already handle that case automatically. + const topLevelFrame = await getTopLevelFrame(); + await contentScriptApi.showSidebar(topLevelFrame); } /** From ad8b11cc9dd0a1866031b02fff60b9b6f14a41bf Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 25 Apr 2024 08:15:06 -0400 Subject: [PATCH 5/6] Split tests --- .../tests/runtime/sidebarController.spec.ts | 51 +++++++++++-------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/end-to-end-tests/tests/runtime/sidebarController.spec.ts b/end-to-end-tests/tests/runtime/sidebarController.spec.ts index f987b2592f..540e8ae1d4 100644 --- a/end-to-end-tests/tests/runtime/sidebarController.spec.ts +++ b/end-to-end-tests/tests/runtime/sidebarController.spec.ts @@ -23,7 +23,10 @@ import { getSidebarPage, isSidebarOpen } from "../../utils"; import { MV } from "../../env"; test.describe("sidebar controller", () => { - test("show sidebar uses top-level frame", async ({ page, extensionId }) => { + test("can open sidebar immediately from iframe without focus dialog", async ({ + page, + extensionId, + }) => { const modId = "@pixies/test/frame-sidebar-actions"; const modActivationPage = new ActivateModPage(page, extensionId, modId); @@ -35,33 +38,41 @@ test.describe("sidebar controller", () => { const frame = page.frameLocator("iframe"); await frame.getByRole("link", { name: "Show Sidebar Immediately" }).click(); - // Will error if page/frame not available - const sidebarPage = await getSidebarPage(page, extensionId); - - await expect( - sidebarPage.getByRole("tab", { name: "Mods Close" }), - ).toBeVisible(); - - await frame.getByRole("link", { name: "Hide Sidebar" }).click(); - + // Don't use getSidebarPage because it automatically clicks the MV3 focus dialog. await expect(() => { expect(isSidebarOpen(page, extensionId)).toBe(false); }).toPass({ timeout: 5000 }); + }); + + test("shows focus dialog in top-level frame", async ({ + page, + extensionId, + }) => { + test.skip(MV === "2", "This test is only relevant for MV3"); + + const modId = "@pixies/test/frame-sidebar-actions"; + + const modActivationPage = new ActivateModPage(page, extensionId, modId); + await modActivationPage.goto(); + await modActivationPage.clickActivateAndWaitForModsPageRedirect(); + + await page.goto("/frames-builder.html"); + + const frame = page.frameLocator("iframe"); - // Mod waits 5 seconds before running Show Sidebar brick to for user gesture dialog to show + // Mod waits 5 seconds before running Show Sidebar brick to ensure the user gesture dialog is shown await frame.getByRole("link", { name: "Show Sidebar after Wait" }).click(); + // eslint-disable-next-line playwright/no-wait-for-timeout -- match wait in the mod await page.waitForTimeout(5000); - // Expect the focus dialog to be visible on the top-level frame - if (MV === "3") { - // FIXME: why aren't we getting a dialog here when running locally? - // Should be on the top-level frame - await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); + // FIXME: https://github.com/pixiebrix/pixiebrix-extension/pull/8299/files#r1574832956. In Playwright, the + // the focus dialog is not shown at all, despite the timeout in the mod. + // Focus dialog should be visible in the top-level frame + // await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); - // Should not be on the frame. Check after checking the top-level frame because it's a positive check for - // the dialog being shown. - await expect(frame.getByRole("button", { name: "OK" })).not.toBeVisible(); - } + // The focus dialog should not be shown in the iframe. Check after checking the top-level frame + // because it's a positive check for the dialog being shown. + await expect(frame.getByRole("button", { name: "OK" })).not.toBeVisible(); // Will error if page/frame not available await getSidebarPage(page, extensionId); From 51b1816fdcbc4c242efc8ee48e80938092e7209f Mon Sep 17 00:00:00 2001 From: Todd Schiller Date: Thu, 25 Apr 2024 08:20:11 -0400 Subject: [PATCH 6/6] Fix focus dialog test --- end-to-end-tests/tests/runtime/sidebarController.spec.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/end-to-end-tests/tests/runtime/sidebarController.spec.ts b/end-to-end-tests/tests/runtime/sidebarController.spec.ts index 540e8ae1d4..445e140627 100644 --- a/end-to-end-tests/tests/runtime/sidebarController.spec.ts +++ b/end-to-end-tests/tests/runtime/sidebarController.spec.ts @@ -60,15 +60,13 @@ test.describe("sidebar controller", () => { const frame = page.frameLocator("iframe"); - // Mod waits 5 seconds before running Show Sidebar brick to ensure the user gesture dialog is shown + // Mod waits 7.5 seconds before running Show Sidebar brick to ensure the user gesture dialog is shown await frame.getByRole("link", { name: "Show Sidebar after Wait" }).click(); // eslint-disable-next-line playwright/no-wait-for-timeout -- match wait in the mod - await page.waitForTimeout(5000); + await page.waitForTimeout(8000); - // FIXME: https://github.com/pixiebrix/pixiebrix-extension/pull/8299/files#r1574832956. In Playwright, the - // the focus dialog is not shown at all, despite the timeout in the mod. // Focus dialog should be visible in the top-level frame - // await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); + await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); // The focus dialog should not be shown in the iframe. Check after checking the top-level frame // because it's a positive check for the dialog being shown.