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

#8293 Unavailable overlay for sidebar forms and temp panels on nav #8351

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
8 changes: 8 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,14 @@ module.exports = {
allowConditional: true,
},
],
"playwright/no-wait-for-timeout": "error",
"playwright/no-useless-not": "error",
"playwright/expect-expect": [
"error",
{ assertFunctionNames: ["checkUnavailibilityForNavigationMethod"] },
],
"playwright/no-conditional-in-test": "error",
"playwright/no-conditional-expect": "error",
"playwright/no-commented-out-tests": "error",
"playwright/no-hooks": "error", // Use fixtures instead to share common setup / teardown code
"playwright/no-get-by-title": "error",
Expand Down
1 change: 1 addition & 0 deletions end-to-end-tests/fixtures/envSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const test = base.extend<{
assertRequiredEnvVariables();

for (const key of additionalRequiredEnvVariables) {
// eslint-disable-next-line security/detect-object-injection -- internally controlled
if (process.env[key] === undefined) {
throw new Error(
`This test requires additional environment variable ${key} to be configured. Configure it in your .env.development file and re-build the extension.`,
Expand Down
1 change: 0 additions & 1 deletion end-to-end-tests/tests/bricks/sidebarEffects.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ test.describe("sidebar effect bricks", () => {
await page.goto("/");

// Ensure the page is focused by clicking on an element before running the keyboard shortcut, see runModViaQuickbar
await page.getByText("Index of /").click();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored the runModViaQuickBar method so we don't have to have these click calls before it.

await runModViaQuickBar(page, "Toggle Sidebar");

// Will error if page/frame not available
Expand Down
7 changes: 5 additions & 2 deletions end-to-end-tests/tests/extensionConsoleActivation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,11 @@ test("can activate a mod with built-in integration", async ({
await modActivationPage.clickActivateAndWaitForModsPageRedirect();
await page.goto("/");

// Ensure the page is focused by clicking on an element before running the keyboard shortcut, see runModViaQuickbar
await page.getByText("Index of /").click();
// Ensure the QuickBar is ready
await expect(
page.getByRole("button", { name: "open the PixieBrix quick bar" }),
).toBeVisible();

await runModViaQuickBar(page, "GIPHY Search");

// Search for "kitten" keyword
Expand Down
1 change: 0 additions & 1 deletion end-to-end-tests/tests/regressions/formFlicker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ test.describe("forms flickering due to components unexpectedly unmounting/remoun

await page.goto("/bootstrap-5");

await page.getByRole("heading", { name: "PixieBrix" }).click();
await runModViaQuickBar(page, "Open Sidebar");

const sideBarPage = await getSidebarPage(page, extensionId);
Expand Down
2 changes: 1 addition & 1 deletion end-to-end-tests/tests/runtime/sidebarController.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ test.describe("sidebar controller", () => {

// 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();
await expect(frame.getByRole("button", { name: "OK" })).toBeHidden();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated lint error fixed


// Will error if page/frame not available
await getSidebarPage(page, extensionId);
Expand Down
134 changes: 131 additions & 3 deletions end-to-end-tests/tests/runtime/sidebarNavigation.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { type Page, test as base } from "@playwright/test";
import { getSidebarPage, runModViaQuickBar } from "../../utils";
import { MV, SERVICE_URL } from "../../env";

test("sidebar is persistent during navigation", async ({
test("sidebar mod panels are persistent during navigation", async ({
page,
extensionId,
}) => {
Expand All @@ -38,8 +38,6 @@ test("sidebar is persistent during navigation", async ({

await page.goto("/");

// Ensure the page is focused by clicking on an element before running the keyboard shortcut, see runModViaQuickbar
await page.getByText("Index of /").click();
await runModViaQuickBar(page, "Open Sidebar");

const sideBarPage = (await getSidebarPage(page, extensionId)) as Page; // MV3 sidebar is a separate page
Expand Down Expand Up @@ -108,3 +106,133 @@ test("sidebar is persistent during navigation", async ({
expect(sideBarPageClosed).toBe(true);
}).toPass({ timeout: 5000 });
});

const navigationMethods: Array<{
name: string;
navigationMethod: (page: Page) => Promise<void>;
}> = [
{
name: "refresh",
async navigationMethod(page: Page) {
await page.reload();
},
},
{
name: "back button",
async navigationMethod(page: Page) {
await page.goBack();
await page.goBack();
},
},
{
name: "goto new page",
async navigationMethod(page: Page) {
await page.goto(SERVICE_URL);
},
},
];

// Helper method for checking that the sidebar panels are unavailable after a navigation method
async function checkUnavailibilityForNavigationMethod(
page: Page,
extensionId: string,
navigationMethod: (page: Page) => Promise<void>,
) {
await page.goto("/advanced-fields");
await runModViaQuickBar(page, "Open form");

const sideBarPage = (await getSidebarPage(page, extensionId)) as Page; // MV3 sidebar is a separate page
// Set up close listener for sidebar page
let sideBarPageClosed = false;
sideBarPage.on("close", () => {
sideBarPageClosed = true;
});

await expect(
sideBarPage
.frameLocator("iframe")
.getByRole("heading", { name: "Example Form" }),
).toBeVisible();
await expect(
sideBarPage.getByRole("tab", { name: "Example form" }),
).toBeVisible();

await runModViaQuickBar(page, "Open temp panel");
await expect(
sideBarPage.getByRole("heading", { name: "Example document" }),
).toBeVisible();
await expect(
sideBarPage.getByRole("tab", { name: "Example info" }),
).toBeVisible();

// Click on "contentEditable" header, which updates the url to .../#contenteditable
await page.getByRole("link", { name: "contentEditable" }).click();
expect(page.url()).toBe(
"https://pbx.vercel.app/advanced-fields/#contenteditable",
);
// Should not cause the temporary panel to become unavailable
await expect(
sideBarPage
.getByLabel("Example Info")
.getByText("Panel no longer available"),
).toBeHidden();

await navigationMethod(page);

await expect(
sideBarPage
.getByLabel("Example Info")
.getByText("Panel no longer available"),
).toBeVisible();
await sideBarPage
.getByLabel("Example Info")
.getByLabel("Close the unavailable panel")
.click();
await expect(
sideBarPage.getByRole("tab", { name: "Example info" }),
).toBeHidden();

// The unavailable overlay is still displayed for the form panel
await expect(
sideBarPage
.getByLabel("Example form")
.getByText("Panel no longer available"),
).toBeVisible();
await sideBarPage
.getByLabel("Example form")
.getByLabel("Close the unavailable panel")
.click();

// Closing the last panel should close the sidebar
await expect(() => {
expect(sideBarPageClosed).toBe(true);
}).toPass({ timeout: 5000 });
}

test("sidebar form and temporary panels are unavailable after navigation", async ({
page,
extensionId,
}) => {
test.skip(MV === "2", "Navigation is not supported for MV2 sidebar");
// This mod has two quickbar actions for opening a temporary panel and a form panel in the sidebar.
const modId = "@e2e-testing/temp-panel-unavailable-on-navigation";

const modActivationPage = new ActivateModPage(page, extensionId, modId);
await modActivationPage.goto();

await modActivationPage.clickActivateAndWaitForModsPageRedirect();

// Prime the browser history with an initial navigation
await page.goto(SERVICE_URL);

for (const { navigationMethod, name } of navigationMethods) {
// eslint-disable-next-line no-await-in-loop -- check each navigation method sequentially
await test.step(`Checking navigation method: ${name}`, async () => {
await checkUnavailibilityForNavigationMethod(
page,
extensionId,
navigationMethod,
);
});
}
});
61 changes: 35 additions & 26 deletions end-to-end-tests/tests/telemetry/errors.spec.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,37 @@
import { test, expect } from "../../fixtures/extensionBase";
import { type Request } from "playwright-core";
// @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 { type BrowserContext, type Page, test as base } from "@playwright/test";
import { getBaseExtensionConsoleUrl } from "../../pageObjects/constants";
import { MV } from "../../env";

async function waitForBackgroundPageRequest(
context: BrowserContext,
extensionId: string,
errorServiceEndpoint: string,
) {
Comment on lines +7 to +11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactored this method in the process of fixing an eslint error for no conditionals

if (MV === "3") {
// Due to service worker limitations with the Datadog SDK, we need to report errors via an offscreen document
// (see https://github.com/pixiebrix/pixiebrix-extension/issues/8268). The offscreen document is created when
// the first error is reported, so we need to wait for it to be created before we can interact with it.
let offscreenPage: Page | undefined;
await expect(async () => {
offscreenPage = context
.pages()
.find((value) =>
value
.url()
.startsWith(`chrome-extension://${extensionId}/offscreen.html`),
);

expect(offscreenPage?.url()).toBeDefined();
}).toPass({ timeout: 5000 });
return offscreenPage?.waitForRequest(errorServiceEndpoint);
}

const backgroundPage = context.backgroundPages()[0];
return backgroundPage?.waitForRequest(errorServiceEndpoint);
}

test.use({
additionalRequiredEnvVariables: [
"DATADOG_CLIENT_TOKEN",
Expand Down Expand Up @@ -39,32 +66,14 @@ test("can report application error to telemetry service", async ({
await page.goto(getBaseExtensionConsoleUrl(extensionId));
await expect(page.getByText("Something went wrong.")).toBeVisible();

let waitForRequest: Promise<Request> | undefined;
if (MV === "3") {
// Due to service worker limitations with the Datadog SDK, we need to report errors via an offscreen document
// (see https://github.com/pixiebrix/pixiebrix-extension/issues/8268). The offscreen document is created when
// the first error is reported, so we need to wait for it to be created before we can interact with it.
let offscreenPage: Page | undefined;
await expect(async () => {
offscreenPage = context
.pages()
.find((value) =>
value
.url()
.startsWith(`chrome-extension://${extensionId}/offscreen.html`),
);

expect(offscreenPage?.url()).toBeDefined();
}).toPass({ timeout: 5000 });
waitForRequest = offscreenPage?.waitForRequest(errorServiceEndpoint);
} else {
const backgroundPage = context.backgroundPages()[0];
waitForRequest = backgroundPage?.waitForRequest(errorServiceEndpoint);
}

// TODO: due to Datadog SDK implementation, it will take ~30 seconds for the
// request to be sent. We should figure out a way to induce the request to be sent sooner.
const request = await waitForRequest;
const request = await waitForBackgroundPageRequest(
context,
extensionId,
errorServiceEndpoint,
);

const errorLogsJson = request
?.postData()
?.split("\n")
Expand Down
16 changes: 14 additions & 2 deletions end-to-end-tests/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,14 @@ export async function ensureVisibility(
}

// Run a mod via the Quickbar.
// NOTE: Page needs to be focused before running this function, e.g. by clicking on the page.
// TODO: Fix the page-focus precondition by generalizing the page-focusing logic to be page-agnostic
export async function runModViaQuickBar(page: Page, modName: string) {
await waitForQuickBarReadiness(page);
await page.locator("html").focus(); // Ensure the page is focused before running the keyboard shortcut
await page.keyboard.press("Meta+M"); // MacOS
await page.keyboard.press("Control+M"); // Windows and Linux
// Short delay to allow the quickbar to finish opening
// eslint-disable-next-line playwright/no-wait-for-timeout -- TODO: Find a better way to detect when the quickbar is done loading opening
await page.waitForTimeout(500);
await page.getByRole("option", { name: modName }).click();
}

Expand Down Expand Up @@ -161,6 +164,15 @@ export async function waitForSelectionMenuReadiness(page: Page) {
}).toPass({ timeout: 5000 });
}

// Waits for the quick bar to be ready to use
async function waitForQuickBarReadiness(page: Page) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this check, sometimes the quickbar would not open when immediately attempting to open it after loading a new page.

await expect(async () => {
await expect(page.locator("html")).toHaveAttribute(
"data-pb-quick-bar-ready",
);
}).toPass({ timeout: 5000 });
}

// Simulates mouse entering the sidebar to track focus on MV2
// https://github.com/pixiebrix/pixiebrix-extension/blob/1794863937f343fbc8e3a4434eace74191f8dfbd/src/contentScript/sidebarController.tsx#L563-L563
export async function conditionallyHoverOverMV2Sidebar(page: Page) {
Expand Down
8 changes: 8 additions & 0 deletions src/components/quickBar/QuickBarApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { once } from "lodash";
import {
MAX_Z_INDEX,
PIXIEBRIX_QUICK_BAR_CONTAINER_CLASS,
QUICK_BAR_READY_ATTRIBUTE,
} from "@/domConstants";
import useEventListener from "@/hooks/useEventListener";
import { Stylesheets } from "@/components/Stylesheets";
Expand Down Expand Up @@ -203,6 +204,11 @@ export const QuickBarApp: React.FC = () => (
</KBarProvider>
);

function markQuickBarReady() {
const html = globalThis.document?.documentElement;
html.setAttribute(QUICK_BAR_READY_ATTRIBUTE, "true");
}

export const initQuickBarApp = once(async () => {
expectContext("contentScript");

Expand All @@ -226,6 +232,8 @@ export const initQuickBarApp = once(async () => {
ReactDOM.render(<QuickBarApp />, container);
console.debug("Initialized quick bar");

markQuickBarReady();

onContextInvalidated.addListener(() => {
console.debug("Removed quick bar due to context invalidation");
ReactDOM.unmountComponentAtNode(container);
Expand Down
2 changes: 2 additions & 0 deletions src/domConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ export const MAX_Z_INDEX = NOTIFICATIONS_Z_INDEX - 1; // Let notifications alway

export const SELECTION_MENU_READY_ATTRIBUTE = "data-pb-selection-menu-ready";

export const QUICK_BAR_READY_ATTRIBUTE = "data-pb-quick-bar-ready";

export const PANEL_FRAME_ID = "pixiebrix-extension";

export const PIXIEBRIX_DATA_ATTR = "data-pb-uuid";
Expand Down
Loading
Loading