From 2603003745ad6a93acadcf05deee12b16526f6e7 Mon Sep 17 00:00:00 2001 From: Will Hunt <will@half-shot.uk> Date: Thu, 28 Mar 2024 12:04:14 +0000 Subject: [PATCH] =?UTF-8?q?Revert=20"Make=20EC=20widget=20theme=20reactive?= =?UTF-8?q?=20-=20Update=20widget=20url=20when=20the=20theme=20chan?= =?UTF-8?q?=E2=80=A6"=20(#12382)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit c42562ef39672c7b665b91956df7a97fc7553f8d. (cherry picked from commit 75a989e409f70e292c88a48a471768782a272337) --- src/components/views/elements/AppTile.tsx | 38 ++------- src/settings/watchers/ThemeWatcher.ts | 13 +-- src/stores/widgets/StopGapWidget.ts | 10 +-- src/theme.ts | 2 +- .../views/elements/AppTile-test.tsx | 79 +------------------ .../__snapshots__/AppTile-test.tsx.snap | 4 - test/stores/widgets/StopGapWidget-test.ts | 49 +++--------- 7 files changed, 23 insertions(+), 172 deletions(-) diff --git a/src/components/views/elements/AppTile.tsx b/src/components/views/elements/AppTile.tsx index f55751420e..34c773d3c9 100644 --- a/src/components/views/elements/AppTile.tsx +++ b/src/components/views/elements/AppTile.tsx @@ -57,7 +57,6 @@ import { WidgetMessagingStore } from "../../../stores/widgets/WidgetMessagingSto import { SdkContextClass } from "../../../contexts/SDKContext"; import { ModuleRunner } from "../../../modules/ModuleRunner"; import { parseUrl } from "../../../utils/UrlUtils"; -import ThemeWatcher, { ThemeWatcherEvents } from "../../../settings/watchers/ThemeWatcher"; interface IProps { app: IWidget | IApp; @@ -116,7 +115,6 @@ interface IState { menuDisplayed: boolean; requiresClient: boolean; hasContextMenuOptions: boolean; - widgetUrl?: string; } export default class AppTile extends React.Component<IProps, IState> { @@ -142,7 +140,7 @@ export default class AppTile extends React.Component<IProps, IState> { private sgWidget: StopGapWidget | null; private dispatcherRef?: string; private unmounted = false; - private themeWatcher = new ThemeWatcher(); + public constructor(props: IProps, context: ContextType<typeof MatrixClientContext>) { super(props); this.context = context; // XXX: workaround for lack of `declare` support on `public context!:` definition @@ -269,7 +267,6 @@ export default class AppTile extends React.Component<IProps, IState> { !newProps.userWidget, newProps.onDeleteClick, ), - widgetUrl: this.sgWidget?.embedUrl, }; } @@ -355,8 +352,6 @@ export default class AppTile extends React.Component<IProps, IState> { } private setupSgListeners(): void { - this.themeWatcher.on(ThemeWatcherEvents.ThemeChange, this.onThemeChanged); - this.themeWatcher.start(); this.sgWidget?.on("ready", this.onWidgetReady); this.sgWidget?.on("error:preparing", this.updateRequiresClient); // emits when the capabilities have been set up or changed @@ -364,9 +359,7 @@ export default class AppTile extends React.Component<IProps, IState> { } private stopSgListeners(): void { - this.themeWatcher.stop(); if (!this.sgWidget) return; - this.themeWatcher.off(ThemeWatcherEvents.ThemeChange, this.onThemeChanged); this.sgWidget?.off("ready", this.onWidgetReady); this.sgWidget.off("error:preparing", this.updateRequiresClient); this.sgWidget.off("capabilitiesNotified", this.updateRequiresClient); @@ -389,7 +382,6 @@ export default class AppTile extends React.Component<IProps, IState> { private startWidget(): void { this.sgWidget?.prepare().then(() => { if (this.unmounted) return; - if (!this.state.initialising) return; this.setState({ initialising: false }); }); } @@ -464,17 +456,6 @@ export default class AppTile extends React.Component<IProps, IState> { }); }; - private onThemeChanged = (): void => { - // Regenerate widget url when the theme changes - // this updates the url from e.g. `theme=light` to `theme=dark` - // We only do this with EC widgets where the theme prop is in the hash. If the widget puts the - // theme template variable outside the url hash this would cause a (IFrame) page reload on every theme change. - if (WidgetType.CALL.matches(this.props.app.type)) this.setState({ widgetUrl: this.sgWidget?.embedUrl }); - - // TODO: This is a stop gap solution to responsively update the theme of the widget. - // A new action should be introduced and the widget driver should be called here, so it informs the widget. (or connect to this by itself) - }; - private onAction = (payload: ActionPayload): void => { switch (payload.action) { case "m.sticker": @@ -567,9 +548,9 @@ export default class AppTile extends React.Component<IProps, IState> { this.resetWidget(this.props); this.startMessaging(); - if (this.iframe && this.state.widgetUrl) { + if (this.iframe && this.sgWidget) { // Reload iframe - this.iframe.src = this.state.widgetUrl; + this.iframe.src = this.sgWidget.embedUrl; } }); } @@ -638,7 +619,7 @@ export default class AppTile extends React.Component<IProps, IState> { "mx_AppTileBody--mini": this.props.miniMode, "mx_AppTileBody--loading": this.state.loading, // We don't want mx_AppTileBody (rounded corners) for call widgets - "mx_AppTileBody--call": WidgetType.CALL.matches(this.props.app.type), + "mx_AppTileBody--call": this.props.app.type === WidgetType.CALL.preferred, }); const appTileBodyStyles: CSSProperties = {}; if (this.props.pointerEvents) { @@ -667,7 +648,7 @@ export default class AppTile extends React.Component<IProps, IState> { <AppPermission roomId={this.props.room.roomId} creatorUserId={this.props.creatorUserId} - url={this.state.widgetUrl} + url={this.sgWidget.embedUrl} isRoomEncrypted={isEncrypted} onPermissionGranted={this.grantWidgetPermission} /> @@ -695,7 +676,7 @@ export default class AppTile extends React.Component<IProps, IState> { title={widgetTitle} allow={iframeFeatures} ref={this.iframeRefChange} - src={this.state.widgetUrl} + src={this.sgWidget.embedUrl} allowFullScreen={true} sandbox={sandboxFlags} /> @@ -718,12 +699,7 @@ export default class AppTile extends React.Component<IProps, IState> { const zIndexAboveOtherPersistentElements = 101; appTileBody = ( - <div - className="mx_AppTile_persistedWrapper" - // We store the widget url to make it possible to test the value of the widgetUrl. since the iframe itself wont be here. (PersistedElement are in a different dom tree) - data-test-widget-url={this.state.widgetUrl} - data-testid="widget-app-tile" - > + <div className="mx_AppTile_persistedWrapper"> <PersistedElement zIndex={this.props.miniMode ? zIndexAboveOtherPersistentElements : 9} persistKey={this.persistKey} diff --git a/src/settings/watchers/ThemeWatcher.ts b/src/settings/watchers/ThemeWatcher.ts index e4e65d3b3b..79a63f5470 100644 --- a/src/settings/watchers/ThemeWatcher.ts +++ b/src/settings/watchers/ThemeWatcher.ts @@ -16,7 +16,6 @@ limitations under the License. */ import { logger } from "matrix-js-sdk/src/logger"; -import { TypedEventEmitter } from "matrix-js-sdk/src/matrix"; import SettingsStore from "../SettingsStore"; import dis from "../../dispatcher/dispatcher"; @@ -26,15 +25,7 @@ import { findHighContrastTheme, setTheme } from "../../theme"; import { ActionPayload } from "../../dispatcher/payloads"; import { SettingLevel } from "../SettingLevel"; -export enum ThemeWatcherEvents { - ThemeChange = "theme_change", -} - -type EventHandlerMap = { - [ThemeWatcherEvents.ThemeChange]: (theme: string) => void; -}; - -export default class ThemeWatcher extends TypedEventEmitter<ThemeWatcherEvents, EventHandlerMap> { +export default class ThemeWatcher { private themeWatchRef: string | null; private systemThemeWatchRef: string | null; private dispatcherRef: string | null; @@ -46,7 +37,6 @@ export default class ThemeWatcher extends TypedEventEmitter<ThemeWatcherEvents, private currentTheme: string; public constructor() { - super(); this.themeWatchRef = null; this.systemThemeWatchRef = null; this.dispatcherRef = null; @@ -96,7 +86,6 @@ export default class ThemeWatcher extends TypedEventEmitter<ThemeWatcherEvents, this.currentTheme = forceTheme === undefined ? this.getEffectiveTheme() : forceTheme; if (oldTheme !== this.currentTheme) { setTheme(this.currentTheme); - this.emit(ThemeWatcherEvents.ThemeChange, this.currentTheme); } } diff --git a/src/stores/widgets/StopGapWidget.ts b/src/stores/widgets/StopGapWidget.ts index 64e9a980d1..bec4371af2 100644 --- a/src/stores/widgets/StopGapWidget.ts +++ b/src/stores/widgets/StopGapWidget.ts @@ -44,6 +44,7 @@ import { MatrixClientPeg } from "../../MatrixClientPeg"; import { OwnProfileStore } from "../OwnProfileStore"; import WidgetUtils from "../../utils/WidgetUtils"; import { IntegrationManagers } from "../../integrations/IntegrationManagers"; +import SettingsStore from "../../settings/SettingsStore"; import { WidgetType } from "../../widgets/WidgetType"; import ActiveWidgetStore from "../ActiveWidgetStore"; import { objectShallowClone } from "../../utils/objects"; @@ -161,7 +162,6 @@ export class StopGapWidget extends EventEmitter { private readonly virtual: boolean; private readUpToMap: { [roomId: string]: string } = {}; // room ID to event ID private stickyPromise?: () => Promise<void>; // This promise will be called and needs to resolve before the widget will actually become sticky. - private themeWatcher = new ThemeWatcher(); public constructor(private appTileProps: IAppTileProps) { super(); @@ -212,19 +212,13 @@ export class StopGapWidget extends EventEmitter { private runUrlTemplate(opts = { asPopout: false }): string { const fromCustomisation = WidgetVariableCustomisations?.provideVariables?.() ?? {}; - let theme = this.themeWatcher.getEffectiveTheme(); - if (theme.startsWith("custom-")) { - // simplify custom theme to only light/dark - const customTheme = getCustomTheme(theme.slice(7)); - theme = customTheme.is_dark ? "dark" : "light"; - } const defaults: ITemplateParams = { widgetRoomId: this.roomId, currentUserId: this.client.getUserId()!, userDisplayName: OwnProfileStore.instance.displayName ?? undefined, userHttpAvatarUrl: OwnProfileStore.instance.getHttpAvatarUrl() ?? undefined, clientId: ELEMENT_CLIENT_ID, - clientTheme: theme, + clientTheme: SettingsStore.getValue("theme"), clientLanguage: getUserLanguage(), deviceId: this.client.getDeviceId() ?? undefined, baseUrl: this.client.baseUrl, diff --git a/src/theme.ts b/src/theme.ts index d462db466c..8e2e893334 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -121,7 +121,7 @@ function clearCustomTheme(): void { // remove all css variables, we assume these are there because of the custom theme const inlineStyleProps = Object.values(document.body.style); for (const prop of inlineStyleProps) { - if (typeof prop === "string" && prop.startsWith("--")) { + if (prop.startsWith("--")) { document.body.style.removeProperty(prop); } } diff --git a/test/components/views/elements/AppTile-test.tsx b/test/components/views/elements/AppTile-test.tsx index b5510c7f3f..f15f0ce560 100644 --- a/test/components/views/elements/AppTile-test.tsx +++ b/test/components/views/elements/AppTile-test.tsx @@ -19,7 +19,7 @@ import { jest } from "@jest/globals"; import { Room, MatrixClient } from "matrix-js-sdk/src/matrix"; import { ClientWidgetApi, IWidget, MatrixWidgetType } from "matrix-widget-api"; import { Optional } from "matrix-events-sdk"; -import { act, render, RenderResult, waitFor } from "@testing-library/react"; +import { act, render, RenderResult } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { SpiedFunction } from "jest-mock"; import { @@ -50,8 +50,6 @@ import { ElementWidget } from "../../../../src/stores/widgets/StopGapWidget"; import { WidgetMessagingStore } from "../../../../src/stores/widgets/WidgetMessagingStore"; import { ModuleRunner } from "../../../../src/modules/ModuleRunner"; import { RoomPermalinkCreator } from "../../../../src/utils/permalinks/Permalinks"; -import { SettingLevel } from "../../../../src/settings/SettingLevel"; -import { WidgetType } from "../../../../src/widgets/WidgetType"; jest.mock("../../../../src/stores/OwnProfileStore", () => ({ OwnProfileStore: { @@ -70,7 +68,6 @@ describe("AppTile", () => { const resizeNotifier = new ResizeNotifier(); let app1: IApp; let app2: IApp; - let appElementCall: IApp; const waitForRps = (roomId: string) => new Promise<void>((resolve) => { @@ -123,17 +120,6 @@ describe("AppTile", () => { creatorUserId: cli.getSafeUserId(), avatar_url: undefined, }; - appElementCall = { - id: "1", - eventId: "2", - roomId: "r2", - type: WidgetType.CALL.preferred, - url: "https://example.com#theme=$org.matrix.msc2873.client_theme", - name: "Element Call", - creatorUserId: cli.getSafeUserId(), - avatar_url: undefined, - }; - jest.spyOn(WidgetStore.instance, "getApps").mockImplementation((roomId: string): Array<IApp> => { if (roomId === "r1") return [app1]; if (roomId === "r2") return [app2]; @@ -453,6 +439,7 @@ describe("AppTile", () => { expect(moveToContainerSpy).toHaveBeenCalledWith(r1, app1, Container.Top); }); }); + describe("with an existing widgetApi with requiresClient = false", () => { beforeEach(() => { const api = { @@ -479,68 +466,6 @@ describe("AppTile", () => { }); }); - describe("with an element call widget", () => { - beforeEach(() => { - document.body.style.setProperty("--custom-color", "red"); - document.body.style.setProperty("normal-color", "blue"); - }); - it("should update the widget url on theme change", async () => { - const renderResult = render( - <MatrixClientContext.Provider value={cli}> - <a href="http://themeb" data-mx-theme="light"> - A - </a> - <a href="http://themeA" data-mx-theme="dark"> - B - </a> - <AppTile key={appElementCall.id} app={appElementCall} room={r1} /> - </MatrixClientContext.Provider>, - ); - await waitFor(() => { - expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual( - "https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light", - ); - }); - await SettingsStore.setValue("theme", null, SettingLevel.DEVICE, "dark"); - await waitFor(() => { - expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual( - "https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=dark", - ); - }); - await SettingsStore.setValue("theme", null, SettingLevel.DEVICE, "light"); - await waitFor(() => { - expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual( - "https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light", - ); - }); - }); - it("should not update the widget url for non Element Call widgets on theme change", async () => { - const appNonElementCall = { ...appElementCall, type: MatrixWidgetType.Custom }; - const renderResult = render( - <MatrixClientContext.Provider value={cli}> - <a href="http://themeb" data-mx-theme="light"> - A - </a> - <a href="http://themeA" data-mx-theme="dark"> - B - </a> - <AppTile key={appNonElementCall.id} app={appNonElementCall} room={r1} /> - </MatrixClientContext.Provider>, - ); - await waitFor(() => { - expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual( - "https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light", - ); - }); - await SettingsStore.setValue("theme", null, SettingLevel.DEVICE, "dark"); - await waitFor(() => { - expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual( - "https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light", - ); - }); - }); - }); - describe("for a persistent app", () => { let renderResult: RenderResult; diff --git a/test/components/views/elements/__snapshots__/AppTile-test.tsx.snap b/test/components/views/elements/__snapshots__/AppTile-test.tsx.snap index 932306ac5c..92b89013e1 100644 --- a/test/components/views/elements/__snapshots__/AppTile-test.tsx.snap +++ b/test/components/views/elements/__snapshots__/AppTile-test.tsx.snap @@ -85,8 +85,6 @@ exports[`AppTile for a persistent app should render 1`] = ` > <div class="mx_AppTile_persistedWrapper" - data-test-widget-url="https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F" - data-testid="widget-app-tile" > <div /> </div> @@ -174,8 +172,6 @@ exports[`AppTile for a pinned widget should render 1`] = ` </div> <div class="mx_AppTile_persistedWrapper" - data-test-widget-url="https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F" - data-testid="widget-app-tile" > <div /> </div> diff --git a/test/stores/widgets/StopGapWidget-test.ts b/test/stores/widgets/StopGapWidget-test.ts index e190aa91c0..9d1ade0f39 100644 --- a/test/stores/widgets/StopGapWidget-test.ts +++ b/test/stores/widgets/StopGapWidget-test.ts @@ -28,7 +28,6 @@ import { VoiceBroadcastInfoEventType, VoiceBroadcastRecording } from "../../../s import { SdkContextClass } from "../../../src/contexts/SDKContext"; import ActiveWidgetStore from "../../../src/stores/ActiveWidgetStore"; import SettingsStore from "../../../src/settings/SettingsStore"; -import * as Theme from "../../../src/theme"; jest.mock("matrix-widget-api/lib/ClientWidgetApi"); @@ -64,46 +63,18 @@ describe("StopGapWidget", () => { widget.stopMessaging(); }); - describe("url template", () => { - it("should replace parameters in widget url template", () => { - const originalGetValue = SettingsStore.getValue; - const spy = jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { - if (setting === "theme") return "my-theme-for-testing"; - return originalGetValue(setting); - }); - expect(widget.embedUrl).toBe( - "https://example.org/?user-id=%40userId%3Amatrix.org&device-id=ABCDEFGHI&base-url=https%3A%2F%2Fmatrix-client.matrix.org&theme=my-theme-for-testing&widgetId=test&parentUrl=http%3A%2F%2Flocalhost%2F", - ); - spy.mockRestore(); - }); - it("should replace custom theme with light/dark", () => { - const originalGetValue = SettingsStore.getValue; - const spy = jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { - if (setting === "theme") return "custom-my-theme"; - return originalGetValue(setting); - }); - jest.spyOn(Theme, "getCustomTheme").mockReturnValue({ is_dark: false } as unknown as Theme.CustomTheme); - expect(widget.embedUrl).toBe( - "https://example.org/?user-id=%40userId%3Amatrix.org&device-id=ABCDEFGHI&base-url=https%3A%2F%2Fmatrix-client.matrix.org&theme=light&widgetId=test&parentUrl=http%3A%2F%2Flocalhost%2F", - ); - jest.spyOn(Theme, "getCustomTheme").mockReturnValue({ is_dark: true } as unknown as Theme.CustomTheme); - expect(widget.embedUrl).toBe( - "https://example.org/?user-id=%40userId%3Amatrix.org&device-id=ABCDEFGHI&base-url=https%3A%2F%2Fmatrix-client.matrix.org&theme=dark&widgetId=test&parentUrl=http%3A%2F%2Flocalhost%2F", - ); - spy.mockRestore(); - }); - it("should replace parameters in widget popoutUrl template", () => { - const originalGetValue = SettingsStore.getValue; - const spy = jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { - if (setting === "theme") return "my-theme-for-testing"; - return originalGetValue(setting); - }); - expect(widget.popoutUrl).toBe( - "https://example.org/?user-id=%40userId%3Amatrix.org&device-id=ABCDEFGHI&base-url=https%3A%2F%2Fmatrix-client.matrix.org&theme=my-theme-for-testing", - ); - spy.mockRestore(); + it("should replace parameters in widget url template", () => { + const originGetValue = SettingsStore.getValue; + const spy = jest.spyOn(SettingsStore, "getValue").mockImplementation((setting) => { + if (setting === "theme") return "my-theme-for-testing"; + return originGetValue(setting); }); + expect(widget.embedUrl).toBe( + "https://example.org/?user-id=%40userId%3Amatrix.org&device-id=ABCDEFGHI&base-url=https%3A%2F%2Fmatrix-client.matrix.org&theme=my-theme-for-testing&widgetId=test&parentUrl=http%3A%2F%2Flocalhost%2F", + ); + spy.mockClear(); }); + it("feeds incoming to-device messages to the widget", async () => { const event = mkEvent({ event: true,