From 836272df3d8dcc2ef67659330f5a3ffa62db94d6 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 15 May 2023 12:58:25 +0100 Subject: [PATCH 1/6] Fix roving tab index getting confused after dragging space order --- src/accessibility/RovingTabIndex.tsx | 42 +++++++++++++++++++--- src/components/views/spaces/SpacePanel.tsx | 30 +++++++++------- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/src/accessibility/RovingTabIndex.tsx b/src/accessibility/RovingTabIndex.tsx index 1963459835d..a5b6025eef3 100644 --- a/src/accessibility/RovingTabIndex.tsx +++ b/src/accessibility/RovingTabIndex.tsx @@ -31,6 +31,7 @@ import React, { import { getKeyBindingsManager } from "../KeyBindingsManager"; import { KeyBindingAction } from "./KeyboardShortcuts"; import { FocusHandler, Ref } from "./roving/types"; +import { moveElement } from "../utils/arrays"; /** * Module to simplify implementing the Roving TabIndex accessibility technique @@ -78,16 +79,27 @@ export enum Type { Register = "REGISTER", Unregister = "UNREGISTER", SetFocus = "SET_FOCUS", + Move = "MOVE", } export interface IAction { - type: Type; + type: Exclude; payload: { ref: Ref; }; } -export const reducer: Reducer = (state: IState, action: IAction) => { +interface MoveAction { + type: Type.Move; + payload: { + sourceIndex: number; + destinationIndex: number; + }; +} + +type Action = IAction | MoveAction; + +export const reducer: Reducer = (state: IState, action: Action) => { switch (action.type) { case Type.Register: { if (!state.activeRef) { @@ -150,6 +162,16 @@ export const reducer: Reducer = (state: IState, action: IAction return { ...state }; } + case Type.Move: { + if (action.payload.sourceIndex === action.payload.destinationIndex) return state; + // Perform the swap + moveElement(state.refs, action.payload.sourceIndex, action.payload.destinationIndex); + // Some callers, e.g. react-beautiful-dnd do wacky things with swapping refs around + // so we have to ensure we update our activeRef to point at the right thing + state.activeRef = state.refs[action.payload.destinationIndex]; + return { ...state }; + } + default: return state; } @@ -160,7 +182,10 @@ interface IProps { handleHomeEnd?: boolean; handleUpDown?: boolean; handleLeftRight?: boolean; - children(renderProps: { onKeyDownHandler(ev: React.KeyboardEvent): void }): ReactNode; + children(renderProps: { + onKeyDownHandler(ev: React.KeyboardEvent): void; + onDragEndHandler(sourceIndex: number, destinationIndex: number): void; + }): ReactNode; onKeyDown?(ev: React.KeyboardEvent, state: IState, dispatch: Dispatch): void; } @@ -199,7 +224,7 @@ export const RovingTabIndexProvider: React.FC = ({ handleLoop, onKeyDown, }) => { - const [state, dispatch] = useReducer>(reducer, { + const [state, dispatch] = useReducer>(reducer, { refs: [], }); @@ -301,9 +326,16 @@ export const RovingTabIndexProvider: React.FC = ({ [context, onKeyDown, handleHomeEnd, handleUpDown, handleLeftRight, handleLoop], ); + const onDragEndHandler = useCallback((sourceIndex: number, destinationIndex: number) => { + dispatch({ + type: Type.Move, + payload: { sourceIndex, destinationIndex }, + }); + }, []); + return ( - {children({ onKeyDownHandler })} + {children({ onKeyDownHandler, onDragEndHandler })} ); }; diff --git a/src/components/views/spaces/SpacePanel.tsx b/src/components/views/spaces/SpacePanel.tsx index 8e95f29667e..935c8721810 100644 --- a/src/components/views/spaces/SpacePanel.tsx +++ b/src/components/views/spaces/SpacePanel.tsx @@ -309,7 +309,8 @@ const InnerSpacePanel = React.memo( ( ); const SpacePanel: React.FC = () => { + const [invites, metaSpaces] = useSpaces(); const [isPanelCollapsed, setPanelCollapsed] = useState(true); const ref = useRef(null); useLayoutEffect(() => { @@ -344,14 +346,18 @@ const SpacePanel: React.FC = () => { }); return ( - { - if (!result.destination) return; // dropped outside the list - SpaceStore.instance.moveRootSpace(result.source.index, result.destination.index); - }} - > - - {({ onKeyDownHandler }) => ( + + {({ onKeyDownHandler, onDragEndHandler }) => ( + { + if (!result.destination) return; // dropped outside the list + SpaceStore.instance.moveRootSpace(result.source.index, result.destination.index); + // The roving tab index tracks all space buttons in the panel + // but dnd only the draggable real spaces, so we need to match them up + const offset = invites.length + metaSpaces.length; + onDragEndHandler(result.source.index + offset, result.destination.index + offset); + }} + >
{
- )} - - + + )} + ); }; From 60767c030354d50aa0f7d35cf86a3e5f75134574 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 17 May 2023 11:15:11 +0100 Subject: [PATCH 2/6] Fix roving tab index for drag reordering --- src/accessibility/RovingTabIndex.tsx | 66 +++++++++------------- src/components/views/spaces/SpacePanel.tsx | 16 +++--- 2 files changed, 36 insertions(+), 46 deletions(-) diff --git a/src/accessibility/RovingTabIndex.tsx b/src/accessibility/RovingTabIndex.tsx index a5b6025eef3..2847031c3f4 100644 --- a/src/accessibility/RovingTabIndex.tsx +++ b/src/accessibility/RovingTabIndex.tsx @@ -79,25 +79,38 @@ export enum Type { Register = "REGISTER", Unregister = "UNREGISTER", SetFocus = "SET_FOCUS", - Move = "MOVE", + Update = "UPDATE", } export interface IAction { - type: Exclude; + type: Exclude; payload: { ref: Ref; }; } -interface MoveAction { - type: Type.Move; - payload: { - sourceIndex: number; - destinationIndex: number; - }; +interface UpdateAction { + type: Type.Update; + payload?: undefined; } -type Action = IAction | MoveAction; +type Action = IAction | UpdateAction; + +const refSorter = (a: Ref, b: Ref): number => { + if (a === b) { + return 0; + } + + const position = a.current!.compareDocumentPosition(b.current!); + + if (position & Node.DOCUMENT_POSITION_FOLLOWING || position & Node.DOCUMENT_POSITION_CONTAINED_BY) { + return -1; + } else if (position & Node.DOCUMENT_POSITION_PRECEDING || position & Node.DOCUMENT_POSITION_CONTAINS) { + return 1; + } else { + return 0; + } +}; export const reducer: Reducer = (state: IState, action: Action) => { switch (action.type) { @@ -109,21 +122,7 @@ export const reducer: Reducer = (state: IState, action: Action) // Sadly due to the potential of DOM elements swapping order we can't do anything fancy like a binary insert state.refs.push(action.payload.ref); - state.refs.sort((a, b) => { - if (a === b) { - return 0; - } - - const position = a.current!.compareDocumentPosition(b.current!); - - if (position & Node.DOCUMENT_POSITION_FOLLOWING || position & Node.DOCUMENT_POSITION_CONTAINED_BY) { - return -1; - } else if (position & Node.DOCUMENT_POSITION_PRECEDING || position & Node.DOCUMENT_POSITION_CONTAINS) { - return 1; - } else { - return 0; - } - }); + state.refs.sort(refSorter); return { ...state }; } @@ -162,13 +161,8 @@ export const reducer: Reducer = (state: IState, action: Action) return { ...state }; } - case Type.Move: { - if (action.payload.sourceIndex === action.payload.destinationIndex) return state; - // Perform the swap - moveElement(state.refs, action.payload.sourceIndex, action.payload.destinationIndex); - // Some callers, e.g. react-beautiful-dnd do wacky things with swapping refs around - // so we have to ensure we update our activeRef to point at the right thing - state.activeRef = state.refs[action.payload.destinationIndex]; + case Type.Update: { + state.refs.sort(refSorter); return { ...state }; } @@ -182,10 +176,7 @@ interface IProps { handleHomeEnd?: boolean; handleUpDown?: boolean; handleLeftRight?: boolean; - children(renderProps: { - onKeyDownHandler(ev: React.KeyboardEvent): void; - onDragEndHandler(sourceIndex: number, destinationIndex: number): void; - }): ReactNode; + children(renderProps: { onKeyDownHandler(ev: React.KeyboardEvent): void; onDragEndHandler(): void }): ReactNode; onKeyDown?(ev: React.KeyboardEvent, state: IState, dispatch: Dispatch): void; } @@ -326,10 +317,9 @@ export const RovingTabIndexProvider: React.FC = ({ [context, onKeyDown, handleHomeEnd, handleUpDown, handleLeftRight, handleLoop], ); - const onDragEndHandler = useCallback((sourceIndex: number, destinationIndex: number) => { + const onDragEndHandler = useCallback(() => { dispatch({ - type: Type.Move, - payload: { sourceIndex, destinationIndex }, + type: Type.Update, }); }, []); diff --git a/src/components/views/spaces/SpacePanel.tsx b/src/components/views/spaces/SpacePanel.tsx index 935c8721810..c3ca861fad6 100644 --- a/src/components/views/spaces/SpacePanel.tsx +++ b/src/components/views/spaces/SpacePanel.tsx @@ -309,8 +309,7 @@ const InnerSpacePanel = React.memo( ( ); const SpacePanel: React.FC = () => { - const [invites, metaSpaces] = useSpaces(); + const [dragging, setDragging] = useState(false); const [isPanelCollapsed, setPanelCollapsed] = useState(true); const ref = useRef(null); useLayoutEffect(() => { @@ -346,16 +345,17 @@ const SpacePanel: React.FC = () => { }); return ( - + {({ onKeyDownHandler, onDragEndHandler }) => ( { + setDragging(true); + }} onDragEnd={(result) => { + setDragging(false); if (!result.destination) return; // dropped outside the list SpaceStore.instance.moveRootSpace(result.source.index, result.destination.index); - // The roving tab index tracks all space buttons in the panel - // but dnd only the draggable real spaces, so we need to match them up - const offset = invites.length + metaSpaces.length; - onDragEndHandler(result.source.index + offset, result.destination.index + offset); + onDragEndHandler(); }} >
Date: Wed, 17 May 2023 11:18:11 +0100 Subject: [PATCH 3/6] delint --- src/accessibility/RovingTabIndex.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/accessibility/RovingTabIndex.tsx b/src/accessibility/RovingTabIndex.tsx index 2847031c3f4..5f3901a3916 100644 --- a/src/accessibility/RovingTabIndex.tsx +++ b/src/accessibility/RovingTabIndex.tsx @@ -31,7 +31,6 @@ import React, { import { getKeyBindingsManager } from "../KeyBindingsManager"; import { KeyBindingAction } from "./KeyboardShortcuts"; import { FocusHandler, Ref } from "./roving/types"; -import { moveElement } from "../utils/arrays"; /** * Module to simplify implementing the Roving TabIndex accessibility technique From 2463b61d51715fceab9a1be2410ff383620c0dac Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 17 May 2023 13:28:23 +0100 Subject: [PATCH 4/6] Add test --- .../views/spaces/SpacePanel-test.tsx | 95 ++++++- .../__snapshots__/SpacePanel-test.tsx.snap | 268 ++++++++++++++++++ 2 files changed, 361 insertions(+), 2 deletions(-) create mode 100644 test/components/views/spaces/__snapshots__/SpacePanel-test.tsx.snap diff --git a/test/components/views/spaces/SpacePanel-test.tsx b/test/components/views/spaces/SpacePanel-test.tsx index 8ed108ee398..d238f0c21b7 100644 --- a/test/components/views/spaces/SpacePanel-test.tsx +++ b/test/components/views/spaces/SpacePanel-test.tsx @@ -15,7 +15,7 @@ limitations under the License. */ import React from "react"; -import { render, screen, fireEvent } from "@testing-library/react"; +import { render, screen, fireEvent, act } from "@testing-library/react"; import { mocked } from "jest-mock"; import { MatrixClient } from "matrix-js-sdk/src/matrix"; @@ -24,8 +24,71 @@ import { MatrixClientPeg } from "../../../../src/MatrixClientPeg"; import { MetaSpace, SpaceKey } from "../../../../src/stores/spaces"; import { shouldShowComponent } from "../../../../src/customisations/helpers/UIComponents"; import { UIComponent } from "../../../../src/settings/UIFeature"; -import { wrapInSdkContext } from "../../../test-utils"; +import { mkStubRoom, wrapInSdkContext } from "../../../test-utils"; import { SdkContextClass } from "../../../../src/contexts/SDKContext"; +import SpaceStore from "../../../../src/stores/spaces/SpaceStore"; +import DMRoomMap from "../../../../src/utils/DMRoomMap"; + +// DND test utilities based on +// https://github.com/colinrobertbrooks/react-beautiful-dnd-test-utils/issues/18#issuecomment-1373388693 +enum Keys { + SPACE = 32, + ARROW_LEFT = 37, + ARROW_UP = 38, + ARROW_RIGHT = 39, + ARROW_DOWN = 40, +} + +enum DragDirection { + LEFT = Keys.ARROW_LEFT, + UP = Keys.ARROW_UP, + RIGHT = Keys.ARROW_RIGHT, + DOWN = Keys.ARROW_DOWN, +} + +// taken from https://github.com/hello-pangea/dnd/blob/main/test/unit/integration/util/controls.ts#L20 +const createTransitionEndEvent = (): Event => { + const event = new Event("transitionend", { + bubbles: true, + cancelable: true, + }) as TransitionEvent; + + // cheating and adding property to event as + // TransitionEvent constructor does not exist. + // This is needed because of the following check + // https://github.com/atlassian/react-beautiful-dnd/blob/master/src/view/draggable/draggable.jsx#L130 + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (event as any).propertyName = "transform"; + + return event; +}; + +const pickUp = async (element: HTMLElement) => { + fireEvent.keyDown(element, { + keyCode: Keys.SPACE, + }); + await screen.findByText(/You have lifted an item/i); + + act(() => { + jest.runOnlyPendingTimers(); + }); +}; + +const move = async (element: HTMLElement, direction: DragDirection) => { + fireEvent.keyDown(element, { + keyCode: direction, + }); + await screen.findByText(/(You have moved the item | has been combined with)/i); +}; + +const drop = async (element: HTMLElement) => { + fireEvent.keyDown(element, { + keyCode: Keys.SPACE, + }); + fireEvent(element.parentElement, createTransitionEndEvent()); + + await screen.findByText(/You have dropped the item/i); +}; jest.mock("../../../../src/stores/spaces/SpaceStore", () => { // eslint-disable-next-line @typescript-eslint/no-var-requires @@ -35,6 +98,10 @@ jest.mock("../../../../src/stores/spaces/SpaceStore", () => { enabledMetaSpaces: MetaSpace[] = []; spacePanelSpaces: string[] = []; activeSpace: SpaceKey = "!space1"; + getChildSpaces = () => []; + getNotificationState = () => null; + setActiveSpace = jest.fn(); + moveRootSpace = jest.fn(); } return { instance: new MockSpaceStore(), @@ -49,8 +116,12 @@ describe("", () => { const mockClient = { getUserId: jest.fn().mockReturnValue("@test:test"), getSafeUserId: jest.fn().mockReturnValue("@test:test"), + mxcUrlToHttp: jest.fn(), + getRoom: jest.fn(), isGuest: jest.fn(), getAccountData: jest.fn(), + on: jest.fn(), + removeListener: jest.fn(), } as unknown as MatrixClient; const SpacePanel = wrapInSdkContext(UnwrappedSpacePanel, SdkContextClass.instance); @@ -81,4 +152,24 @@ describe("", () => { screen.getByTestId("create-space-button"); }); }); + + it("should allow rearranging via drag and drop", async () => { + (SpaceStore.instance.spacePanelSpaces as any) = [ + mkStubRoom("!room1:server", "Room 1", mockClient), + mkStubRoom("!room2:server", "Room 2", mockClient), + mkStubRoom("!room3:server", "Room 3", mockClient), + ]; + DMRoomMap.makeShared(); + jest.useFakeTimers(); + + const { asFragment, getByLabelText } = render(); + expect(asFragment()).toMatchSnapshot(); + + const room1 = getByLabelText("Room 1"); + await pickUp(room1); + await move(room1, DragDirection.DOWN); + await drop(room1); + + expect(SpaceStore.instance.moveRootSpace).toHaveBeenCalledWith(0, 1); + }); }); diff --git a/test/components/views/spaces/__snapshots__/SpacePanel-test.tsx.snap b/test/components/views/spaces/__snapshots__/SpacePanel-test.tsx.snap new file mode 100644 index 00000000000..7d5621acef1 --- /dev/null +++ b/test/components/views/spaces/__snapshots__/SpacePanel-test.tsx.snap @@ -0,0 +1,268 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` should allow rearranging via drag and drop 1`] = ` + +