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
This commit is contained in:
@@ -11,6 +11,32 @@ import { Bot } from "../../pages/bot";
|
|||||||
const ROOM_NAME = "Test room";
|
const ROOM_NAME = "Test room";
|
||||||
const NAME = "Alice";
|
const NAME = "Alice";
|
||||||
|
|
||||||
|
async function setupRoomWithMembers(
|
||||||
|
app: any,
|
||||||
|
page: any,
|
||||||
|
homeserver: any,
|
||||||
|
roomName: string,
|
||||||
|
memberNames: string[],
|
||||||
|
): Promise<string> {
|
||||||
|
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({
|
test.use({
|
||||||
synapseConfig: {
|
synapseConfig: {
|
||||||
presence: {
|
presence: {
|
||||||
@@ -25,17 +51,8 @@ test.use({
|
|||||||
test.describe("Memberlist", () => {
|
test.describe("Memberlist", () => {
|
||||||
test.beforeEach(async ({ app, user, page, homeserver }, testInfo) => {
|
test.beforeEach(async ({ app, user, page, homeserver }, testInfo) => {
|
||||||
testInfo.setTimeout(testInfo.timeout + 30_000);
|
testInfo.setTimeout(testInfo.timeout + 30_000);
|
||||||
const id = await app.client.createRoom({ name: ROOM_NAME });
|
|
||||||
const newBots: Bot[] = [];
|
|
||||||
const names = ["Bob", "Bob", "Susan"];
|
const names = ["Bob", "Bob", "Susan"];
|
||||||
for (let i = 0; i < 3; i++) {
|
await setupRoomWithMembers(app, page, homeserver, ROOM_NAME, names);
|
||||||
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);
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
|
|
||||||
test("Renders correctly", { tag: "@screenshot" }, async ({ page, app }) => {
|
test("Renders correctly", { tag: "@screenshot" }, async ({ page, app }) => {
|
||||||
@@ -45,4 +62,37 @@ test.describe("Memberlist", () => {
|
|||||||
await expect(memberlist.getByText("Invited")).toHaveCount(1);
|
await expect(memberlist.getByText("Invited")).toHaveCount(1);
|
||||||
await expect(page.locator(".mx_MemberListView")).toMatchScreenshot("with-four-members.png");
|
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();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -42,7 +42,12 @@ export interface IListViewProps<Item, Context>
|
|||||||
* @param context - The context object containing the focused key and any additional data
|
* @param context - The context object containing the focused key and any additional data
|
||||||
* @returns JSX element representing the rendered item
|
* @returns JSX element representing the rendered item
|
||||||
*/
|
*/
|
||||||
getItemComponent: (index: number, item: Item, context: ListContext<Context>) => JSX.Element;
|
getItemComponent: (
|
||||||
|
index: number,
|
||||||
|
item: Item,
|
||||||
|
context: ListContext<Context>,
|
||||||
|
onFocus: (e: React.FocusEvent) => void,
|
||||||
|
) => JSX.Element;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Optional additional context data to pass to each rendered item.
|
* Optional additional context data to pass to each rendered item.
|
||||||
@@ -217,6 +222,20 @@ export function ListView<Item, Context = any>(props: IListViewProps<Item, Contex
|
|||||||
virtuosoDomRef.current = element;
|
virtuosoDomRef.current = element;
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
|
const getItemComponentInternal = useCallback(
|
||||||
|
(index: number, item: Item, context: ListContext<Context>): 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.
|
* Handles focus events on the list.
|
||||||
* Sets the focused state and scrolls to the focused item if it is not currently visible.
|
* Sets the focused state and scrolls to the focused item if it is not currently visible.
|
||||||
@@ -265,7 +284,7 @@ export function ListView<Item, Context = any>(props: IListViewProps<Item, Contex
|
|||||||
data={props.items}
|
data={props.items}
|
||||||
onFocus={onFocus}
|
onFocus={onFocus}
|
||||||
onBlur={onBlur}
|
onBlur={onBlur}
|
||||||
itemContent={props.getItemComponent}
|
itemContent={getItemComponentInternal}
|
||||||
{...virtuosoProps}
|
{...virtuosoProps}
|
||||||
/>
|
/>
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -41,7 +41,12 @@ const MemberListView: React.FC<IProps> = (props: IProps) => {
|
|||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
const getItemComponent = useCallback(
|
const getItemComponent = useCallback(
|
||||||
(index: number, item: MemberWithSeparator, context: ListContext<any>): JSX.Element => {
|
(
|
||||||
|
index: number,
|
||||||
|
item: MemberWithSeparator,
|
||||||
|
context: ListContext<any>,
|
||||||
|
onFocus: (e: React.FocusEvent) => void,
|
||||||
|
): JSX.Element => {
|
||||||
const itemKey = getItemKey(item);
|
const itemKey = getItemKey(item);
|
||||||
const isRovingItem = itemKey === context.tabIndexKey;
|
const isRovingItem = itemKey === context.tabIndexKey;
|
||||||
const focused = isRovingItem && context.focused;
|
const focused = isRovingItem && context.focused;
|
||||||
@@ -56,6 +61,7 @@ const MemberListView: React.FC<IProps> = (props: IProps) => {
|
|||||||
tabIndex={isRovingItem ? 0 : -1}
|
tabIndex={isRovingItem ? 0 : -1}
|
||||||
index={index}
|
index={index}
|
||||||
memberCount={memberCount}
|
memberCount={memberCount}
|
||||||
|
onFocus={onFocus}
|
||||||
/>
|
/>
|
||||||
);
|
);
|
||||||
} else {
|
} else {
|
||||||
@@ -66,6 +72,7 @@ const MemberListView: React.FC<IProps> = (props: IProps) => {
|
|||||||
tabIndex={isRovingItem ? 0 : -1}
|
tabIndex={isRovingItem ? 0 : -1}
|
||||||
memberIndex={index - 1} // Adjust as invites are below the separator
|
memberIndex={index - 1} // Adjust as invites are below the separator
|
||||||
memberCount={memberCount}
|
memberCount={memberCount}
|
||||||
|
onFocus={onFocus}
|
||||||
/>
|
/>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ interface IProps {
|
|||||||
showPresence?: boolean;
|
showPresence?: boolean;
|
||||||
focused?: boolean;
|
focused?: boolean;
|
||||||
tabIndex?: number;
|
tabIndex?: number;
|
||||||
|
onFocus: (e: React.FocusEvent) => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function RoomMemberTileView(props: IProps): JSX.Element {
|
export function RoomMemberTileView(props: IProps): JSX.Element {
|
||||||
@@ -59,6 +60,7 @@ export function RoomMemberTileView(props: IProps): JSX.Element {
|
|||||||
return (
|
return (
|
||||||
<MemberTileView
|
<MemberTileView
|
||||||
onClick={vm.onClick}
|
onClick={vm.onClick}
|
||||||
|
onFocus={props.onFocus}
|
||||||
avatarJsx={av}
|
avatarJsx={av}
|
||||||
presenceJsx={presenceJSX}
|
presenceJsx={presenceJSX}
|
||||||
nameJsx={nameJSX}
|
nameJsx={nameJSX}
|
||||||
|
|||||||
@@ -19,6 +19,7 @@ interface Props {
|
|||||||
memberCount: number;
|
memberCount: number;
|
||||||
focused?: boolean;
|
focused?: boolean;
|
||||||
tabIndex?: number;
|
tabIndex?: number;
|
||||||
|
onFocus: (e: React.FocusEvent) => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
export function ThreePidInviteTileView(props: Props): JSX.Element {
|
export function ThreePidInviteTileView(props: Props): JSX.Element {
|
||||||
@@ -39,6 +40,7 @@ export function ThreePidInviteTileView(props: Props): JSX.Element {
|
|||||||
iconJsx={iconJsx}
|
iconJsx={iconJsx}
|
||||||
focused={props.focused}
|
focused={props.focused}
|
||||||
tabIndex={props.tabIndex}
|
tabIndex={props.tabIndex}
|
||||||
|
onFocus={props.onFocus}
|
||||||
/>
|
/>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ interface Props {
|
|||||||
avatarJsx: JSX.Element;
|
avatarJsx: JSX.Element;
|
||||||
nameJsx: JSX.Element | string;
|
nameJsx: JSX.Element | string;
|
||||||
onClick: () => void;
|
onClick: () => void;
|
||||||
|
onFocus: (e: React.FocusEvent) => void;
|
||||||
memberIndex: number;
|
memberIndex: number;
|
||||||
memberCount: number;
|
memberCount: number;
|
||||||
ariaLabel?: string;
|
ariaLabel?: string;
|
||||||
@@ -41,6 +42,7 @@ export function MemberTileView(props: Props): JSX.Element {
|
|||||||
ref={ref}
|
ref={ref}
|
||||||
className="mx_MemberTileView"
|
className="mx_MemberTileView"
|
||||||
onClick={props.onClick}
|
onClick={props.onClick}
|
||||||
|
onFocus={props.onFocus}
|
||||||
aria-label={props?.ariaLabel}
|
aria-label={props?.ariaLabel}
|
||||||
tabIndex={props.tabIndex}
|
tabIndex={props.tabIndex}
|
||||||
role="option"
|
role="option"
|
||||||
|
|||||||
@@ -35,7 +35,9 @@ describe("MemberTileView", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it("should not display an E2EIcon when the e2E status = normal", () => {
|
it("should not display an E2EIcon when the e2E status = normal", () => {
|
||||||
const { container } = render(<RoomMemberTileView member={member} index={0} memberCount={1} />);
|
const { container } = render(
|
||||||
|
<RoomMemberTileView member={member} index={0} memberCount={1} onFocus={jest.fn()} />,
|
||||||
|
);
|
||||||
const e2eIcon = container.querySelector(".mx_E2EIconView");
|
const e2eIcon = container.querySelector(".mx_E2EIconView");
|
||||||
expect(e2eIcon).toBeNull();
|
expect(e2eIcon).toBeNull();
|
||||||
expect(container).toMatchSnapshot();
|
expect(container).toMatchSnapshot();
|
||||||
@@ -47,7 +49,9 @@ describe("MemberTileView", () => {
|
|||||||
wasCrossSigningVerified: jest.fn().mockReturnValue(true),
|
wasCrossSigningVerified: jest.fn().mockReturnValue(true),
|
||||||
} as unknown as UserVerificationStatus);
|
} as unknown as UserVerificationStatus);
|
||||||
|
|
||||||
const { container } = render(<RoomMemberTileView member={member} index={0} memberCount={1} />);
|
const { container } = render(
|
||||||
|
<RoomMemberTileView member={member} index={0} memberCount={1} onFocus={jest.fn()} />,
|
||||||
|
);
|
||||||
await waitFor(async () => {
|
await waitFor(async () => {
|
||||||
await userEvent.hover(container.querySelector(".mx_E2EIcon")!);
|
await userEvent.hover(container.querySelector(".mx_E2EIcon")!);
|
||||||
expect(screen.getByText("This user has not verified all of their sessions.")).toBeInTheDocument();
|
expect(screen.getByText("This user has not verified all of their sessions.")).toBeInTheDocument();
|
||||||
@@ -68,7 +72,9 @@ describe("MemberTileView", () => {
|
|||||||
crossSigningVerified: true,
|
crossSigningVerified: true,
|
||||||
} as DeviceVerificationStatus);
|
} as DeviceVerificationStatus);
|
||||||
|
|
||||||
const { container } = render(<RoomMemberTileView member={member} index={0} memberCount={1} />);
|
const { container } = render(
|
||||||
|
<RoomMemberTileView member={member} index={0} memberCount={1} onFocus={jest.fn()} />,
|
||||||
|
);
|
||||||
|
|
||||||
await waitFor(async () => {
|
await waitFor(async () => {
|
||||||
await userEvent.hover(container.querySelector(".mx_E2EIcon")!);
|
await userEvent.hover(container.querySelector(".mx_E2EIcon")!);
|
||||||
@@ -81,15 +87,21 @@ describe("MemberTileView", () => {
|
|||||||
|
|
||||||
it("renders user labels correctly", async () => {
|
it("renders user labels correctly", async () => {
|
||||||
member.powerLevel = 50;
|
member.powerLevel = 50;
|
||||||
const { container: container1 } = render(<RoomMemberTileView member={member} index={0} memberCount={1} />);
|
const { container: container1 } = render(
|
||||||
|
<RoomMemberTileView member={member} index={0} memberCount={1} onFocus={jest.fn()} />,
|
||||||
|
);
|
||||||
expect(container1).toHaveTextContent("Moderator");
|
expect(container1).toHaveTextContent("Moderator");
|
||||||
|
|
||||||
member.powerLevel = 100;
|
member.powerLevel = 100;
|
||||||
const { container: container2 } = render(<RoomMemberTileView member={member} index={0} memberCount={1} />);
|
const { container: container2 } = render(
|
||||||
|
<RoomMemberTileView member={member} index={0} memberCount={1} onFocus={jest.fn()} />,
|
||||||
|
);
|
||||||
expect(container2).toHaveTextContent("Admin");
|
expect(container2).toHaveTextContent("Admin");
|
||||||
|
|
||||||
member.isInvite = true;
|
member.isInvite = true;
|
||||||
const { container: container3 } = render(<RoomMemberTileView member={member} index={0} memberCount={1} />);
|
const { container: container3 } = render(
|
||||||
|
<RoomMemberTileView member={member} index={0} memberCount={1} onFocus={jest.fn()} />,
|
||||||
|
);
|
||||||
expect(container3).toHaveTextContent("Invited");
|
expect(container3).toHaveTextContent("Invited");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
@@ -110,7 +122,12 @@ describe("MemberTileView", () => {
|
|||||||
it("renders ThreePidInvite correctly", async () => {
|
it("renders ThreePidInvite correctly", async () => {
|
||||||
const [{ threePidInvite }] = getPending3PidInvites(room);
|
const [{ threePidInvite }] = getPending3PidInvites(room);
|
||||||
const { container } = render(
|
const { container } = render(
|
||||||
<ThreePidInviteTileView threePidInvite={threePidInvite!} memberIndex={0} memberCount={1} />,
|
<ThreePidInviteTileView
|
||||||
|
threePidInvite={threePidInvite!}
|
||||||
|
memberIndex={0}
|
||||||
|
memberCount={1}
|
||||||
|
onFocus={jest.fn()}
|
||||||
|
/>,
|
||||||
);
|
);
|
||||||
expect(container).toMatchSnapshot();
|
expect(container).toMatchSnapshot();
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -334,6 +334,82 @@ describe("ListView", () => {
|
|||||||
|
|
||||||
expect(container).toBeInTheDocument();
|
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 (
|
||||||
|
<div
|
||||||
|
className="mx_item"
|
||||||
|
data-testid={`row-${index}`}
|
||||||
|
tabIndex={isFocused ? 0 : -1}
|
||||||
|
onClick={() => mockOnClick(item)}
|
||||||
|
onFocus={onFocus}
|
||||||
|
>
|
||||||
|
{item === SEPARATOR_ITEM ? "---" : (item as TestItem).name}
|
||||||
|
</div>
|
||||||
|
);
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
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", () => {
|
describe("Accessibility", () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user