From dcf7643d4aee3c3349676c866406016587c91cd8 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 5 Jul 2024 14:39:13 +0100 Subject: [PATCH] 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 --- src/Modal.tsx | 37 +++++++++++++++---- src/components/structures/LoggedInView.tsx | 8 +++- src/components/structures/MatrixChat.tsx | 2 +- test/Modal-test.ts | 32 ++++++++++++++++ .../structures/LoggedInView-test.tsx | 19 ++++++++++ .../RoomGeneralContextMenu-test.tsx | 6 +-- .../views/dialogs/InviteDialog-test.tsx | 6 +-- test/test-utils/utilities.ts | 2 +- 8 files changed, 95 insertions(+), 17 deletions(-) create mode 100644 test/Modal-test.ts diff --git a/src/Modal.tsx b/src/Modal.tsx index d93e114a53..b72180097c 100644 --- a/src/Modal.tsx +++ b/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 { elem: React.ReactNode; className?: string; beforeClosePromise?: Promise; - closeReason?: string; - onBeforeClose?(reason?: string): Promise; + closeReason?: ModalCloseReason; + onBeforeClose?(reason?: ModalCloseReason): Promise; onFinished: ComponentProps["onFinished"]; close(...args: Parameters["onFinished"]>): void; hidden?: boolean; + deferred?: IDeferred["onFinished"]>>; } export interface IHandle { @@ -73,6 +74,8 @@ type HandlerMap = { [ModalManagerEvent.Closed]: () => void; }; +type ModalCloseReason = "backgroundClick"; + export class ModalManager extends TypedEventEmitter { 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( prom: Promise, props?: ComponentProps, @@ -199,7 +222,7 @@ export class ModalManager extends TypedEventEmitter, props?: ComponentProps, ): [IHandle["close"], IHandle["finished"]] { - const deferred = defer["onFinished"]>>(); + modal.deferred = defer["onFinished"]>>(); return [ async (...args: Parameters["onFinished"]>): Promise => { if (modal.beforeClosePromise) { @@ -212,7 +235,7 @@ export class ModalManager extends TypedEventEmitter= 0) { @@ -236,7 +259,7 @@ export class ModalManager extends TypedEventEmitter { 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); diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index d3bae478d8..70c4f9ce5f 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -1544,7 +1544,7 @@ export default class MatrixChat extends React.PureComponent { 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"); diff --git a/test/Modal-test.ts b/test/Modal-test.ts new file mode 100644 index 0000000000..d7630bd2da --- /dev/null +++ b/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); + }); +}); diff --git a/test/components/structures/LoggedInView-test.tsx b/test/components/structures/LoggedInView-test.tsx index 27ab93f2b8..da21ac7c90 100644 --- a/test/components/structures/LoggedInView-test.tsx +++ b/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("", () => { const userId = "@alice:domain.org"; @@ -398,4 +399,22 @@ describe("", () => { 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{/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{/Control}"); + expect(defaultDispatcher.dispatch).not.toHaveBeenCalledWith({ action: Action.ViewHomePage }); + }); }); diff --git a/test/components/views/context_menus/RoomGeneralContextMenu-test.tsx b/test/components/views/context_menus/RoomGeneralContextMenu-test.tsx index d2fc8b95a6..c64e0b2e6b 100644 --- a/test/components/views/context_menus/RoomGeneralContextMenu-test.tsx +++ b/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 () => { diff --git a/test/components/views/dialogs/InviteDialog-test.tsx b/test/components/views/dialogs/InviteDialog-test.tsx index 16f756cb01..4e1dca4193 100644 --- a/test/components/views/dialogs/InviteDialog-test.tsx +++ b/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; }); diff --git a/test/test-utils/utilities.ts b/test/test-utils/utilities.ts index 6765fb754b..22153e2535 100644 --- a/test/test-utils/utilities.ts +++ b/test/test-utils/utilities.ts @@ -204,7 +204,7 @@ export const clearAllModals = async (): Promise => { // 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