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

Author: dbkr

src/Modal.tsx

@@ -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,
         ];
     }
 

src/components/structures/LoggedInView.tsx

@@ -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);

src/components/structures/MatrixChat.tsx

@@ -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");

test/Modal-test.ts

@@ -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);
+    });
+});

test/components/structures/LoggedInView-test.tsx

@@ -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 });
+    });
 });

test/components/views/context_menus/RoomGeneralContextMenu-test.tsx

@@ -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 () => {

test/components/views/dialogs/InviteDialog-test.tsx

@@ -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;
     });

test/test-utils/utilities.ts

@@ -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