Focus the thread panel when clicking on an item in the TAC (#12410)

* Focus the thread panel when clicking on an item in the TAC actually the 'close' button in the threads panel as it's the only interactive element: we can improve this later when we use landmarks & generally have better a11y. * Undo minor refactoring as none of it is test3ed, it's not worth it. * add unit test * Add matrixchat tests * Needs awaits * ts-ignore * Fix test (I think...) * Remove unnecessary value set * Not how assignments work
matrix-org · Apr 10, 2024 · 59395ab · 59395ab
1 parent 0daf0cf
commit 59395ab
Show file tree

Hide file tree

Showing 12 changed files with 136 additions and 11 deletions.
diff --git a/playwright/e2e/spaces/threads-activity-centre/index.ts b/playwright/e2e/spaces/threads-activity-centre/index.ts
@@ -336,6 +336,15 @@ export class Helpers {
         return expect(this.page.locator(".mx_ThreadPanel")).toBeVisible();
     }
 
+    /**
+     * Assert that the thread panel is focused (actually the 'close' button, specifically)
+     */
+    assertThreadPanelFocused() {
+        return expect(
+            this.page.locator(".mx_ThreadPanel").locator(".mx_BaseCard_header").getByTitle("Close"),
+        ).toBeFocused();
+    }
+
     /**
      * Populate the rooms with messages and threads
      * @param room1

diff --git a/playwright/e2e/spaces/threads-activity-centre/threadsActivityCentre.spec.ts b/playwright/e2e/spaces/threads-activity-centre/threadsActivityCentre.spec.ts
@@ -160,4 +160,18 @@ test.describe("Threads Activity Centre", () => {
 
         await util.assertNoTacIndicator();
     });
+
+    test("should focus the thread panel close button when clicking an item in the TAC", async ({
+        room1,
+        room2,
+        util,
+        msg,
+    }) => {
+        await util.receiveMessages(room1, ["Msg1", msg.threadedOff("Msg1", "Resp1")]);
+
+        await util.openTac();
+        await util.clickRoomInTac(room1.name);
+
+        await util.assertThreadPanelFocused();
+    });
 });
diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx
@@ -116,7 +116,7 @@ import { ButtonEvent } from "../views/elements/AccessibleButton";
 import { ActionPayload } from "../../dispatcher/payloads";
 import { SummarizedNotificationState } from "../../stores/notifications/SummarizedNotificationState";
 import Views from "../../Views";
-import { ViewRoomPayload } from "../../dispatcher/payloads/ViewRoomPayload";
+import { FocusNextType, ViewRoomPayload } from "../../dispatcher/payloads/ViewRoomPayload";
 import { ViewHomePagePayload } from "../../dispatcher/payloads/ViewHomePagePayload";
 import { AfterLeaveRoomPayload } from "../../dispatcher/payloads/AfterLeaveRoomPayload";
 import { DoAfterSyncPreparedPayload } from "../../dispatcher/payloads/DoAfterSyncPreparedPayload";
@@ -229,7 +229,8 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
 
     private screenAfterLogin?: IScreen;
     private tokenLogin?: boolean;
-    private focusComposer: boolean;
+    // What to focus on next component update, if anything
+    private focusNext: FocusNextType;
     private subTitleStatus: string;
     private prevWindowWidth: number;
     private voiceBroadcastResumer?: VoiceBroadcastResumer;
@@ -298,8 +299,6 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
         this.themeWatcher.start();
         this.fontWatcher.start();
 
-        this.focusComposer = false;
-
         // object field used for tracking the status info appended to the title tag.
         // we don't do it as react state as i'm scared about triggering needless react refreshes.
         this.subTitleStatus = "";
@@ -483,9 +482,11 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
                 PosthogTrackers.instance.trackPageChange(this.state.view, this.state.page_type, durationMs);
             }
         }
-        if (this.focusComposer) {
+        if (this.focusNext === "composer") {
             dis.fire(Action.FocusSendMessageComposer);
-            this.focusComposer = false;
+            this.focusNext = undefined;
+        } else if (this.focusNext === "threadsPanel") {
+            dis.fire(Action.FocusThreadsPanel);
         }
     }
 
@@ -985,7 +986,7 @@ export default class MatrixChat extends React.PureComponent<IProps, IState> {
 
     // switch view to the given room
     private async viewRoom(roomInfo: ViewRoomPayload): Promise<void> {
-        this.focusComposer = true;
+        this.focusNext = roomInfo.focusNext ?? "composer";
 
         if (roomInfo.room_alias) {
             logger.log(`Switching to room alias ${roomInfo.room_alias} at event ${roomInfo.event_id}`);

diff --git a/src/components/structures/RoomView.tsx b/src/components/structures/RoomView.tsx
@@ -1268,7 +1268,7 @@ export class RoomView extends React.Component<IRoomProps, IRoomState> {
             case Action.FocusAComposer: {
                 dis.dispatch<FocusComposerPayload>({
                     ...(payload as FocusComposerPayload),
-                    // re-dispatch to the correct composer
+                    // re-dispatch to the correct composer (the send message will still be on screen even when editing a message)
                     action: this.state.editState ? Action.FocusEditMessageComposer : Action.FocusSendMessageComposer,
                 });
                 break;

diff --git a/src/components/structures/ThreadPanel.tsx b/src/components/structures/ThreadPanel.tsx
@@ -37,6 +37,9 @@ import { ButtonEvent } from "../views/elements/AccessibleButton";
 import Spinner from "../views/elements/Spinner";
 import Heading from "../views/typography/Heading";
 import { clearRoomNotification } from "../../utils/notifications";
+import { useDispatcher } from "../../hooks/useDispatcher";
+import dis from "../../dispatcher/dispatcher";
+import { Action } from "../../dispatcher/actions";
 
 interface IProps {
     roomId: string;
@@ -229,6 +232,7 @@ const ThreadPanel: React.FC<IProps> = ({ roomId, onClose, permalinkCreator }) =>
     const roomContext = useContext(RoomContext);
     const timelinePanel = useRef<TimelinePanel | null>(null);
     const card = useRef<HTMLDivElement | null>(null);
+    const closeButonRef = useRef<HTMLDivElement | null>(null);
 
     const [filterOption, setFilterOption] = useState<ThreadFilterType>(ThreadFilterType.All);
     const [room, setRoom] = useState<Room | null>(null);
@@ -255,6 +259,14 @@ const ThreadPanel: React.FC<IProps> = ({ roomId, onClose, permalinkCreator }) =>
         }
     }, [timelineSet, timelinePanel]);
 
+    useDispatcher(dis, (payload) => {
+        // This actually foucses the close button on the threads panel, as its the only interactive element,
+        // but at least it puts the user in the right area of the app.
+        if (payload.action === Action.FocusThreadsPanel) {
+            closeButonRef.current?.focus();
+        }
+    });
+
     return (
         <RoomContext.Provider
             value={{
@@ -276,6 +288,7 @@ const ThreadPanel: React.FC<IProps> = ({ roomId, onClose, permalinkCreator }) =>
                 onClose={onClose}
                 withoutScrollContainer={true}
                 ref={card}
+                closeButtonRef={closeButonRef}
             >
                 {card.current && <Measured sensor={card.current} onMeasurement={setNarrow} />}
                 {timelineSet ? (

diff --git a/src/components/views/right_panel/BaseCard.tsx b/src/components/views/right_panel/BaseCard.tsx
@@ -35,6 +35,8 @@ interface IProps {
     onKeyDown?(ev: KeyboardEvent): void;
     cardState?: any;
     ref?: Ref<HTMLDivElement>;
+    // Ref for the 'close' button the the card
+    closeButtonRef?: Ref<HTMLDivElement>;
     children: ReactNode;
 }
 
@@ -54,7 +56,21 @@ export const Group: React.FC<IGroupProps> = ({ className, title, children }) =>
 };
 
 const BaseCard: React.FC<IProps> = forwardRef<HTMLDivElement, IProps>(
-    ({ closeLabel, onClose, onBack, className, header, footer, withoutScrollContainer, children, onKeyDown }, ref) => {
+    (
+        {
+            closeLabel,
+            onClose,
+            onBack,
+            className,
+            header,
+            footer,
+            withoutScrollContainer,
+            children,
+            onKeyDown,
+            closeButtonRef,
+        },
+        ref,
+    ) => {
         let backButton;
         const cardHistory = RightPanelStore.instance.roomPhaseHistory;
         if (cardHistory.length > 1) {
@@ -75,6 +91,7 @@ const BaseCard: React.FC<IProps> = forwardRef<HTMLDivElement, IProps>(
                     className="mx_BaseCard_close"
                     onClick={onClose}
                     title={closeLabel || _t("action|close")}
+                    ref={closeButtonRef}
                 />
             );
         }

diff --git a/src/components/views/spaces/threads-activity-centre/ThreadsActivityCentre.tsx b/src/components/views/spaces/threads-activity-centre/ThreadsActivityCentre.tsx
@@ -159,6 +159,7 @@ function ThreadsActivityCentreRow({ room, onClick, notificationLevel }: ThreadsA
                     show_room_tile: true, // make sure the room is visible in the list
                     room_id: room.roomId,
                     metricsTrigger: "WebThreadsActivityCentre",
+                    focusNext: "threadsPanel",
                 });
             }}
             label={room.name}

diff --git a/src/components/views/spaces/threads-activity-centre/ThreadsActivityCentreButton.tsx b/src/components/views/spaces/threads-activity-centre/ThreadsActivityCentreButton.tsx
@@ -27,7 +27,7 @@ import { notificationLevelToIndicator } from "../../../../utils/notifications";
 
 interface ThreadsActivityCentreButtonProps extends ComponentProps<typeof IconButton> {
     /**
-     * Display the `Treads` label next to the icon.
+     * Display the `Threads` label next to the icon.
      */
     displayLabel?: boolean;
     /**

diff --git a/src/dispatcher/actions.ts b/src/dispatcher/actions.ts
@@ -91,6 +91,11 @@ export enum Action {
      */
     FocusAComposer = "focus_a_composer",
 
+    /**
+     * Focuses the threads panel.
+     */
+    FocusThreadsPanel = "focus_threads_panel",
+
     /**
      * Opens the user menu (previously known as the top left menu). No additional payload information required.
      */

diff --git a/src/dispatcher/payloads/ViewRoomPayload.ts b/src/dispatcher/payloads/ViewRoomPayload.ts
@@ -24,6 +24,8 @@ import { IOpts } from "../../createRoom";
 import { JoinRoomPayload } from "./JoinRoomPayload";
 import { AtLeastOne } from "../../@types/common";
 
+export type FocusNextType = "composer" | "threadsPanel" | undefined;
+
 /* eslint-disable camelcase */
 interface BaseViewRoomPayload extends Pick<ActionPayload, "action"> {
     action: Action.ViewRoom;
@@ -61,5 +63,6 @@ export type ViewRoomPayload = BaseViewRoomPayload &
         // the number of API calls required.
         room_id?: string;
         room_alias?: string;
+        focusNext: FocusNextType; // wat to focus after room switch. Defaults to 'composer' if undefined.
     }>;
 /* eslint-enable camelcase */
diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx
@@ -15,7 +15,7 @@ limitations under the License.
 */
 
 import React, { ComponentProps } from "react";
-import { fireEvent, render, RenderResult, screen, within } from "@testing-library/react";
+import { fireEvent, render, RenderResult, screen, waitFor, within } from "@testing-library/react";
 import fetchMock from "fetch-mock-jest";
 import { Mocked, mocked } from "jest-mock";
 import { ClientEvent, MatrixClient, MatrixEvent, Room, SyncState } from "matrix-js-sdk/src/matrix";
@@ -59,6 +59,7 @@ import { SSO_HOMESERVER_URL_KEY, SSO_ID_SERVER_URL_KEY } from "../../../src/Base
 import SettingsStore from "../../../src/settings/SettingsStore";
 import { SettingLevel } from "../../../src/settings/SettingLevel";
 import { MatrixClientPeg as peg } from "../../../src/MatrixClientPeg";
+import DMRoomMap from "../../../src/utils/DMRoomMap";
 
 jest.mock("matrix-js-sdk/src/oidc/authorize", () => ({
     completeAuthorizationCodeGrant: jest.fn(),
@@ -220,13 +221,19 @@ describe("<MatrixChat />", () => {
         jest.spyOn(StorageManager, "idbLoad").mockReset();
         jest.spyOn(StorageManager, "idbSave").mockResolvedValue(undefined);
         jest.spyOn(defaultDispatcher, "dispatch").mockClear();
+        jest.spyOn(defaultDispatcher, "fire").mockClear();
+
+        DMRoomMap.makeShared(mockClient);
 
         await clearAllModals();
     });
 
     resetJsDomAfterEach();
 
     afterEach(() => {
+        // @ts-ignore
+        DMRoomMap.setShared(null);
+
         jest.restoreAllMocks();
 
         // emit a loggedOut event so that all of the Store singletons forget about their references to the mock client
@@ -239,6 +246,22 @@ describe("<MatrixChat />", () => {
         expect(container).toMatchSnapshot();
     });
 
+    it("should fire to focus the message composer", async () => {
+        getComponent();
+        defaultDispatcher.dispatch({ action: Action.ViewRoom, room_id: "!room:server.org", focusNext: "composer" });
+        await waitFor(() => {
+            expect(defaultDispatcher.fire).toHaveBeenCalledWith(Action.FocusSendMessageComposer);
+        });
+    });
+
+    it("should fire to focus the threads panel", async () => {
+        getComponent();
+        defaultDispatcher.dispatch({ action: Action.ViewRoom, room_id: "!room:server.org", focusNext: "threadsPanel" });
+        await waitFor(() => {
+            expect(defaultDispatcher.fire).toHaveBeenCalledWith(Action.FocusThreadsPanel);
+        });
+    });
+
     describe("when query params have a OIDC params", () => {
         const issuer = "https://auth.com/";
         const homeserverUrl = "https://matrix.org";

diff --git a/test/components/structures/ThreadPanel-test.tsx b/test/components/structures/ThreadPanel-test.tsx
@@ -37,6 +37,8 @@ import ResizeNotifier from "../../../src/utils/ResizeNotifier";
 import { createTestClient, getRoomContext, mkRoom, mockPlatformPeg, stubClient } from "../../test-utils";
 import { mkThread } from "../../test-utils/threads";
 import { IRoomState } from "../../../src/components/structures/RoomView";
+import defaultDispatcher from "../../../src/dispatcher/dispatcher";
+import { Action } from "../../../src/dispatcher/actions";
 
 jest.mock("../../../src/utils/Feedback");
 
@@ -156,6 +158,43 @@ describe("ThreadPanel", () => {
             fireEvent.click(getByRole(container, "button", { name: "Mark all as read" }));
             await waitFor(() => expect(mockClient.sendReadReceipt).not.toHaveBeenCalled());
         });
+
+        it("focuses the close button on FocusThreadsPanel dispatch", () => {
+            const ROOM_ID = "!roomId:example.org";
+
+            stubClient();
+            mockPlatformPeg();
+            const mockClient = mocked(MatrixClientPeg.safeGet());
+
+            const room = new Room(ROOM_ID, mockClient, mockClient.getUserId() ?? "", {
+                pendingEventOrdering: PendingEventOrdering.Detached,
+            });
+
+            render(
+                <MatrixClientContext.Provider value={mockClient}>
+                    <RoomContext.Provider
+                        value={getRoomContext(room, {
+                            canSendMessages: true,
+                        })}
+                    >
+                        <ThreadPanel
+                            roomId={ROOM_ID}
+                            onClose={jest.fn()}
+                            resizeNotifier={new ResizeNotifier()}
+                            permalinkCreator={new RoomPermalinkCreator(room)}
+                        />
+                    </RoomContext.Provider>
+                </MatrixClientContext.Provider>,
+            );
+
+            // Unfocus it first so we know it's not just focused by coincidence
+            screen.getByTestId("base-card-close-button").blur();
+            expect(screen.getByTestId("base-card-close-button")).not.toHaveFocus();
+
+            defaultDispatcher.dispatch({ action: Action.FocusThreadsPanel }, true);
+
+            expect(screen.getByTestId("base-card-close-button")).toHaveFocus();
+        });
     });
 
     describe("Filtering", () => {