Fix roving tab index getting confused after dragging space order (#10901

)

* Fix roving tab index getting confused after dragging space order

* Fix roving tab index for drag reordering

* delint

* Add test

* Make types happier

* Remove snapshot

src/accessibility/RovingTabIndex.tsx

@@ -78,16 +78,40 @@ export enum Type {
     Register = "REGISTER",
     Unregister = "UNREGISTER",
     SetFocus = "SET_FOCUS",
+    Update = "UPDATE",
 }
 
 export interface IAction {
-    type: Type;
+    type: Exclude<Type, Type.Update>;
     payload: {
         ref: Ref;
     };
 }
 
-export const reducer: Reducer<IState, IAction> = (state: IState, action: IAction) => {
+interface UpdateAction {
+    type: Type.Update;
+    payload?: undefined;
+}
+
+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<IState, Action> = (state: IState, action: Action) => {
     switch (action.type) {
         case Type.Register: {
             if (!state.activeRef) {
@@ -97,21 +121,7 @@ export const reducer: Reducer<IState, IAction> = (state: IState, action: IAction
 
             // 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 };
         }
@@ -150,6 +160,11 @@ export const reducer: Reducer<IState, IAction> = (state: IState, action: IAction
             return { ...state };
         }
 
+        case Type.Update: {
+            state.refs.sort(refSorter);
+            return { ...state };
+        }
+
         default:
             return state;
     }
@@ -160,7 +175,7 @@ interface IProps {
     handleHomeEnd?: boolean;
     handleUpDown?: boolean;
     handleLeftRight?: boolean;
-    children(renderProps: { onKeyDownHandler(ev: React.KeyboardEvent): void }): ReactNode;
+    children(renderProps: { onKeyDownHandler(ev: React.KeyboardEvent): void; onDragEndHandler(): void }): ReactNode;
     onKeyDown?(ev: React.KeyboardEvent, state: IState, dispatch: Dispatch<IAction>): void;
 }
 
@@ -199,7 +214,7 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
     handleLoop,
     onKeyDown,
 }) => {
-    const [state, dispatch] = useReducer<Reducer<IState, IAction>>(reducer, {
+    const [state, dispatch] = useReducer<Reducer<IState, Action>>(reducer, {
         refs: [],
     });
 
@@ -301,9 +316,15 @@ export const RovingTabIndexProvider: React.FC<IProps> = ({
         [context, onKeyDown, handleHomeEnd, handleUpDown, handleLeftRight, handleLoop],
     );
 
+    const onDragEndHandler = useCallback(() => {
+        dispatch({
+            type: Type.Update,
+        });
+    }, []);
+
     return (
         <RovingTabIndexContext.Provider value={context}>
-            {children({ onKeyDownHandler })}
+            {children({ onKeyDownHandler, onDragEndHandler })}
         </RovingTabIndexContext.Provider>
     );
 };

src/components/views/spaces/SpacePanel.tsx

@@ -330,6 +330,7 @@ const InnerSpacePanel = React.memo<IInnerSpacePanelProps>(
 );
 
 const SpacePanel: React.FC = () => {
+    const [dragging, setDragging] = useState(false);
     const [isPanelCollapsed, setPanelCollapsed] = useState(true);
     const ref = useRef<HTMLDivElement>(null);
     useLayoutEffect(() => {
@@ -344,14 +345,19 @@ const SpacePanel: React.FC = () => {
     });
 
     return (
-        <DragDropContext
-            onDragEnd={(result) => {
-                if (!result.destination) return; // dropped outside the list
-                SpaceStore.instance.moveRootSpace(result.source.index, result.destination.index);
-            }}
-        >
-            <RovingTabIndexProvider handleHomeEnd handleUpDown>
-                {({ onKeyDownHandler }) => (
+        <RovingTabIndexProvider handleHomeEnd handleUpDown={!dragging}>
+            {({ onKeyDownHandler, onDragEndHandler }) => (
+                <DragDropContext
+                    onDragStart={() => {
+                        setDragging(true);
+                    }}
+                    onDragEnd={(result) => {
+                        setDragging(false);
+                        if (!result.destination) return; // dropped outside the list
+                        SpaceStore.instance.moveRootSpace(result.source.index, result.destination.index);
+                        onDragEndHandler();
+                    }}
+                >
                     <div
                         className={classNames("mx_SpacePanel", { collapsed: isPanelCollapsed })}
                         onKeyDown={onKeyDownHandler}
@@ -395,9 +401,9 @@ const SpacePanel: React.FC = () => {
 
                         <QuickSettingsButton isPanelCollapsed={isPanelCollapsed} />
                     </div>
-                )}
-            </RovingTabIndexProvider>
-        </DragDropContext>
+                </DragDropContext>
+            )}
+        </RovingTabIndexProvider>
     );
 };
 

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("<SpacePanel />", () => {
     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,23 @@ describe("<SpacePanel />", () => {
             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 { getByLabelText } = render(<SpacePanel />);
+
+        const room1 = getByLabelText("Room 1");
+        await pickUp(room1);
+        await move(room1, DragDirection.DOWN);
+        await drop(room1);
+
+        expect(SpaceStore.instance.moveRootSpace).toHaveBeenCalledWith(0, 1);
+    });
 });