From be7d6e94e9a3ddadb7f750ca425d617194ebc5b2 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 9 Nov 2022 09:58:56 +0000 Subject: [PATCH 1/5] Close context menu when a modal is opened to prevent user getting stuck --- src/Modal.tsx | 26 ++++++++++++++++++++++- src/components/structures/ContextMenu.tsx | 14 ++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index 465f3cdad21..9aafd4ef8aa 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -19,6 +19,7 @@ import React from 'react'; import ReactDOM from 'react-dom'; import classNames from 'classnames'; import { defer, sleep } from "matrix-js-sdk/src/utils"; +import { TypedEventEmitter } from 'matrix-js-sdk/src/models/typed-event-emitter'; import dis from './dispatcher/dispatcher'; import AsyncWrapper from './AsyncWrapper'; @@ -54,7 +55,15 @@ interface IOptions { type ParametersWithoutFirst any> = T extends (a: any, ...args: infer P) => any ? P : never; -export class ModalManager { +export enum ModalManagerEvent { + Opened = "opened", +} + +type HandlerMap = { + [ModalManagerEvent.Opened]: () => void; +}; + +export class ModalManager extends TypedEventEmitter { private counter = 0; // The modal to prioritise over all others. If this is set, only show // this modal. Remove all other modals from the stack when this modal @@ -244,6 +253,7 @@ export class ModalManager { isStaticModal = false, options: IOptions = {}, ): IHandle { + const beforeModal = this.getCurrentModal(); const { modal, closeDialog, onFinishedProm } = this.buildModal(prom, props, className, options); if (isPriorityModal) { // XXX: This is destructive @@ -255,7 +265,13 @@ export class ModalManager { this.modals.unshift(modal); } + const currentModal = this.getCurrentModal(); this.reRender(); + + if (beforeModal !== currentModal) { + this.emit(ModalManagerEvent.Opened); + } + return { close: closeDialog, finished: onFinishedProm, @@ -267,10 +283,18 @@ export class ModalManager { props?: IProps, className?: string, ): IHandle { + const beforeModal = this.getCurrentModal(); const { modal, closeDialog, onFinishedProm } = this.buildModal(prom, props, className, {}); this.modals.push(modal); + + const currentModal = this.getCurrentModal(); this.reRender(); + + if (beforeModal !== currentModal) { + this.emit(ModalManagerEvent.Opened); + } + return { close: closeDialog, finished: onFinishedProm, diff --git a/src/components/structures/ContextMenu.tsx b/src/components/structures/ContextMenu.tsx index 136f036f708..cf9aacb8084 100644 --- a/src/components/structures/ContextMenu.tsx +++ b/src/components/structures/ContextMenu.tsx @@ -26,6 +26,7 @@ import UIStore from "../../stores/UIStore"; import { checkInputableElement, RovingTabIndexProvider } from "../../accessibility/RovingTabIndex"; import { KeyBindingAction } from "../../accessibility/KeyboardShortcuts"; import { getKeyBindingsManager } from "../../KeyBindingsManager"; +import Modal, { ModalManagerEvent } from "../../Modal"; // Shamelessly ripped off Modal.js. There's probably a better way // of doing reusable widgets like dialog boxes & menus where we go and @@ -127,11 +128,20 @@ export default class ContextMenu extends React.PureComponent { this.initialFocus = document.activeElement as HTMLElement; } - componentWillUnmount() { + public componentDidMount() { + Modal.on(ModalManagerEvent.Opened, this.onModalOpen); + } + + public componentWillUnmount() { + Modal.off(ModalManagerEvent.Opened, this.onModalOpen); // return focus to the thing which had it before us this.initialFocus.focus(); } + private onModalOpen = () => { + this.props.onFinished?.(); + }; + private collectContextMenuRect = (element: HTMLDivElement) => { // We don't need to clean up when unmounting, so ignore if (!element) return; @@ -183,7 +193,7 @@ export default class ContextMenu extends React.PureComponent { private onFinished = (ev: React.MouseEvent) => { ev.stopPropagation(); ev.preventDefault(); - if (this.props.onFinished) this.props.onFinished(); + this.props.onFinished?.(); }; private onClick = (ev: React.MouseEvent) => { From de4e3f5a387255355bf6d1748f7525524ff61dae Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 9 Nov 2022 10:11:40 +0000 Subject: [PATCH 2/5] Add test --- .../views/context_menus/ContextMenu-test.tsx | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/components/views/context_menus/ContextMenu-test.tsx b/test/components/views/context_menus/ContextMenu-test.tsx index 70ee4b95f4d..e4abb5b5812 100644 --- a/test/components/views/context_menus/ContextMenu-test.tsx +++ b/test/components/views/context_menus/ContextMenu-test.tsx @@ -20,6 +20,8 @@ import { mount } from "enzyme"; import ContextMenu, { ChevronFace } from "../../../../src/components/structures/ContextMenu"; import UIStore from "../../../../src/stores/UIStore"; +import Modal from "../../../../src/Modal"; +import BaseDialog from "../../../../src/components/views/dialogs/BaseDialog"; describe("", () => { // Hardcode window and menu dimensions @@ -141,4 +143,23 @@ describe("", () => { expect(actualChevronOffset).toEqual(targetChevronOffset + targetX - actualX); }); }); + + it("should automatically close when a modal is opened", () => { + const targetX = -50; + const onFinished = jest.fn(); + + mount( + , + ); + + expect(onFinished).not.toHaveBeenCalled(); + Modal.createDialog(BaseDialog); + expect(onFinished).toHaveBeenCalled(); + }); }); From 70cfb963deb72a1edb469b1a751d3bc4161a9a71 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 9 Nov 2022 10:47:15 +0000 Subject: [PATCH 3/5] Fix mock --- test/components/views/location/LocationShareMenu-test.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/components/views/location/LocationShareMenu-test.tsx b/test/components/views/location/LocationShareMenu-test.tsx index 6609af93f3a..f590bcdd7c7 100644 --- a/test/components/views/location/LocationShareMenu-test.tsx +++ b/test/components/views/location/LocationShareMenu-test.tsx @@ -69,6 +69,9 @@ jest.mock('../../../../src/stores/OwnProfileStore', () => ({ jest.mock('../../../../src/Modal', () => ({ createDialog: jest.fn(), + on: jest.fn(), + off: jest.fn(), + ModalManagerEvent: { Opened: "opened" }, })); describe('', () => { From 262a615594213946d038982b044075384bc55d83 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 9 Nov 2022 14:50:32 +0000 Subject: [PATCH 4/5] DRY --- src/Modal.tsx | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/Modal.tsx b/src/Modal.tsx index 9aafd4ef8aa..ee24b15d54d 100644 --- a/src/Modal.tsx +++ b/src/Modal.tsx @@ -265,12 +265,8 @@ export class ModalManager extends TypedEventEmitter): void { + if (beforeModal !== this.getCurrentModal()) { + this.emit(ModalManagerEvent.Opened); + } + } + private onBackgroundClick = () => { const modal = this.getCurrentModal(); if (!modal) { From 0c32cd14f69fe486f79c862200e644af3280418c Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 9 Nov 2022 15:13:52 +0000 Subject: [PATCH 5/5] Increase coverage --- .../views/context_menus/ContextMenu-test.tsx | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/components/views/context_menus/ContextMenu-test.tsx b/test/components/views/context_menus/ContextMenu-test.tsx index e4abb5b5812..3344dee48a8 100644 --- a/test/components/views/context_menus/ContextMenu-test.tsx +++ b/test/components/views/context_menus/ContextMenu-test.tsx @@ -162,4 +162,26 @@ describe("", () => { Modal.createDialog(BaseDialog); expect(onFinished).toHaveBeenCalled(); }); + + it("should not automatically close when a modal is opened under the existing one", () => { + const targetX = -50; + const onFinished = jest.fn(); + + Modal.createDialog(BaseDialog); + mount( + , + ); + + expect(onFinished).not.toHaveBeenCalled(); + Modal.createDialog(BaseDialog, {}, "", false, true); + expect(onFinished).not.toHaveBeenCalled(); + Modal.appendDialog(BaseDialog); + expect(onFinished).not.toHaveBeenCalled(); + }); });