From e8fc0ddb72135bf780d47d4de9e14bb9f7a8d34e Mon Sep 17 00:00:00 2001 From: David Langley Date: Thu, 7 Aug 2025 06:51:26 +0100 Subject: [PATCH] Fix stickyRow/scrollIntoView when switiching space or changing filters - Also remove the rest of space/enter keyboard handling use --- package.json | 2 +- src/components/utils/ListView.tsx | 18 ++++++--- .../memberlist/MemberListViewModel.tsx | 12 ------ .../views/rooms/MemberList/MemberListView.tsx | 2 +- .../views/rooms/RoomListPanel/RoomList.tsx | 37 ++++++++++++++++--- .../rooms/RoomListPanel/RoomListItemView.tsx | 6 +++ src/hooks/useCombinedRef.ts | 23 ++++++++++++ .../RoomListPanel/RoomListItemView-test.tsx | 35 +++++++++++++----- .../views/rooms/memberlist/common.tsx | 2 +- .../components/views/utils/ListView-test.tsx | 2 +- yarn.lock | 7 ++-- 11 files changed, 107 insertions(+), 39 deletions(-) create mode 100644 src/hooks/useCombinedRef.ts diff --git a/package.json b/package.json index 675120f452..25763179fa 100644 --- a/package.json +++ b/package.json @@ -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", diff --git a/src/components/utils/ListView.tsx b/src/components/utils/ListView.tsx index 051d0a25e8..02cdffd5d1 100644 --- a/src/components/utils/ListView.tsx +++ b/src/components/utils/ListView.tsx @@ -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 * @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 * @template Item - The type of data items in the list * @template Context - The type of additional context data passed to items */ -export function ListView(props: IListViewProps): React.ReactElement { +export const ListView = React.forwardRef>(function ListView< + Item, + Context = any, +>(props: IListViewProps, ref: React.Ref): 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(null); + const internalRef = useRef(null); /** Reference to the DOM element containing the virtualized list */ const virtuosoDomRef = useRef(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( props.items[0] ? getItemKey(props.items[0]) : undefined, @@ -125,7 +133,7 @@ export function ListView(props: IListViewProps(props: IListViewProps(props: IListViewProps ); -} +}); diff --git a/src/components/viewmodels/memberlist/MemberListViewModel.tsx b/src/components/viewmodels/memberlist/MemberListViewModel.tsx index 55220d29a9..a03f703511 100644 --- a/src/components/viewmodels/memberlist/MemberListViewModel.tsx +++ b/src/components/viewmodels/memberlist/MemberListViewModel.tsx @@ -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, }; diff --git a/src/components/views/rooms/MemberList/MemberListView.tsx b/src/components/views/rooms/MemberList/MemberListView.tsx index f92f571788..198c1920ca 100644 --- a/src/components/views/rooms/MemberList/MemberListView.tsx +++ b/src/components/views/rooms/MemberList/MemberListView.tsx @@ -28,7 +28,7 @@ interface IProps { const MemberListView: React.FC = (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) { diff --git a/src/components/views/rooms/RoomListPanel/RoomList.tsx b/src/components/views/rooms/RoomListPanel/RoomList.tsx index 3edb4ea667..dad2bc5f75 100644 --- a/src/components/views/rooms/RoomListPanel/RoomList.tsx +++ b/src/components/views/rooms/RoomListPanel/RoomList.tsx @@ -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(null); + const lastRoomsCount = useRef(undefined); + const lastActiveIndex = useRef(undefined); + const [activeIndexInternal, setActiveIndexInternal] = useState(undefined); const getItemComponent = useCallback( - (index: number, item: Room, context: ListContext): JSX.Element => { + (index: number, item: Room, context: ListContext, 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 ( ); }, - [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 ( * 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(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} diff --git a/src/hooks/useCombinedRef.ts b/src/hooks/useCombinedRef.ts new file mode 100644 index 0000000000..a6f2574ad0 --- /dev/null +++ b/src/hooks/useCombinedRef.ts @@ -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(...refs: (React.Ref | null)[]) { + return useCallback((element: T | null) => { + refs.forEach((ref) => { + if (!ref) return; + if (typeof ref === "function") ref(element); + else (ref as React.MutableRefObject).current = element; + }); + }, refs); +} \ No newline at end of file diff --git a/test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx b/test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx index a97e5217ef..ff52b7cf30 100644 --- a/test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx +++ b/test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx @@ -60,7 +60,9 @@ describe("", () => { test("should render a room item", () => { const onClick = jest.fn(); - const { asFragment } = render(); + const { asFragment } = render( + , + ); expect(asFragment()).toMatchSnapshot(); }); @@ -68,13 +70,15 @@ describe("", () => { defaultValue.messagePreview = "The message looks like this"; const onClick = jest.fn(); - const { asFragment } = render(); + const { asFragment } = render( + , + ); expect(asFragment()).toMatchSnapshot(); }); test("should call openRoom when clicked", async () => { const user = userEvent.setup(); - render(); + render(); await user.click(screen.getByRole("button", { name: `Open room ${room.name}` })); expect(defaultValue.openRoom).toHaveBeenCalled(); @@ -84,7 +88,10 @@ describe("", () => { mocked(useRoomListItemViewModel).mockReturnValue({ ...defaultValue, showHoverMenu: true }); const user = userEvent.setup(); - render(, withClientContextRenderOptions(matrixClient)); + render( + , + 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("", () => { test("should hover decoration if focused", async () => { const user = userEvent.setup(); - render(, withClientContextRenderOptions(matrixClient)); + render( + , + 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("", () => { }); test("should be selected if isSelected=true", async () => { - const { asFragment } = render(); + const { asFragment } = render( + , + ); expect(screen.queryByRole("button", { name: `Open room ${room.name}` })).toHaveAttribute( "aria-selected", "true", @@ -118,7 +130,9 @@ describe("", () => { showNotificationDecoration: true, }); - const { asFragment } = render(); + const { asFragment } = render( + , + ); expect(screen.getByTestId("notification-decoration")).toBeInTheDocument(); expect(asFragment()).toMatchSnapshot(); }); @@ -131,7 +145,7 @@ describe("", () => { showNotificationDecoration: true, }); - render(); + render(); const listItem = screen.getByRole("button", { name: `Open room ${room.name}` }); await user.hover(listItem); @@ -146,7 +160,10 @@ describe("", () => { showContextMenu: true, }); - render(, withClientContextRenderOptions(matrixClient)); + render( + , + 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()); diff --git a/test/unit-tests/components/views/rooms/memberlist/common.tsx b/test/unit-tests/components/views/rooms/memberlist/common.tsx index becdfa46f3..592155841f 100644 --- a/test/unit-tests/components/views/rooms/memberlist/common.tsx +++ b/test/unit-tests/components/views/rooms/memberlist/common.tsx @@ -125,7 +125,7 @@ export async function renderMemberList( { wrapper: ({ children }) => ( - {children} + <>{children} ), }, diff --git a/test/unit-tests/components/views/utils/ListView-test.tsx b/test/unit-tests/components/views/utils/ListView-test.tsx index ea3243f097..5c58bdde4c 100644 --- a/test/unit-tests/components/views/utils/ListView-test.tsx +++ b/test/unit-tests/components/views/utils/ListView-test.tsx @@ -48,7 +48,7 @@ describe("ListView", () => { return render(getListViewComponent(mergedProps), { wrapper: ({ children }) => ( - {children} + <>{children} ), }); diff --git a/yarn.lock b/yarn.lock index b4fe445ee6..cd2a2de015 100644 --- a/yarn.lock +++ b/yarn.lock @@ -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"