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

#7670: Simplify injection of widgets onto the page (renderWidget) #8150

Merged
merged 15 commits into from
Apr 8, 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
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 47 additions & 0 deletions src/components/AbortSignalGate.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* 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 React from "react";
import { render, screen } from "@testing-library/react";
import AbortSignalGate from "./AbortSignalGate";

it("renders the children when active and hides them when aborted", () => {
const controller = new AbortController();
const { rerender } = render(
<AbortSignalGate signal={controller.signal}>
<div>Content</div>
</AbortSignalGate>,
);
expect(screen.getByText("Content")).toBeInTheDocument();

controller.abort();
rerender(
<AbortSignalGate signal={controller.signal}>
<div>Content</div>
</AbortSignalGate>,
);
expect(screen.queryByText("Content")).not.toBeInTheDocument();
});

it("does not render children when the signal is already aborted", () => {
render(
<AbortSignalGate signal={AbortSignal.abort()}>
<div>Content</div>
</AbortSignalGate>,
);
expect(screen.queryByText("Content")).not.toBeInTheDocument();
});
32 changes: 32 additions & 0 deletions src/components/AbortSignalGate.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* 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 useAbortSignal from "@/hooks/useAbortSignal";
import React from "react";

/**
* Render children until the signal is aborted
*/
const AbortSignalGate: React.FunctionComponent<{ signal: AbortSignal }> = ({
signal,
children,
}) => {
const aborted = useAbortSignal(signal);
return aborted ? null : <>{children}</>;
};

export default AbortSignalGate;
35 changes: 28 additions & 7 deletions src/components/InvalidatedContextGate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,28 @@ import { Button } from "react-bootstrap";
import useContextInvalidated from "@/hooks/useContextInvalidated";
import useDocumentVisibility from "@/hooks/useDocumentVisibility";

const InvalidatedContextGate: React.FunctionComponent<{
contextNameTitleCase: string;
type ContextInvalidatedProps = {
autoReload?: boolean;
}> = ({ children, contextNameTitleCase, autoReload }) => {
const wasContextInvalidated = useContextInvalidated();

/** The name to show on "Reload Context Name" button */
contextNameTitleCase: string;
};

const InformationPanel: React.FunctionComponent<ContextInvalidatedProps> = ({
autoReload,
contextNameTitleCase,
}) => {
// Only auto-reload if the document is in the background
const isDocumentVisible = useDocumentVisibility();
if (wasContextInvalidated && autoReload && !isDocumentVisible) {
if (autoReload && !isDocumentVisible) {
setTimeout(() => {
// If you reload too soon, Chrome might not be ready to serve the page yet
// TODO: Poll the page until it's ready instead of a timeout. Then auto-reload by default
location.reload();
}, 500);
}, 1000);
}

return wasContextInvalidated ? (
return (
<div className="d-flex flex-column align-items-center justify-content-center">
<p>
PixieBrix was updated or restarted. Reload the{" "}
Expand All @@ -49,6 +55,21 @@ const InvalidatedContextGate: React.FunctionComponent<{
Reload {contextNameTitleCase}
</Button>
</div>
);
};

/**
* A gate that shows an information panel with a reload button if the context was invalidated.
*
* Use `<AbortSignalGate signal={onContextInvalidated.signal}>` if you just want to unmount the children instead.
*/
const InvalidatedContextGate: React.FunctionComponent<
ContextInvalidatedProps
> = ({ children, ...props }) => {
const wasContextInvalidated = useContextInvalidated();

return wasContextInvalidated ? (
<InformationPanel {...props} />
) : (
<>{children}</>
);
Expand Down
75 changes: 30 additions & 45 deletions src/contentScript/modalDom.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,10 @@
*/

import React from "react";
import { scrollbarWidth } from "@xobotyi/scrollbar-width";
import { render, unmountComponentAtNode } from "react-dom";
import { mergeSignals } from "abort-utils";
import { onContextInvalidated } from "webext-events";
import { expectContext } from "@/utils/expectContext";
import { renderWidget } from "@/utils/reactUtils";
import useScrollLock from "@/hooks/useScrollLock";
import useAbortSignal from "@/hooks/useAbortSignal";

// This cannot be moved to globals.d.ts because it's a module augmentation
// https://stackoverflow.com/a/42085876/288906
Expand All @@ -29,41 +28,16 @@ declare module "react" {
onClose?: ReactEventHandler<T> | undefined;
}
}
/**
* Show a modal with the given URL in the host page
* @param url the URL to show
* @param controller AbortController to cancel the modal
* @param onOutsideClick callback to call when the user clicks outside the modal
*/
export function showModal({
url,
controller,
onOutsideClick,
}: {

const IframeModal: React.VFC<{
url: URL;
controller: AbortController;
onOutsideClick?: () => void;
}): void {
// In React apps, should use React modal component
expectContext("contentScript");

// Using `<style>` will avoid overriding the site’s inline styles
const style = document.createElement("style");
}> = ({ url, controller, onOutsideClick }) => {
const aborted = useAbortSignal(controller.signal);
useScrollLock(!aborted);
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 file:

  • replace explicit attachShadow() call with renderWidget()'s own
  • replace inline solution with existing useScrollLock()


const scrollableRoot =
window.getComputedStyle(document.body).overflowY === "scroll"
? "body"
: "html";
style.textContent += `${scrollableRoot} {overflow: hidden !important}`; // Disable scrollbar

// Preserve space initially taken by scrollbar
style.textContent += `html {padding-inline-end: ${scrollbarWidth()}px !important}`;

const container = document.createElement("div");
const shadowRoot = container.attachShadow({ mode: "closed" });
document.body.append(container, style);
render(
// TODO: Wrap into separate component and use useScrollLock hook
return aborted ? null : (
<dialog
onClose={() => {
controller.abort();
Expand Down Expand Up @@ -99,16 +73,27 @@ export function showModal({
colorScheme: "normal", // Match parent color scheme #1650
}}
/>
</dialog>,
shadowRoot,
</dialog>
);
};

mergeSignals(controller, onContextInvalidated.signal).addEventListener(
"abort",
() => {
unmountComponentAtNode(container);
style.remove();
container.remove();
},
);
/**
* Show a modal with the given URL in the host page
* @param url the page to show in the modal
* @param controller AbortController to cancel the modal
* @param onOutsideClick callback to call when the user clicks outside the modal
*/
export function showModal(props: {
url: URL;
controller: AbortController;
onOutsideClick?: () => void;
}): void {
// In React apps, you should use the React modal component
expectContext("contentScript");

renderWidget({
name: "iframe-modal",
widget: <IframeModal {...props} />,
signal: props.controller.signal,
});
}
3 changes: 0 additions & 3 deletions src/domConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ export const PIXIEBRIX_QUICK_BAR_CONTAINER_CLASS =
export const PIXIEBRIX_TOOLTIPS_CONTAINER_CLASS =
"pixiebrix-tooltips-container";

export const PIXIEBRIX_NOTIFICATION_CLASS = "pixiebrix-notifier-container";

export const EXTENSION_POINT_DATA_ATTR = "data-pb-extension-point";

/**
Expand All @@ -47,7 +45,6 @@ export const PRIVATE_ATTRIBUTES_SELECTOR = `
#${PANEL_FRAME_ID},
.${PIXIEBRIX_TOOLTIPS_CONTAINER_CLASS},
.${PIXIEBRIX_QUICK_BAR_CONTAINER_CLASS},
.${PIXIEBRIX_NOTIFICATION_CLASS},
[${PIXIEBRIX_DATA_ATTR}],
[${EXTENSION_POINT_DATA_ATTR}]
`;
36 changes: 36 additions & 0 deletions src/hooks/useAbortSignal.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* 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 { renderHook } from "@testing-library/react-hooks";
import useAbortSignal from "./useAbortSignal";

it("returns the initial state of the signal", () => {
const active = renderHook(() => useAbortSignal(new AbortController().signal));
expect(active.result.current).toBe(false);

const aborted = renderHook(() => useAbortSignal(AbortSignal.abort()));
expect(aborted.result.current).toBe(true);
});

it("updates the state when the signal is aborted", () => {
const controller = new AbortController();
const { result } = renderHook(() => useAbortSignal(controller.signal));
expect(result.current).toBe(false);

controller.abort();
expect(result.current).toBe(true);
});
34 changes: 34 additions & 0 deletions src/hooks/useAbortSignal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* 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 { useSyncExternalStore } from "use-sync-external-store/shim";

export default function useAbortSignal(signal: AbortSignal): boolean {
return useSyncExternalStore(
(callback: () => void) => {
const unsubscribe = new AbortController();
signal.addEventListener("abort", callback, {
signal: unsubscribe.signal,
once: true,
});
return () => {
unsubscribe.abort();
};
},
() => signal.aborted,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise, we can use useSyncExternalStore to deal with promises too. I think the whole useAsyncState can be replaced with two useSyncExternalStore calls in 15 lines of code.

}
12 changes: 3 additions & 9 deletions src/hooks/useContextInvalidated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,9 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { useSyncExternalStore } from "use-sync-external-store/shim";
import { onContextInvalidated, wasContextInvalidated } from "webext-events";

function subscribe(callback: () => void) {
const unsubscribe = new AbortController();
onContextInvalidated.addListener(callback, { signal: unsubscribe.signal });
return unsubscribe.abort.bind(unsubscribe);
}
import { onContextInvalidated } from "webext-events";
import useAbortSignal from "./useAbortSignal";

export default function useContextInvalidated(): boolean {
return useSyncExternalStore(subscribe, wasContextInvalidated);
return useAbortSignal(onContextInvalidated.signal);
}
5 changes: 5 additions & 0 deletions src/tsconfig.strictNullChecks.json
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@
"./bricks/transformers/traverseElements.ts",
"./bricks/transformers/url.ts",
"./bricks/types.ts",
"./components/AbortSignalGate.test.tsx",
"./components/AbortSignalGate.tsx",
"./components/AceEditor.tsx",
"./components/AceEditorSync.tsx",
"./components/Alert.tsx",
Expand Down Expand Up @@ -450,6 +452,8 @@
"./hooks/logging.ts",
"./hooks/useAsyncExternalStore.ts",
"./hooks/useAsyncState.ts",
"./hooks/useAbortSignal.test.ts",
"./hooks/useAbortSignal.ts",
"./hooks/useAuthorizationGrantFlow.ts",
"./hooks/useAutoFocusConfiguration.ts",
"./hooks/useBrowserIdentifier.ts",
Expand Down Expand Up @@ -817,6 +821,7 @@
"./utils/postMessage.ts",
"./utils/preventNativeFormSubmission.ts",
"./utils/promiseUtils.ts",
"./utils/reactUtils.tsx",
"./utils/registryUtils.ts",
"./utils/sanitize.ts",
"./utils/schemaUtils.ts",
Expand Down
Loading
Loading