OpenCloud
/
riot-web
mirror of https://github.com/vector-im/riot-web
1
0
Fork 0
Code Issues Releases Wiki Activity

Fix closing all modals (#12728) 

* Fix closing all modals

We used `Modal.closeCurrentModal()` in a bunch of places, in all cases
(as far as I can see: it wasn't commented) we meant to close all open
modals. This swaps that function for one that closes all open modals.

Also types the close reason which claimed to be something in a comment,
of course, was wrong because a load of places passed their own random
string which was never used.

* Force close modals

* Try with minimal changes

* Already had a method for this

* Add test

* More tests

* Unused importsd
pull/28217/head
David Baker 2024-07-05 14:39:13 +01:00 committed by GitHub
parent a7542dc0ac
commit dcf7643d4a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 95 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
src/Modal.tsx
View File

@ -18,7 +18,7 @@ limitations under the License.
import React from "react";
import ReactDOM from "react-dom";
import classNames from "classnames";
import { defer, sleep } from "matrix-js-sdk/src/utils";
import { IDeferred, defer, sleep } from "matrix-js-sdk/src/utils";
import { TypedEventEmitter } from "matrix-js-sdk/src/matrix";
import { Glass } from "@vector-im/compound-web";
@ -47,11 +47,12 @@ export interface IModal<C extends ComponentType> {
elem: React.ReactNode;
className?: string;
beforeClosePromise?: Promise<boolean>;
closeReason?: string;
onBeforeClose?(reason?: string): Promise<boolean>;
closeReason?: ModalCloseReason;
onBeforeClose?(reason?: ModalCloseReason): Promise<boolean>;
onFinished: ComponentProps<C>["onFinished"];
close(...args: Parameters<ComponentProps<C>["onFinished"]>): void;
hidden?: boolean;
deferred?: IDeferred<Parameters<ComponentProps<C>["onFinished"]>>;
}
export interface IHandle<C extends ComponentType> {
@ -73,6 +74,8 @@ type HandlerMap = {
[ModalManagerEvent.Closed]: () => void;
};
type ModalCloseReason = "backgroundClick";
export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMap> {
private counter = 0;
// The modal to prioritise over all others. If this is set, only show
@ -148,10 +151,14 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
}
/**
* DEPRECATED.
* This is used only for tests. They should be using forceCloseAllModals but that
* caused a chunk of tests to fail, so for now they continue to use this.
*
* @param reason either "backgroundClick" or undefined
* @return whether a modal was closed
*/
public closeCurrentModal(reason?: string): boolean {
public closeCurrentModal(reason?: ModalCloseReason): boolean {
const modal = this.getCurrentModal();
if (!modal) {
return false;
@ -161,6 +168,22 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
return true;
}
/**
* Forces closes all open modals. The modals onBeforeClose function will not be
* run and the modal will not have a chance to prevent closing. Intended for
* situations like the user logging out of the app.
*/
public forceCloseAllModals(): void {
for (const modal of this.modals) {
modal.deferred?.resolve([]);
if (modal.onFinished) modal.onFinished.apply(null);
this.emitClosed();
}
this.modals = [];
this.reRender();
}
private buildModal<C extends ComponentType>(
prom: Promise<C>,
props?: ComponentProps<C>,
@ -199,7 +222,7 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
modal: IModal<C>,
props?: ComponentProps<C>,
): [IHandle<C>["close"], IHandle<C>["finished"]] {
const deferred = defer<Parameters<ComponentProps<C>["onFinished"]>>();
modal.deferred = defer<Parameters<ComponentProps<C>["onFinished"]>>();
return [
async (...args: Parameters<ComponentProps<C>["onFinished"]>): Promise<void> => {
if (modal.beforeClosePromise) {
@ -212,7 +235,7 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
return;
}
}
deferred.resolve(args);
modal.deferred?.resolve(args);
if (props?.onFinished) props.onFinished.apply(null, args);
const i = this.modals.indexOf(modal);
if (i >= 0) {
@ -236,7 +259,7 @@ export class ModalManager extends TypedEventEmitter<ModalManagerEvent, HandlerMa
this.reRender();
this.emitClosed();
},
deferred.promise,
modal.deferred.promise,
];
}

8
src/components/structures/LoggedInView.tsx
View File

@ -488,11 +488,15 @@ class LoggedInView extends React.Component<IProps, IState> {
handled = true;
break;
case KeyBindingAction.GoToHome:
// even if we cancel because there are modals open, we still
// handled it: nothing else should happen.
handled = true;
if (Modal.hasDialogs()) {
return;
}
dis.dispatch({
action: Action.ViewHomePage,
});
Modal.closeCurrentModal("homeKeyboardShortcut");
handled = true;
break;
case KeyBindingAction.ToggleSpacePanel:
dis.fire(Action.ToggleSpacePanel);

2
src/components/structures/MatrixChat.tsx
View File

@ -1544,7 +1544,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
if (Lifecycle.isLoggingOut()) return;
// A modal might have been open when we were logged out by the server
Modal.closeCurrentModal("Session.logged_out");
Modal.forceCloseAllModals();
if (errObj.httpStatus === 401 && errObj.data && errObj.data["soft_logout"]) {
logger.warn("Soft logout issued by server - avoiding data deletion");

32
test/Modal-test.ts Normal file
View File

@ -0,0 +1,32 @@
/*
Copyright 2024 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
import Modal from "../src/Modal";
import QuestionDialog from "../src/components/views/dialogs/QuestionDialog";
describe("Modal", () => {
test("forceCloseAllModals should close all open modals", () => {
Modal.createDialog(QuestionDialog, {
title: "Test dialog",
description: "This is a test dialog",
button: "Word",
});
expect(Modal.hasDialogs()).toBe(true);
Modal.forceCloseAllModals();
expect(Modal.hasDialogs()).toBe(false);
});
});

19
test/components/structures/LoggedInView-test.tsx
View File

@ -31,6 +31,7 @@ import defaultDispatcher from "../../../src/dispatcher/dispatcher";
import SettingsStore from "../../../src/settings/SettingsStore";
import { SettingLevel } from "../../../src/settings/SettingLevel";
import { Action } from "../../../src/dispatcher/actions";
import Modal from "../../../src/Modal";
describe("<LoggedInView />", () => {
const userId = "@alice:domain.org";
@ -398,4 +399,22 @@ describe("<LoggedInView />", () => {
await userEvent.keyboard("{Control>}f{/Control}");
expect(defaultDispatcher.fire).toHaveBeenCalledWith(Action.FocusMessageSearch);
});
it("should go home on home shortcut", async () => {
jest.spyOn(defaultDispatcher, "dispatch");
getComponent();
await userEvent.keyboard("{Control>}{Alt>}h</Alt>{/Control}");
expect(defaultDispatcher.dispatch).toHaveBeenCalledWith({ action: Action.ViewHomePage });
});
it("should ignore home shortcut if dialogs are open", async () => {
jest.spyOn(defaultDispatcher, "dispatch");
jest.spyOn(Modal, "hasDialogs").mockReturnValue(true);
getComponent();
await userEvent.keyboard("{Control>}{Alt>}h</Alt>{/Control}");
expect(defaultDispatcher.dispatch).not.toHaveBeenCalledWith({ action: Action.ViewHomePage });
});
});

6
test/components/views/context_menus/RoomGeneralContextMenu-test.tsx
View File

@ -36,7 +36,7 @@ import { mkMessage, stubClient } from "../../../test-utils/test-utils";
import { shouldShowComponent } from "../../../../src/customisations/helpers/UIComponents";
import { UIComponent } from "../../../../src/settings/UIFeature";
import SettingsStore from "../../../../src/settings/SettingsStore";
import Modal from "../../../../src/Modal";
import { clearAllModals } from "../../../test-utils";
jest.mock("../../../../src/customisations/helpers/UIComponents", () => ({
shouldShowComponent: jest.fn(),
@ -90,8 +90,8 @@ describe("RoomGeneralContextMenu", () => {
onFinished = jest.fn();
});
afterEach(() => {
Modal.closeCurrentModal("force");
afterEach(async () => {
await clearAllModals();
});
it("renders an empty context menu for archived rooms", async () => {

6
test/components/views/dialogs/InviteDialog-test.tsx
View File

@ -25,6 +25,7 @@ import { mocked, Mocked } from "jest-mock";
import InviteDialog from "../../../../src/components/views/dialogs/InviteDialog";
import { InviteKind } from "../../../../src/components/views/dialogs/InviteDialogTypes";
import {
clearAllModals,
filterConsole,
flushPromises,
getMockClientWithEventEmitter,
@ -40,7 +41,6 @@ import { SdkContextClass } from "../../../../src/contexts/SDKContext";
import { IProfileInfo } from "../../../../src/hooks/useProfileInfo";
import { DirectoryMember, startDmOnFirstMessage } from "../../../../src/utils/direct-messages";
import SettingsStore from "../../../../src/settings/SettingsStore";
import Modal from "../../../../src/Modal";
const mockGetAccessToken = jest.fn().mockResolvedValue("getAccessToken");
jest.mock("../../../../src/IdentityAuthClient", () =>
@ -178,8 +178,8 @@ describe("InviteDialog", () => {
SdkContextClass.instance.client = mockClient;
});
afterEach(() => {
Modal.closeCurrentModal();
afterEach(async () => {
await clearAllModals();
SdkContextClass.instance.onLoggedOut();
SdkContextClass.instance.client = undefined;
});

2
test/test-utils/utilities.ts
View File

@ -204,7 +204,7 @@ export const clearAllModals = async (): Promise<void> => {
// Prevent modals from leaking and polluting other tests
let keepClosingModals = true;
while (keepClosingModals) {
keepClosingModals = Modal.closeCurrentModal("End of test (clean-up)");
keepClosingModals = Modal.closeCurrentModal();
// Then wait for the screen to update (probably React rerender and async/await).
// Important for tests using Jest fake timers to not get into an infinite loop