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

Commit e314d3b

Browse files
dbkrdbkr
authored
Merge pull request #12383 from matrix-org/backport-12382-to-staging
[Backport staging] Revert "Make EC widget theme reactive - Update widget url when the theme changes"
2 parents faf7b04 + 2603003 commit e314d3b

File tree

7 files changed

+23
-172
lines changed

7 files changed

+23
-172
lines changed

‎src/components/views/elements/AppTile.tsx‎

Lines changed: 7 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ import { WidgetMessagingStore } from "../../../stores/widgets/WidgetMessagingSto
5757
import { SdkContextClass } from "../../../contexts/SDKContext";
5858
import { ModuleRunner } from "../../../modules/ModuleRunner";
5959
import { parseUrl } from "../../../utils/UrlUtils";
60-
import ThemeWatcher, { ThemeWatcherEvents } from "../../../settings/watchers/ThemeWatcher";
6160

6261
interface IProps {
6362
app: IWidget | IApp;
@@ -116,7 +115,6 @@ interface IState {
116115
menuDisplayed: boolean;
117116
requiresClient: boolean;
118117
hasContextMenuOptions: boolean;
119-
widgetUrl?: string;
120118
}
121119

122120
export default class AppTile extends React.Component<IProps, IState> {
@@ -142,7 +140,7 @@ export default class AppTile extends React.Component<IProps, IState> {
142140
private sgWidget: StopGapWidget | null;
143141
private dispatcherRef?: string;
144142
private unmounted = false;
145-
private themeWatcher = new ThemeWatcher();
143+
146144
public constructor(props: IProps, context: ContextType<typeof MatrixClientContext>) {
147145
super(props);
148146
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> {
269267
!newProps.userWidget,
270268
newProps.onDeleteClick,
271269
),
272-
widgetUrl: this.sgWidget?.embedUrl,
273270
};
274271
}
275272

@@ -355,18 +352,14 @@ export default class AppTile extends React.Component<IProps, IState> {
355352
}
356353

357354
private setupSgListeners(): void {
358-
this.themeWatcher.on(ThemeWatcherEvents.ThemeChange, this.onThemeChanged);
359-
this.themeWatcher.start();
360355
this.sgWidget?.on("ready", this.onWidgetReady);
361356
this.sgWidget?.on("error:preparing", this.updateRequiresClient);
362357
// emits when the capabilities have been set up or changed
363358
this.sgWidget?.on("capabilitiesNotified", this.updateRequiresClient);
364359
}
365360

366361
private stopSgListeners(): void {
367-
this.themeWatcher.stop();
368362
if (!this.sgWidget) return;
369-
this.themeWatcher.off(ThemeWatcherEvents.ThemeChange, this.onThemeChanged);
370363
this.sgWidget?.off("ready", this.onWidgetReady);
371364
this.sgWidget.off("error:preparing", this.updateRequiresClient);
372365
this.sgWidget.off("capabilitiesNotified", this.updateRequiresClient);
@@ -389,7 +382,6 @@ export default class AppTile extends React.Component<IProps, IState> {
389382
private startWidget(): void {
390383
this.sgWidget?.prepare().then(() => {
391384
if (this.unmounted) return;
392-
if (!this.state.initialising) return;
393385
this.setState({ initialising: false });
394386
});
395387
}
@@ -464,17 +456,6 @@ export default class AppTile extends React.Component<IProps, IState> {
464456
});
465457
};
466458

467-
private onThemeChanged = (): void => {
468-
// Regenerate widget url when the theme changes
469-
// this updates the url from e.g. `theme=light` to `theme=dark`
470-
// We only do this with EC widgets where the theme prop is in the hash. If the widget puts the
471-
// theme template variable outside the url hash this would cause a (IFrame) page reload on every theme change.
472-
if (WidgetType.CALL.matches(this.props.app.type)) this.setState({ widgetUrl: this.sgWidget?.embedUrl });
473-
474-
// TODO: This is a stop gap solution to responsively update the theme of the widget.
475-
// 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)
476-
};
477-
478459
private onAction = (payload: ActionPayload): void => {
479460
switch (payload.action) {
480461
case "m.sticker":
@@ -567,9 +548,9 @@ export default class AppTile extends React.Component<IProps, IState> {
567548
this.resetWidget(this.props);
568549
this.startMessaging();
569550

570-
if (this.iframe && this.state.widgetUrl) {
551+
if (this.iframe && this.sgWidget) {
571552
// Reload iframe
572-
this.iframe.src = this.state.widgetUrl;
553+
this.iframe.src = this.sgWidget.embedUrl;
573554
}
574555
});
575556
}
@@ -638,7 +619,7 @@ export default class AppTile extends React.Component<IProps, IState> {
638619
"mx_AppTileBody--mini": this.props.miniMode,
639620
"mx_AppTileBody--loading": this.state.loading,
640621
// We don't want mx_AppTileBody (rounded corners) for call widgets
641-
"mx_AppTileBody--call": WidgetType.CALL.matches(this.props.app.type),
622+
"mx_AppTileBody--call": this.props.app.type === WidgetType.CALL.preferred,
642623
});
643624
const appTileBodyStyles: CSSProperties = {};
644625
if (this.props.pointerEvents) {
@@ -667,7 +648,7 @@ export default class AppTile extends React.Component<IProps, IState> {
667648
<AppPermission
668649
roomId={this.props.room.roomId}
669650
creatorUserId={this.props.creatorUserId}
670-
url={this.state.widgetUrl}
651+
url={this.sgWidget.embedUrl}
671652
isRoomEncrypted={isEncrypted}
672653
onPermissionGranted={this.grantWidgetPermission}
673654
/>
@@ -695,7 +676,7 @@ export default class AppTile extends React.Component<IProps, IState> {
695676
title={widgetTitle}
696677
allow={iframeFeatures}
697678
ref={this.iframeRefChange}
698-
src={this.state.widgetUrl}
679+
src={this.sgWidget.embedUrl}
699680
allowFullScreen={true}
700681
sandbox={sandboxFlags}
701682
/>
@@ -718,12 +699,7 @@ export default class AppTile extends React.Component<IProps, IState> {
718699
const zIndexAboveOtherPersistentElements = 101;
719700

720701
appTileBody = (
721-
<div
722-
className="mx_AppTile_persistedWrapper"
723-
// 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)
724-
data-test-widget-url={this.state.widgetUrl}
725-
data-testid="widget-app-tile"
726-
>
702+
<div className="mx_AppTile_persistedWrapper">
727703
<PersistedElement
728704
zIndex={this.props.miniMode ? zIndexAboveOtherPersistentElements : 9}
729705
persistKey={this.persistKey}

‎src/settings/watchers/ThemeWatcher.ts‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ limitations under the License.
1616
*/
1717

1818
import { logger } from "matrix-js-sdk/src/logger";
19-
import { TypedEventEmitter } from "matrix-js-sdk/src/matrix";
2019

2120
import SettingsStore from "../SettingsStore";
2221
import dis from "../../dispatcher/dispatcher";
@@ -26,15 +25,7 @@ import { findHighContrastTheme, setTheme } from "../../theme";
2625
import { ActionPayload } from "../../dispatcher/payloads";
2726
import { SettingLevel } from "../SettingLevel";
2827

29-
export enum ThemeWatcherEvents {
30-
ThemeChange = "theme_change",
31-
}
32-
33-
type EventHandlerMap = {
34-
[ThemeWatcherEvents.ThemeChange]: (theme: string) => void;
35-
};
36-
37-
export default class ThemeWatcher extends TypedEventEmitter<ThemeWatcherEvents, EventHandlerMap> {
28+
export default class ThemeWatcher {
3829
private themeWatchRef: string | null;
3930
private systemThemeWatchRef: string | null;
4031
private dispatcherRef: string | null;
@@ -46,7 +37,6 @@ export default class ThemeWatcher extends TypedEventEmitter<ThemeWatcherEvents,
4637
private currentTheme: string;
4738

4839
public constructor() {
49-
super();
5040
this.themeWatchRef = null;
5141
this.systemThemeWatchRef = null;
5242
this.dispatcherRef = null;
@@ -96,7 +86,6 @@ export default class ThemeWatcher extends TypedEventEmitter<ThemeWatcherEvents,
9686
this.currentTheme = forceTheme === undefined ? this.getEffectiveTheme() : forceTheme;
9787
if (oldTheme !== this.currentTheme) {
9888
setTheme(this.currentTheme);
99-
this.emit(ThemeWatcherEvents.ThemeChange, this.currentTheme);
10089
}
10190
}
10291

‎src/stores/widgets/StopGapWidget.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import { MatrixClientPeg } from "../../MatrixClientPeg";
4444
import { OwnProfileStore } from "../OwnProfileStore";
4545
import WidgetUtils from "../../utils/WidgetUtils";
4646
import { IntegrationManagers } from "../../integrations/IntegrationManagers";
47+
import SettingsStore from "../../settings/SettingsStore";
4748
import { WidgetType } from "../../widgets/WidgetType";
4849
import ActiveWidgetStore from "../ActiveWidgetStore";
4950
import { objectShallowClone } from "../../utils/objects";
@@ -161,7 +162,6 @@ export class StopGapWidget extends EventEmitter {
161162
private readonly virtual: boolean;
162163
private readUpToMap: { [roomId: string]: string } = {}; // room ID to event ID
163164
private stickyPromise?: () => Promise<void>; // This promise will be called and needs to resolve before the widget will actually become sticky.
164-
private themeWatcher = new ThemeWatcher();
165165

166166
public constructor(private appTileProps: IAppTileProps) {
167167
super();
@@ -212,19 +212,13 @@ export class StopGapWidget extends EventEmitter {
212212

213213
private runUrlTemplate(opts = { asPopout: false }): string {
214214
const fromCustomisation = WidgetVariableCustomisations?.provideVariables?.() ?? {};
215-
let theme = this.themeWatcher.getEffectiveTheme();
216-
if (theme.startsWith("custom-")) {
217-
// simplify custom theme to only light/dark
218-
const customTheme = getCustomTheme(theme.slice(7));
219-
theme = customTheme.is_dark ? "dark" : "light";
220-
}
221215
const defaults: ITemplateParams = {
222216
widgetRoomId: this.roomId,
223217
currentUserId: this.client.getUserId()!,
224218
userDisplayName: OwnProfileStore.instance.displayName ?? undefined,
225219
userHttpAvatarUrl: OwnProfileStore.instance.getHttpAvatarUrl() ?? undefined,
226220
clientId: ELEMENT_CLIENT_ID,
227-
clientTheme: theme,
221+
clientTheme: SettingsStore.getValue("theme"),
228222
clientLanguage: getUserLanguage(),
229223
deviceId: this.client.getDeviceId() ?? undefined,
230224
baseUrl: this.client.baseUrl,

‎src/theme.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ function clearCustomTheme(): void {
121121
// remove all css variables, we assume these are there because of the custom theme
122122
const inlineStyleProps = Object.values(document.body.style);
123123
for (const prop of inlineStyleProps) {
124-
if (typeof prop === "string" && prop.startsWith("--")) {
124+
if (prop.startsWith("--")) {
125125
document.body.style.removeProperty(prop);
126126
}
127127
}

‎test/components/views/elements/AppTile-test.tsx‎

Lines changed: 2 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { jest } from "@jest/globals";
1919
import { Room, MatrixClient } from "matrix-js-sdk/src/matrix";
2020
import { ClientWidgetApi, IWidget, MatrixWidgetType } from "matrix-widget-api";
2121
import { Optional } from "matrix-events-sdk";
22-
import { act, render, RenderResult, waitFor } from "@testing-library/react";
22+
import { act, render, RenderResult } from "@testing-library/react";
2323
import userEvent from "@testing-library/user-event";
2424
import { SpiedFunction } from "jest-mock";
2525
import {
@@ -50,8 +50,6 @@ import { ElementWidget } from "../../../../src/stores/widgets/StopGapWidget";
5050
import { WidgetMessagingStore } from "../../../../src/stores/widgets/WidgetMessagingStore";
5151
import { ModuleRunner } from "../../../../src/modules/ModuleRunner";
5252
import { RoomPermalinkCreator } from "../../../../src/utils/permalinks/Permalinks";
53-
import { SettingLevel } from "../../../../src/settings/SettingLevel";
54-
import { WidgetType } from "../../../../src/widgets/WidgetType";
5553

5654
jest.mock("../../../../src/stores/OwnProfileStore", () => ({
5755
OwnProfileStore: {
@@ -70,7 +68,6 @@ describe("AppTile", () => {
7068
const resizeNotifier = new ResizeNotifier();
7169
let app1: IApp;
7270
let app2: IApp;
73-
let appElementCall: IApp;
7471

7572
const waitForRps = (roomId: string) =>
7673
new Promise<void>((resolve) => {
@@ -123,17 +120,6 @@ describe("AppTile", () => {
123120
creatorUserId: cli.getSafeUserId(),
124121
avatar_url: undefined,
125122
};
126-
appElementCall = {
127-
id: "1",
128-
eventId: "2",
129-
roomId: "r2",
130-
type: WidgetType.CALL.preferred,
131-
url: "https://example.com#theme=$org.matrix.msc2873.client_theme",
132-
name: "Element Call",
133-
creatorUserId: cli.getSafeUserId(),
134-
avatar_url: undefined,
135-
};
136-
137123
jest.spyOn(WidgetStore.instance, "getApps").mockImplementation((roomId: string): Array<IApp> => {
138124
if (roomId === "r1") return [app1];
139125
if (roomId === "r2") return [app2];
@@ -453,6 +439,7 @@ describe("AppTile", () => {
453439
expect(moveToContainerSpy).toHaveBeenCalledWith(r1, app1, Container.Top);
454440
});
455441
});
442+
456443
describe("with an existing widgetApi with requiresClient = false", () => {
457444
beforeEach(() => {
458445
const api = {
@@ -479,68 +466,6 @@ describe("AppTile", () => {
479466
});
480467
});
481468

482-
describe("with an element call widget", () => {
483-
beforeEach(() => {
484-
document.body.style.setProperty("--custom-color", "red");
485-
document.body.style.setProperty("normal-color", "blue");
486-
});
487-
it("should update the widget url on theme change", async () => {
488-
const renderResult = render(
489-
<MatrixClientContext.Provider value={cli}>
490-
<a href="http://themeb" data-mx-theme="light">
491-
A
492-
</a>
493-
<a href="http://themeA" data-mx-theme="dark">
494-
B
495-
</a>
496-
<AppTile key={appElementCall.id} app={appElementCall} room={r1} />
497-
</MatrixClientContext.Provider>,
498-
);
499-
await waitFor(() => {
500-
expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual(
501-
"https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light",
502-
);
503-
});
504-
await SettingsStore.setValue("theme", null, SettingLevel.DEVICE, "dark");
505-
await waitFor(() => {
506-
expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual(
507-
"https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=dark",
508-
);
509-
});
510-
await SettingsStore.setValue("theme", null, SettingLevel.DEVICE, "light");
511-
await waitFor(() => {
512-
expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual(
513-
"https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light",
514-
);
515-
});
516-
});
517-
it("should not update the widget url for non Element Call widgets on theme change", async () => {
518-
const appNonElementCall = { ...appElementCall, type: MatrixWidgetType.Custom };
519-
const renderResult = render(
520-
<MatrixClientContext.Provider value={cli}>
521-
<a href="http://themeb" data-mx-theme="light">
522-
A
523-
</a>
524-
<a href="http://themeA" data-mx-theme="dark">
525-
B
526-
</a>
527-
<AppTile key={appNonElementCall.id} app={appNonElementCall} room={r1} />
528-
</MatrixClientContext.Provider>,
529-
);
530-
await waitFor(() => {
531-
expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual(
532-
"https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light",
533-
);
534-
});
535-
await SettingsStore.setValue("theme", null, SettingLevel.DEVICE, "dark");
536-
await waitFor(() => {
537-
expect(renderResult.getByTestId("widget-app-tile").dataset.testWidgetUrl).toEqual(
538-
"https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F#theme=light",
539-
);
540-
});
541-
});
542-
});
543-
544469
describe("for a persistent app", () => {
545470
let renderResult: RenderResult;
546471

‎test/components/views/elements/__snapshots__/AppTile-test.tsx.snap‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,6 @@ exports[`AppTile for a persistent app should render 1`] = `
8585
>
8686
<div
8787
class="mx_AppTile_persistedWrapper"
88-
data-test-widget-url="https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F"
89-
data-testid="widget-app-tile"
9088
>
9189
<div />
9290
</div>
@@ -174,8 +172,6 @@ exports[`AppTile for a pinned widget should render 1`] = `
174172
</div>
175173
<div
176174
class="mx_AppTile_persistedWrapper"
177-
data-test-widget-url="https://example.com/?widgetId=1&parentUrl=http%3A%2F%2Flocalhost%2F"
178-
data-testid="widget-app-tile"
179175
>
180176
<div />
181177
</div>

0 commit comments

Comments
 (0)