Fix stickyRow/scrollIntoView when switiching space or changing filters

- Also remove the rest of space/enter keyboard handling use
This commit is contained in:
David Langley 2025-08-07 06:51:26 +01:00
parent c61cb8093a
commit e8fc0ddb72
11 changed files with 107 additions and 39 deletions

View File

@ -154,7 +154,7 @@
"react-string-replace": "^1.1.1",
"react-transition-group": "^4.4.1",
"react-virtualized": "^9.22.5",
"react-virtuoso": "^4.12.6",
"react-virtuoso": "https://gitpkg.vercel.app/langleyd/react-virtuoso/packages/react-virtuoso?36f7b675163853fc08ddc2704de90e59baa6f3fa&scripts.postinstall=npm%20install%20--ignore-scripts%20%26%26%20npm%20run%20build",
"rfc4648": "^1.4.0",
"sanitize-filename": "^1.6.3",
"sanitize-html": "2.17.0",

View File

@ -7,6 +7,7 @@ Please see LICENSE files in the repository root for full details.
import React, { useRef, type JSX, useCallback, useEffect, useState } from "react";
import { type VirtuosoHandle, type ListRange, Virtuoso, type VirtuosoProps } from "react-virtuoso";
import { useCombinedRef } from "../../hooks/useCombinedRef";
/**
* Context object passed to each list item containing the currently focused key
@ -34,6 +35,7 @@ export interface IListViewProps<Item, Context>
* @param index - The index of the item in the list
* @param item - The data item to render
* @param context - The context object containing the focused key and any additional data
* @param onFocus - A callback that is required to be called when the item component receives focus
* @returns JSX element representing the rendered item
*/
getItemComponent: (
@ -71,13 +73,19 @@ export interface IListViewProps<Item, Context>
* @template Item - The type of data items in the list
* @template Context - The type of additional context data passed to items
*/
export function ListView<Item, Context = any>(props: IListViewProps<Item, Context>): React.ReactElement {
export const ListView = React.forwardRef<VirtuosoHandle, IListViewProps<any, any>>(function ListView<
Item,
Context = any,
>(props: IListViewProps<Item, Context>, ref: React.Ref<VirtuosoHandle>): React.ReactElement {
// Extract our custom props to avoid conflicts with Virtuoso props
const { items, getItemComponent, isItemFocusable, getItemKey, context, ...virtuosoProps } = props;
/** Reference to the Virtuoso component for programmatic scrolling */
const virtuosoHandleRef = useRef<VirtuosoHandle>(null);
const internalRef = useRef<VirtuosoHandle>(null);
/** Reference to the DOM element containing the virtualized list */
const virtuosoDomRef = useRef<HTMLElement | Window>(null);
// Combined ref callback to handle both internal use and forwarding
const combinedRef = useCombinedRef(internalRef, ref);
/** Key of the item that should have tabIndex == 0 */
const [tabIndexKey, setTabIndexKey] = useState<string | undefined>(
props.items[0] ? getItemKey(props.items[0]) : undefined,
@ -125,7 +133,7 @@ export function ListView<Item, Context = any>(props: IListViewProps<Item, Contex
const key = getItemKey(items[clampedIndex]);
setTabIndexKey(key);
isScrollingToItem.current = true;
virtuosoHandleRef?.current?.scrollIntoView({
internalRef.current?.scrollIntoView({
index: clampedIndex,
align: align,
behavior: "auto",
@ -269,7 +277,7 @@ export function ListView<Item, Context = any>(props: IListViewProps<Item, Contex
<Virtuoso
tabIndex={props.tabIndex || undefined} // We don't need to focus the container, so leave it undefined by default
scrollerRef={scrollerRef}
ref={virtuosoHandleRef}
ref={combinedRef}
onKeyDown={keyDownCallback}
context={listContext}
rangeChanged={setVisibleRange}
@ -282,4 +290,4 @@ export function ListView<Item, Context = any>(props: IListViewProps<Item, Contex
{...virtuosoProps}
/>
);
}
});

View File

@ -38,8 +38,6 @@ import { isValid3pidInvite } from "../../../RoomInvite";
import { type ThreePIDInvite } from "../../../models/rooms/ThreePIDInvite";
import { type XOR } from "../../../@types/common";
import { useTypedEventEmitter } from "../../../hooks/useEventEmitter";
import { Action } from "../../../dispatcher/actions";
import dis from "../../../dispatcher/dispatcher";
type Member = XOR<{ member: RoomMember }, { threePidInvite: ThreePIDInvite }>;
@ -113,7 +111,6 @@ export interface MemberListViewState {
shouldShowSearch: boolean;
isLoading: boolean;
canInvite: boolean;
onClickMember: (member: RoomMember | ThreePIDInvite) => void;
onInviteButtonClick: (ev: ButtonEvent) => void;
}
export function useMemberListViewModel(roomId: string): MemberListViewState {
@ -136,14 +133,6 @@ export function useMemberListViewModel(roomId: string): MemberListViewState {
*/
const [memberCount, setMemberCount] = useState(0);
const onClickMember = (member: RoomMember | ThreePIDInvite): void => {
dis.dispatch({
action: Action.ViewUser,
member: member,
push: true,
});
};
const loadMembers = useMemo(
() =>
throttle(
@ -278,7 +267,6 @@ export function useMemberListViewModel(roomId: string): MemberListViewState {
isPresenceEnabled,
isLoading,
onInviteButtonClick,
onClickMember,
shouldShowSearch: totalMemberCount >= 20,
canInvite,
};

View File

@ -28,7 +28,7 @@ interface IProps {
const MemberListView: React.FC<IProps> = (props: IProps) => {
const vm = useMemberListViewModel(props.roomId);
const { isPresenceEnabled, onClickMember, memberCount } = vm;
const { isPresenceEnabled, memberCount } = vm;
const getItemKey = useCallback((item: MemberWithSeparator): string => {
if (item === SEPARATOR) {

View File

@ -5,13 +5,14 @@
* Please see LICENSE files in the repository root for full details.
*/
import React, { useCallback, type JSX } from "react";
import React, { useCallback, useEffect, useRef, useState, type JSX } from "react";
import { type RoomListViewState } from "../../../viewmodels/roomlist/RoomListViewModel";
import { _t } from "../../../../languageHandler";
import { RoomListItemView } from "./RoomListItemView";
import { ListContext, ListView } from "../../../utils/ListView";
import { Room } from "matrix-js-sdk/src/matrix";
import { VirtuosoHandle } from "react-virtuoso";
interface RoomListProps {
/**
@ -24,12 +25,16 @@ interface RoomListProps {
* A virtualized list of rooms.
*/
export function RoomList({ vm: { rooms, activeIndex } }: RoomListProps): JSX.Element {
const virtuosoRef = useRef<VirtuosoHandle>(null);
const lastRoomsCount = useRef<number | undefined>(undefined);
const lastActiveIndex = useRef<number | undefined>(undefined);
const [activeIndexInternal, setActiveIndexInternal] = useState<number | undefined>(undefined);
const getItemComponent = useCallback(
(index: number, item: Room, context: ListContext<any>): JSX.Element => {
(index: number, item: Room, context: ListContext<any>, onFocus: (e: React.FocusEvent) => void): JSX.Element => {
const itemKey = item.roomId;
const isRovingItem = itemKey === context.tabIndexKey;
const isFocused = isRovingItem && context.focused;
const isSelected = activeIndex === index;
const isSelected = activeIndexInternal === index;
return (
<RoomListItemView
room={item}
@ -37,19 +42,41 @@ export function RoomList({ vm: { rooms, activeIndex } }: RoomListProps): JSX.Ele
isSelected={isSelected}
isFocused={isFocused}
tabIndex={isRovingItem ? 0 : -1}
onFocus={onFocus}
/>
);
},
[rooms, activeIndex],
[activeIndexInternal],
);
const getItemKey = useCallback((item: Room): string => {
return item.roomId;
}, []);
// The first div is needed to make the virtualized list take all the remaining space and scroll correctly
useEffect(() => {
// When the rooms length or active index changes(from changing the space, filter, etc), scroll to the active room.
if (
activeIndex !== undefined &&
rooms.length != undefined &&
virtuosoRef.current &&
(lastRoomsCount.current !== rooms.length || lastActiveIndex.current !== activeIndex)
) {
virtuosoRef.current.scrollIntoView({
align: `start`,
index: activeIndex,
behavior: "auto",
targetsNextRefresh: true,
});
}
lastRoomsCount.current = rooms.length;
lastActiveIndex.current = activeIndex;
// targetsNextRefresh affects the next update so we store activeIndex in state here to force a re-render
setActiveIndexInternal(activeIndex);
}, [activeIndex, rooms.length]); // Dependencies that should trigger the scroll
console.log("!! rooms.length", rooms.length, "activeIndex", activeIndex, "virtuosoRef", virtuosoRef.current);
return (
<ListView
ref={virtuosoRef}
data-testid="room-list"
role="grid"
aria-label={_t("room_list|list_title")}

View File

@ -29,6 +29,10 @@ interface RoomListItemViewProps extends React.HTMLAttributes<HTMLButtonElement>
* Whether the room is focused
*/
isFocused: boolean;
/**
* A callback that indicates the item has received focus
*/
onFocus: (e: React.FocusEvent) => void;
}
/**
@ -38,6 +42,7 @@ export const RoomListItemView = memo(function RoomListItemView({
room,
isSelected,
isFocused,
onFocus,
...props
}: RoomListItemViewProps): JSX.Element {
const ref = useRef<HTMLButtonElement>(null);
@ -78,6 +83,7 @@ export const RoomListItemView = memo(function RoomListItemView({
aria-selected={isSelected}
aria-label={vm.a11yLabel}
onClick={() => vm.openRoom()}
onFocus={onFocus}
onMouseOver={() => setHover(true)}
onMouseOut={() => setHover(false)}
tabIndex={isFocused ? 0 : -1}

View File

@ -0,0 +1,23 @@
/*
Copyright 2025 New Vector Ltd.
SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
Please see LICENSE files in the repository root for full details.
*/
import { useCallback } from "react";
/**
* Combines multiple refs into a single ref callback.
* @param refs - The refs to combine
* @returns A ref callback that sets all provided refs to the same element
*/
export function useCombinedRef<T>(...refs: (React.Ref<T> | null)[]) {
return useCallback((element: T | null) => {
refs.forEach((ref) => {
if (!ref) return;
if (typeof ref === "function") ref(element);
else (ref as React.MutableRefObject<T | null>).current = element;
});
}, refs);
}

View File

@ -60,7 +60,9 @@ describe("<RoomListItemView />", () => {
test("should render a room item", () => {
const onClick = jest.fn();
const { asFragment } = render(<RoomListItemView room={room} onClick={onClick} isSelected={false} />);
const { asFragment } = render(
<RoomListItemView room={room} onClick={onClick} isSelected={false} isFocused={false} onFocus={jest.fn()} />,
);
expect(asFragment()).toMatchSnapshot();
});
@ -68,13 +70,15 @@ describe("<RoomListItemView />", () => {
defaultValue.messagePreview = "The message looks like this";
const onClick = jest.fn();
const { asFragment } = render(<RoomListItemView room={room} onClick={onClick} isSelected={false} />);
const { asFragment } = render(
<RoomListItemView room={room} onClick={onClick} isSelected={false} isFocused={false} onFocus={jest.fn()} />,
);
expect(asFragment()).toMatchSnapshot();
});
test("should call openRoom when clicked", async () => {
const user = userEvent.setup();
render(<RoomListItemView room={room} isSelected={false} />);
render(<RoomListItemView room={room} isSelected={false} isFocused={false} onFocus={jest.fn()} />);
await user.click(screen.getByRole("button", { name: `Open room ${room.name}` }));
expect(defaultValue.openRoom).toHaveBeenCalled();
@ -84,7 +88,10 @@ describe("<RoomListItemView />", () => {
mocked(useRoomListItemViewModel).mockReturnValue({ ...defaultValue, showHoverMenu: true });
const user = userEvent.setup();
render(<RoomListItemView room={room} isSelected={false} />, withClientContextRenderOptions(matrixClient));
render(
<RoomListItemView room={room} isSelected={false} isFocused={false} onFocus={jest.fn()} />,
withClientContextRenderOptions(matrixClient),
);
const listItem = screen.getByRole("button", { name: `Open room ${room.name}` });
expect(screen.queryByRole("button", { name: "More Options" })).toBeNull();
@ -94,7 +101,10 @@ describe("<RoomListItemView />", () => {
test("should hover decoration if focused", async () => {
const user = userEvent.setup();
render(<RoomListItemView room={room} isSelected={false} />, withClientContextRenderOptions(matrixClient));
render(
<RoomListItemView room={room} isSelected={false} isFocused={false} onFocus={jest.fn()} />,
withClientContextRenderOptions(matrixClient),
);
const listItem = screen.getByRole("button", { name: `Open room ${room.name}` });
await user.click(listItem);
expect(listItem).toHaveClass("mx_RoomListItemView_hover");
@ -104,7 +114,9 @@ describe("<RoomListItemView />", () => {
});
test("should be selected if isSelected=true", async () => {
const { asFragment } = render(<RoomListItemView room={room} isSelected={true} />);
const { asFragment } = render(
<RoomListItemView room={room} isSelected={true} isFocused={false} onFocus={jest.fn()} />,
);
expect(screen.queryByRole("button", { name: `Open room ${room.name}` })).toHaveAttribute(
"aria-selected",
"true",
@ -118,7 +130,9 @@ describe("<RoomListItemView />", () => {
showNotificationDecoration: true,
});
const { asFragment } = render(<RoomListItemView room={room} isSelected={false} />);
const { asFragment } = render(
<RoomListItemView room={room} isSelected={false} isFocused={false} onFocus={jest.fn()} />,
);
expect(screen.getByTestId("notification-decoration")).toBeInTheDocument();
expect(asFragment()).toMatchSnapshot();
});
@ -131,7 +145,7 @@ describe("<RoomListItemView />", () => {
showNotificationDecoration: true,
});
render(<RoomListItemView room={room} isSelected={false} />);
render(<RoomListItemView room={room} isSelected={false} isFocused={false} onFocus={jest.fn()} />);
const listItem = screen.getByRole("button", { name: `Open room ${room.name}` });
await user.hover(listItem);
@ -146,7 +160,10 @@ describe("<RoomListItemView />", () => {
showContextMenu: true,
});
render(<RoomListItemView room={room} isSelected={false} />, withClientContextRenderOptions(matrixClient));
render(
<RoomListItemView room={room} isSelected={false} isFocused={false} onFocus={jest.fn()} />,
withClientContextRenderOptions(matrixClient),
);
const button = screen.getByRole("button", { name: `Open room ${room.name}` });
await user.pointer([{ target: button }, { keys: "[MouseRight]", target: button }]);
await waitFor(() => expect(screen.getByRole("menu")).toBeInTheDocument());

View File

@ -125,7 +125,7 @@ export async function renderMemberList(
{
wrapper: ({ children }) => (
<VirtuosoMockContext.Provider value={{ viewportHeight: 600, itemHeight: 56 }}>
{children}
<>{children}</>
</VirtuosoMockContext.Provider>
),
},

View File

@ -48,7 +48,7 @@ describe("ListView", () => {
return render(getListViewComponent(mergedProps), {
wrapper: ({ children }) => (
<VirtuosoMockContext.Provider value={{ viewportHeight: 400, itemHeight: 56 }}>
{children}
<>{children}</>
</VirtuosoMockContext.Provider>
),
});

View File

@ -13091,10 +13091,9 @@ react-virtualized@^9.22.5:
prop-types "^15.7.2"
react-lifecycles-compat "^3.0.4"
react-virtuoso@^4.12.6:
version "4.12.6"
resolved "https://registry.yarnpkg.com/react-virtuoso/-/react-virtuoso-4.12.6.tgz#20fe374d43cce3c9821e29f4cc4d050596d06d01"
integrity sha512-bfvS6aCL1ehXmq39KRiz/vxznGUbtA27I5I24TYCe1DhMf84O3aVNCIwrSjYQjkJGJGzY46ihdN8WkYlemuhMQ==
"react-virtuoso@https://gitpkg.vercel.app/langleyd/react-virtuoso/packages/react-virtuoso?36f7b675163853fc08ddc2704de90e59baa6f3fa&scripts.postinstall=npm%20install%20--ignore-scripts%20%26%26%20npm%20run%20build":
version "4.13.0"
resolved "https://gitpkg.vercel.app/langleyd/react-virtuoso/packages/react-virtuoso?36f7b675163853fc08ddc2704de90e59baa6f3fa&scripts.postinstall=npm%20install%20--ignore-scripts%20%26%26%20npm%20run%20build#a979dd4bb7c14e30c8011d5c3db9301fc8466e84"
"react@^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", react@^19.0.0:
version "19.1.0"