From 2250f5e6a21db280a8f3928d91403547bcaeb847 Mon Sep 17 00:00:00 2001 From: David Langley Date: Fri, 1 Aug 2025 14:16:13 +0100 Subject: [PATCH] Fix: Clicking on an item in the member list causes it to scroll to the top rather than show the profile view (#30455) * Fix issue and add test * Fix MemberTileView * Add e2e test and comment --- playwright/e2e/right-panel/memberlist.spec.ts | 70 ++++++++++++++--- src/components/utils/ListView.tsx | 23 +++++- .../views/rooms/MemberList/MemberListView.tsx | 9 ++- .../MemberList/tiles/RoomMemberTileView.tsx | 2 + .../tiles/ThreePidInviteTileView.tsx | 2 + .../tiles/common/MemberTileView.tsx | 2 + .../rooms/memberlist/MemberTileView-test.tsx | 31 ++++++-- .../components/views/utils/ListView-test.tsx | 76 +++++++++++++++++++ 8 files changed, 195 insertions(+), 20 deletions(-) diff --git a/playwright/e2e/right-panel/memberlist.spec.ts b/playwright/e2e/right-panel/memberlist.spec.ts index cd22626575..71a4a3cada 100644 --- a/playwright/e2e/right-panel/memberlist.spec.ts +++ b/playwright/e2e/right-panel/memberlist.spec.ts @@ -11,6 +11,32 @@ import { Bot } from "../../pages/bot"; const ROOM_NAME = "Test room"; const NAME = "Alice"; +async function setupRoomWithMembers( + app: any, + page: any, + homeserver: any, + roomName: string, + memberNames: string[], +): Promise { + const visibility = await page.evaluate(() => (window as any).matrixcs.Visibility.Public); + const id = await app.client.createRoom({ name: roomName, visibility }); + const bots: Bot[] = []; + + for (let i = 0; i < memberNames.length; i++) { + const displayName = memberNames[i]; + const bot = new Bot(page, homeserver, { displayName, startClient: false, autoAcceptInvites: false }); + if (displayName === "Susan") { + await bot.prepareClient(); + await app.client.inviteUser(id, bot.credentials?.userId); + } else { + await bot.joinRoom(id); + } + bots.push(bot); + } + + return id; +} + test.use({ synapseConfig: { presence: { @@ -25,17 +51,8 @@ test.use({ test.describe("Memberlist", () => { test.beforeEach(async ({ app, user, page, homeserver }, testInfo) => { testInfo.setTimeout(testInfo.timeout + 30_000); - const id = await app.client.createRoom({ name: ROOM_NAME }); - const newBots: Bot[] = []; const names = ["Bob", "Bob", "Susan"]; - for (let i = 0; i < 3; i++) { - const displayName = names[i]; - const autoAcceptInvites = displayName !== "Susan"; - const bot = new Bot(page, homeserver, { displayName, startClient: true, autoAcceptInvites }); - await bot.prepareClient(); - await app.client.inviteUser(id, bot.credentials?.userId); - newBots.push(bot); - } + await setupRoomWithMembers(app, page, homeserver, ROOM_NAME, names); }); test("Renders correctly", { tag: "@screenshot" }, async ({ page, app }) => { @@ -45,4 +62,37 @@ test.describe("Memberlist", () => { await expect(memberlist.getByText("Invited")).toHaveCount(1); await expect(page.locator(".mx_MemberListView")).toMatchScreenshot("with-four-members.png"); }); + + test("should handle scroll and click to view member profile", async ({ page, app, homeserver }) => { + // Create a room with many members to enable scrolling + const memberNames = Array.from({ length: 15 }, (_, i) => `Member${i.toString()}`); + await setupRoomWithMembers(app, page, homeserver, "Large Room", memberNames); + + // Navigate to the room and open member list + await app.viewRoomByName("Large Room"); + + const memberlist = await app.toggleMemberlistPanel(); + + // Get the scrollable container + const memberListContainer = memberlist.locator(".mx_AutoHideScrollbar"); + + // Scroll down to the bottom of the member list + await app.scrollListToBottom(memberListContainer); + + // Wait for the target member to be visible after scrolling + const targetName = "Member14"; + const targetMember = memberlist.locator(".mx_MemberTileView_name").filter({ hasText: targetName }); + await targetMember.waitFor({ state: "visible" }); + + // Verify Alice is not visible at this point + await expect(memberlist.locator(".mx_MemberTileView_name").filter({ hasText: "Alice" })).toHaveCount(0); + + // Click on a member near the bottom of the list + await expect(targetMember).toBeVisible(); + await targetMember.click(); + + // Verify that the user info screen is shown and hasn't scrolled back to top + await expect(page.locator(".mx_UserInfo")).toBeVisible(); + await expect(page.locator(".mx_UserInfo_profile").getByText(targetName)).toBeVisible(); + }); }); diff --git a/src/components/utils/ListView.tsx b/src/components/utils/ListView.tsx index 377ffb141b..585552e23a 100644 --- a/src/components/utils/ListView.tsx +++ b/src/components/utils/ListView.tsx @@ -42,7 +42,12 @@ export interface IListViewProps * @param context - The context object containing the focused key and any additional data * @returns JSX element representing the rendered item */ - getItemComponent: (index: number, item: Item, context: ListContext) => JSX.Element; + getItemComponent: ( + index: number, + item: Item, + context: ListContext, + onFocus: (e: React.FocusEvent) => void, + ) => JSX.Element; /** * Optional additional context data to pass to each rendered item. @@ -217,6 +222,20 @@ export function ListView(props: IListViewProps): JSX.Element => { + const onFocus = (e: React.FocusEvent): void => { + // If one of the item components has been focused directly, set the focused and tabIndex state + // and stop propagation so the ListViews onFocus doesn't also handle it. + const key = getItemKey(item); + setIsFocused(true); + setTabIndexKey(key); + e.stopPropagation(); + }; + return getItemComponent(index, item, context, onFocus); + }, + [getItemComponent, getItemKey], + ); /** * Handles focus events on the list. * Sets the focused state and scrolls to the focused item if it is not currently visible. @@ -265,7 +284,7 @@ export function ListView(props: IListViewProps ); diff --git a/src/components/views/rooms/MemberList/MemberListView.tsx b/src/components/views/rooms/MemberList/MemberListView.tsx index 8afdeaf990..fec82a68f2 100644 --- a/src/components/views/rooms/MemberList/MemberListView.tsx +++ b/src/components/views/rooms/MemberList/MemberListView.tsx @@ -41,7 +41,12 @@ const MemberListView: React.FC = (props: IProps) => { }, []); const getItemComponent = useCallback( - (index: number, item: MemberWithSeparator, context: ListContext): JSX.Element => { + ( + index: number, + item: MemberWithSeparator, + context: ListContext, + onFocus: (e: React.FocusEvent) => void, + ): JSX.Element => { const itemKey = getItemKey(item); const isRovingItem = itemKey === context.tabIndexKey; const focused = isRovingItem && context.focused; @@ -56,6 +61,7 @@ const MemberListView: React.FC = (props: IProps) => { tabIndex={isRovingItem ? 0 : -1} index={index} memberCount={memberCount} + onFocus={onFocus} /> ); } else { @@ -66,6 +72,7 @@ const MemberListView: React.FC = (props: IProps) => { tabIndex={isRovingItem ? 0 : -1} memberIndex={index - 1} // Adjust as invites are below the separator memberCount={memberCount} + onFocus={onFocus} /> ); } diff --git a/src/components/views/rooms/MemberList/tiles/RoomMemberTileView.tsx b/src/components/views/rooms/MemberList/tiles/RoomMemberTileView.tsx index 4837972da3..99a15893af 100644 --- a/src/components/views/rooms/MemberList/tiles/RoomMemberTileView.tsx +++ b/src/components/views/rooms/MemberList/tiles/RoomMemberTileView.tsx @@ -24,6 +24,7 @@ interface IProps { showPresence?: boolean; focused?: boolean; tabIndex?: number; + onFocus: (e: React.FocusEvent) => void; } export function RoomMemberTileView(props: IProps): JSX.Element { @@ -59,6 +60,7 @@ export function RoomMemberTileView(props: IProps): JSX.Element { return ( void; } export function ThreePidInviteTileView(props: Props): JSX.Element { @@ -39,6 +40,7 @@ export function ThreePidInviteTileView(props: Props): JSX.Element { iconJsx={iconJsx} focused={props.focused} tabIndex={props.tabIndex} + onFocus={props.onFocus} /> ); } diff --git a/src/components/views/rooms/MemberList/tiles/common/MemberTileView.tsx b/src/components/views/rooms/MemberList/tiles/common/MemberTileView.tsx index 3f97e2b48e..cb0cec74d9 100644 --- a/src/components/views/rooms/MemberList/tiles/common/MemberTileView.tsx +++ b/src/components/views/rooms/MemberList/tiles/common/MemberTileView.tsx @@ -13,6 +13,7 @@ interface Props { avatarJsx: JSX.Element; nameJsx: JSX.Element | string; onClick: () => void; + onFocus: (e: React.FocusEvent) => void; memberIndex: number; memberCount: number; ariaLabel?: string; @@ -41,6 +42,7 @@ export function MemberTileView(props: Props): JSX.Element { ref={ref} className="mx_MemberTileView" onClick={props.onClick} + onFocus={props.onFocus} aria-label={props?.ariaLabel} tabIndex={props.tabIndex} role="option" diff --git a/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx b/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx index cbcab38655..37feb44751 100644 --- a/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx +++ b/test/unit-tests/components/views/rooms/memberlist/MemberTileView-test.tsx @@ -35,7 +35,9 @@ describe("MemberTileView", () => { }); it("should not display an E2EIcon when the e2E status = normal", () => { - const { container } = render(); + const { container } = render( + , + ); const e2eIcon = container.querySelector(".mx_E2EIconView"); expect(e2eIcon).toBeNull(); expect(container).toMatchSnapshot(); @@ -47,7 +49,9 @@ describe("MemberTileView", () => { wasCrossSigningVerified: jest.fn().mockReturnValue(true), } as unknown as UserVerificationStatus); - const { container } = render(); + const { container } = render( + , + ); await waitFor(async () => { await userEvent.hover(container.querySelector(".mx_E2EIcon")!); expect(screen.getByText("This user has not verified all of their sessions.")).toBeInTheDocument(); @@ -68,7 +72,9 @@ describe("MemberTileView", () => { crossSigningVerified: true, } as DeviceVerificationStatus); - const { container } = render(); + const { container } = render( + , + ); await waitFor(async () => { await userEvent.hover(container.querySelector(".mx_E2EIcon")!); @@ -81,15 +87,21 @@ describe("MemberTileView", () => { it("renders user labels correctly", async () => { member.powerLevel = 50; - const { container: container1 } = render(); + const { container: container1 } = render( + , + ); expect(container1).toHaveTextContent("Moderator"); member.powerLevel = 100; - const { container: container2 } = render(); + const { container: container2 } = render( + , + ); expect(container2).toHaveTextContent("Admin"); member.isInvite = true; - const { container: container3 } = render(); + const { container: container3 } = render( + , + ); expect(container3).toHaveTextContent("Invited"); }); }); @@ -110,7 +122,12 @@ describe("MemberTileView", () => { it("renders ThreePidInvite correctly", async () => { const [{ threePidInvite }] = getPending3PidInvites(room); const { container } = render( - , + , ); expect(container).toMatchSnapshot(); }); diff --git a/test/unit-tests/components/views/utils/ListView-test.tsx b/test/unit-tests/components/views/utils/ListView-test.tsx index 8964fd2655..3aa3776c06 100644 --- a/test/unit-tests/components/views/utils/ListView-test.tsx +++ b/test/unit-tests/components/views/utils/ListView-test.tsx @@ -334,6 +334,82 @@ describe("ListView", () => { expect(container).toBeInTheDocument(); }); + + it("should not scroll to top when clicking an item after manual scroll", () => { + // Create a larger list to enable meaningful scrolling + const largerItems = Array.from({ length: 50 }, (_, i) => ({ + id: `item-${i}`, + name: `Item ${i}`, + })); + + const mockOnClick = jest.fn(); + + mockGetItemComponent.mockImplementation( + (index: number, item: TestItemWithSeparator, context: any, onFocus: (e: React.FocusEvent) => void) => { + const itemKey = typeof item === "string" ? item : item.id; + const isFocused = context.tabIndexKey === itemKey; + return ( +
mockOnClick(item)} + onFocus={onFocus} + > + {item === SEPARATOR_ITEM ? "---" : (item as TestItem).name} +
+ ); + }, + ); + + const { container } = renderListViewWithHeight({ items: largerItems }); + const listContainer = screen.getByRole("grid"); + + // Step 1: Focus the list initially (this sets tabIndexKey to first item: "item-0") + fireEvent.focus(listContainer); + + // Verify first item is focused initially and tabIndexKey is set to first item + let items = container.querySelectorAll(".mx_item"); + expect(items[0]).toHaveAttribute("tabindex", "0"); + expect(items[0]).toHaveAttribute("data-testid", "row-0"); + + // Step 2: Simulate manual scrolling (mouse wheel, scroll bar drag, etc.) + // This changes which items are visible but DOES NOT change tabIndexKey + // tabIndexKey should still point to "item-0" but "item-0" is no longer visible + fireEvent.scroll(listContainer, { target: { scrollTop: 300 } }); + + // Step 3: After scrolling, different items should now be visible + // but tabIndexKey should still point to "item-0" (which is no longer visible) + items = container.querySelectorAll(".mx_item"); + + // Verify that item-0 is no longer in the DOM (because it's scrolled out of view) + const item0 = container.querySelector("[data-testid='row-0']"); + expect(item0).toBeNull(); + + // Find a visible item to click on (should be items from further down the list) + const visibleItems = container.querySelectorAll(".mx_item"); + expect(visibleItems.length).toBeGreaterThan(0); + const clickTargetItem = visibleItems[0]; // Click on the first visible item + + // Click on the visible item + fireEvent.click(clickTargetItem); + + // The click should trigger the onFocus callback, which updates the tabIndexKey + // This simulates the real user interaction where clicking an item focuses it + fireEvent.focus(clickTargetItem); + + // Verify the click was handled + expect(mockOnClick).toHaveBeenCalled(); + + // With the fix applied: the clicked item should become focused (tabindex="0") + // This validates that the fix prevents unwanted scrolling back to the top + expect(clickTargetItem).toHaveAttribute("tabindex", "0"); + + // The key validation: ensure we haven't scrolled back to the top + // item-0 should still not be visible (if the fix is working) + const item0AfterClick = container.querySelector("[data-testid='row-0']"); + expect(item0AfterClick).toBeNull(); + }); }); describe("Accessibility", () => {