Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

[Backport staging] Revert "Make EC widget theme reactive - Update widget url when the theme changes" #12383

Merged
merged 1 commit into from
Mar 28, 2024
Merged

[Backport staging] Revert "Make EC widget theme reactive - Update widget url when the theme changes" #12383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
38 changes: 7 additions & 31 deletions src/components/views/elements/AppTile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -116,7 +115,6 @@ interface IState {
menuDisplayed: boolean;
requiresClient: boolean;
hasContextMenuOptions: boolean;
widgetUrl?: string;
}

export default class AppTile extends React.Component<IProps, IState> {
Expand All @@ -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
Expand Down Expand Up @@ -269,7 +267,6 @@ export default class AppTile extends React.Component<IProps, IState> {
!newProps.userWidget,
newProps.onDeleteClick,
),
widgetUrl: this.sgWidget?.embedUrl,
};
}

Expand Down Expand Up @@ -355,18 +352,14 @@ 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
this.sgWidget?.on("capabilitiesNotified", this.updateRequiresClient);
}

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);
Expand All @@ -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 });
});
}
Expand Down Expand Up @@ -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":
Expand Down Expand Up @@ -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;
}
});
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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}
/>
Expand Down Expand Up @@ -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}
/>
Expand All @@ -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}
Expand Down
13 changes: 1 addition & 12 deletions src/settings/watchers/ThemeWatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
}

Expand Down
10 changes: 2 additions & 8 deletions src/stores/widgets/StopGapWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/theme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
79 changes: 2 additions & 77 deletions test/components/views/elements/AppTile-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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: {
Expand All @@ -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) => {
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -453,6 +439,7 @@ describe("AppTile", () => {
expect(moveToContainerSpy).toHaveBeenCalledWith(r1, app1, Container.Top);
});
});

describe("with an existing widgetApi with requiresClient = false", () => {
beforeEach(() => {
const api = {
Expand All @@ -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;

Expand Down
4 changes: 0 additions & 4 deletions test/components/views/elements/__snapshots__/AppTile-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
Loading
Loading