Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#8228: run show sidebar in top-level frame #8299

Merged
merged 7 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions end-to-end-tests/tests/runtime/sidebarController.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* 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 <http://www.gnu.org/licenses/>.
*/

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";
import { MV } from "../../env";

test.describe("sidebar controller", () => {
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);
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();

// 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 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(8000);

// Focus dialog should be visible in the top-level frame
await expect(page.getByRole("button", { name: "OK" })).toBeVisible();
Comment on lines +65 to +69
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think you should be able to get rid of the waitForTimeout by extending the timeout in the toBeVisible call.

Suggested change
// eslint-disable-next-line playwright/no-wait-for-timeout -- match wait in the mod
await page.waitForTimeout(8000);
// Focus dialog should be visible in the top-level frame
await expect(page.getByRole("button", { name: "OK" })).toBeVisible();
// Focus dialog should be visible in the top-level frame
await expect(page.getByRole("button", { name: "OK" })).toBeVisible({timeout: 8000});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I prefer the waitForTimeout there's an explicit sleep/wait in the mod. So it's matching that wait/sleep brick


// 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);
});
});
6 changes: 3 additions & 3 deletions src/contentScript/messenger/strict/registration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import {
hideMv2SidebarInTopFrame,
showSidebar,
showSidebarInTopFrame,
sidebarWasLoaded,
updateSidebar,
removeExtensions as removeSidebars,
Expand Down Expand Up @@ -54,7 +54,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;
Expand Down Expand Up @@ -84,7 +84,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,
Expand Down
68 changes: 50 additions & 18 deletions src/contentScript/sidebarController.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,21 @@ 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";
import { type RegistryId } from "@/types/registryTypes";
import { type ModComponentRef } from "@/types/modComponentTypes";
import type {
ActivatePanelOptions,
ModActivationPanelEntry,
FormPanelEntry,
ModActivationPanelEntry,
PanelEntry,
PanelPayload,
TemporaryPanelEntry,
Expand All @@ -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";

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved module variables to top of file

* Event listeners triggered when the sidebar shows and is ready to receive messages.
*/
export const sidebarShowEvents = new SimpleEventTarget<RunArgs>();

// 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
Expand Down Expand Up @@ -88,30 +101,37 @@ const pingSidebar = memoizeUntilSettled(
}, 1000) as () => Promise<void>,
);

/**
* Event listeners triggered when the sidebar shows and is ready to receive messages.
*/
export const sidebarShowEvents = new SimpleEventTarget<RunArgs>();

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
* @see pingSidebar
* @throws Error if the sidebar ping fails or does not respond in time
*/
export async function showSidebar(): Promise<void> {
// 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()) {
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;
}

Expand All @@ -137,6 +157,18 @@ export async function showSidebar(): Promise<void> {
}
}

/**
* 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<void> {
// 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);
}

/**
* Force-show the panel for the given extension id
* @param extensionId the extension UUID
Expand Down
12 changes: 8 additions & 4 deletions src/utils/sidePanelUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
if (isBrowserSidebar()) {
console.warn(
Expand Down Expand Up @@ -60,10 +67,7 @@ async function openSidePanelMv3(tabId: number): Promise<void> {
// 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;
Expand Down
Loading