From a2535cf1b41bc2ac0f351af5324c15e9436e6486 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 8 Jul 2026 16:18:19 +0200 Subject: [PATCH 1/2] Fix alt+arrow up/down navigation in room list --- .../viewmodels/room-list/RoomListViewModel.ts | 86 +++++++++++++++++-- .../room-list/RoomListViewModel-test.tsx | 75 ++++++++++++++++ 2 files changed, 154 insertions(+), 7 deletions(-) diff --git a/apps/web/src/viewmodels/room-list/RoomListViewModel.ts b/apps/web/src/viewmodels/room-list/RoomListViewModel.ts index 9b913475b46..1345df67530 100644 --- a/apps/web/src/viewmodels/room-list/RoomListViewModel.ts +++ b/apps/web/src/viewmodels/room-list/RoomListViewModel.ts @@ -137,6 +137,13 @@ export class RoomListViewModel */ private scrollToIndex?: (index: number) => void; + /** + * Signature of the store's room list ({@link roomListSignature}) as of the last update, used to + * detect no-op list updates: selecting a room sends a read receipt that re-emits the same list, + * and we must not re-sort then or rooms shuffle around just from being selected. + */ + private lastRoomListSignature: number = 0; + public constructor(props: RoomListViewModelProps) { const activeSpace = SpaceStore.instance.activeSpaceRoom; @@ -167,6 +174,7 @@ export class RoomListViewModel }); this.roomsResult = roomsResult; + this.lastRoomListSignature = roomListSignature(roomsResult); this.sections = sections; // Build initial roomsMap from roomsResult @@ -242,6 +250,7 @@ export class RoomListViewModel // Update rooms result with new filter const filterKeys = this.activeFilter !== undefined ? [this.activeFilter] : undefined; this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); + this.lastRoomListSignature = roomListSignature(this.roomsResult); // Update roomsMap immediately before clearing VMs this.updateRoomsMap(this.roomsResult); @@ -272,10 +281,11 @@ export class RoomListViewModel } /** - * Get the ordered list of room IDs. + * The room IDs in displayed order (sticky-adjusted, collapse-filtered), matching the indices the + * view reports to {@link updateVisibleRooms}. */ public get roomIds(): string[] { - return this.roomsResult.sections.flatMap((section) => section.rooms).map((room) => room.roomId); + return this.sections.flatMap((section) => section.rooms).map((room) => room.roomId); } /** @@ -483,6 +493,25 @@ export class RoomListViewModel if (index !== undefined) this.scrollToIndex?.(index); } + /** + * Settle the list to the store's real order after a room is clicked, keeping the clicked room at + * its current displayed position (making it the new sticky room) so the previously sticky room + * falls back into place. No-op if the room is not in the list. + */ + private async settleRoomOrder(roomId: string): Promise { + const position = this.findRoomPosition(this.roomsResult.sections, roomId); + if (!position) return; + + // Anchor the sticky room to the clicked room at its current position, then re-sort from the + // store's real order around it. + this.lastActiveRoomPosition = position; + const filterKeys = this.activeFilter !== undefined ? [this.activeFilter] : undefined; + this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); + this.lastRoomListSignature = roomListSignature(this.roomsResult); + this.updateRoomsMap(this.roomsResult); + await this.updateRoomListData(false, roomId); + } + /** * Compute a room's index in the list's entry space, or undefined if it is not in the displayed * sections (e.g. collapsed or filtered out). Flat list: the room index; grouped list: includes @@ -511,8 +540,12 @@ export class RoomListViewModel // Handle keyboard navigation shortcuts (Alt+ArrowUp/Down) // This was previously handled by useRoomListNavigation hook this.handleViewRoomDelta(payload as ViewRoomDeltaPayload); - } else if (payload.action === Action.ViewRoom && payload.show_room_tile && payload.room_id) { - await this.scrollRoomIntoView(payload.room_id); + } else if (payload.action === Action.ViewRoom && payload.room_id) { + if (payload.show_room_tile) { + await this.scrollRoomIntoView(payload.room_id); + } else { + await this.settleRoomOrder(payload.room_id); + } } else if (payload.action === Action.RoomListCollapseAllSections) { this.onCollapseAllSections(false); } else if (payload.action === Action.RoomListExpandAllSections) { @@ -588,11 +621,14 @@ export class RoomListViewModel const oldSpaceId = this.roomsResult.spaceId; // Refresh room data from store - this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); - const newSpaceId = this.roomsResult.spaceId; + const freshResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); + const newSpaceId = freshResult.spaceId; // Detect space change if (oldSpaceId !== newSpaceId) { + this.roomsResult = freshResult; + this.lastRoomListSignature = roomListSignature(freshResult); + // Clear view models when the space changes // We only want to do this on space changes, not on regular list updates, to preserve view models when possible // The view models are disposed when scrolling out of view (handled by updateVisibleRooms) @@ -614,6 +650,17 @@ export class RoomListViewModel return; } + // No-op update: the store returned the same list as last time (e.g. selecting a room sent a + // read receipt, which the recency sort re-inserts in place). Pass isRoomChange to keep the + // displayed order so rooms don't shuffle around, while still recomputing derived state. + const newSignature = roomListSignature(freshResult); + if (newSignature === this.lastRoomListSignature) { + this.updateRoomListData(true); + return; + } + + this.roomsResult = freshResult; + this.lastRoomListSignature = newSignature; this.updateRoomsMap(this.roomsResult); // Normal room list update (not a space change) @@ -658,7 +705,7 @@ export class RoomListViewModel * Apply sticky room logic to keep the active room at the same position within its section. * When the room list updates, this prevents the selected room from jumping around in the UI. * - * @param isRoomChange - Whether this update is due to a room change (not a list update) + * @param isRoomChange - When true, keep the current order (skip sticky re-sorting) * @param roomId - The room ID to apply sticky logic for (can be null/undefined) * @returns The modified sections array with sticky positioning applied */ @@ -708,6 +755,13 @@ export class RoomListViewModel }); } + /** + * Updates the room list data. + * @param isRoomChange - Keep the current displayed order instead of re-sorting. Set for a room + * change (selecting a room must not reshuffle the list) or a no-op list update. + * @param roomIdOverride - Optional room ID to use for calculations. + * @param scrollToSectionTag - Optional section tag to scroll to. + */ private async updateRoomListData( isRoomChange: boolean = false, roomIdOverride: string | null = null, @@ -837,6 +891,7 @@ export class RoomListViewModel // Refresh roomsResult so the new section lands in the same snapshot as the scroll-to. const filterKeys = this.activeFilter !== undefined ? [this.activeFilter] : undefined; this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); + this.lastRoomListSignature = roomListSignature(this.roomsResult); this.updateRoomsMap(this.roomsResult); this.updateRoomListData(false, null, tag); this.showToast("section_created"); @@ -884,6 +939,7 @@ export class RoomListViewModel // Scroll to the section after it moved const filterKeys = this.activeFilter !== undefined ? [this.activeFilter] : undefined; this.roomsResult = RoomListStoreV3.instance.getSortedRoomsInActiveSpace(filterKeys); + this.lastRoomListSignature = roomListSignature(this.roomsResult); this.updateRoomsMap(this.roomsResult); this.updateRoomListData(false, null, sourceTag); }; @@ -918,6 +974,22 @@ export class RoomListViewModel }; } +/** + * A signature of the store's room list capturing section order, section membership and room order, + * hashed to a fixed-size number so it doesn't grow with the number of rooms. Two results with the + * same signature render to the same list, so an update between them is a no-op that we can skip + * re-sorting for (see {@link RoomListViewModel.lastRoomListSignature}). + */ +function roomListSignature(roomsResult: RoomsResult): number { + const signature = roomsResult.sections.map((s) => `${s.tag}:${s.rooms.map((r) => r.roomId).join(",")}`).join("|"); + // FNV-1a 32-bit hash. + let hash = 0x811c9dc5; + for (let i = 0; i < signature.length; i++) { + hash = Math.imul(hash ^ (signature.codePointAt(i) || 0), 0x01000193); + } + return hash >>> 0; +} + /** * Compute the sections to display in the room list based on the rooms result and section expansion state. * @param roomsResult - The current rooms result containing sections and rooms diff --git a/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx b/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx index e71e82ddcf4..38971834cc7 100644 --- a/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx +++ b/apps/web/test/viewmodels/room-list/RoomListViewModel-test.tsx @@ -303,6 +303,81 @@ describe("RoomListViewModel", () => { await flushPromises(); expect(viewModel.getSnapshot().roomListState.activeRoomIndex).toBe(2); }); + + /** + * Set up a displayed order of room1, room2, room3 where room3 is the active room kept sticky + * at the bottom even though the store bumped it to the top (so the displayed order differs + * from the store's real order of room3, room1, room2). + */ + const setUpStickyRoom3 = async (): Promise => { + jest.spyOn(SDKContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!room3:server"); + dispatcher.dispatch({ action: Action.ActiveRoomChanged, newRoomId: "!room3:server" }); + await flushPromises(); + + jest.spyOn(RoomListStoreV3.instance, "getSortedRoomsInActiveSpace").mockReturnValue({ + spaceId: "home", + sections: [{ tag: CHATS_TAG, rooms: [room3, room1, room2] }], + }); + RoomListStoreV3.instance.emit(RoomListStoreV3Event.ListsUpdate); + expect(viewModel.getSnapshot().sections[0].roomIds).toEqual([ + "!room1:server", + "!room2:server", + "!room3:server", + ]); + }; + + it("should not reshuffle when navigating with the keyboard (show_room_tile)", async () => { + stubClient(); + viewModel = new RoomListViewModel({ client: matrixClient }); + await setUpStickyRoom3(); + + // Navigate to room1 with the keyboard: dispatches ViewRoom with show_room_tile. + jest.spyOn(SDKContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!room1:server"); + dispatcher.dispatch({ + action: Action.ViewRoom, + room_id: "!room1:server", + show_room_tile: true, + metricsTrigger: undefined, + }); + dispatcher.dispatch({ + action: Action.ActiveRoomChanged, + oldRoomId: "!room3:server", + newRoomId: "!room1:server", + }); + await flushPromises(); + + // A no-op list update (e.g. a read receipt from selecting the room) must not move any room. + RoomListStoreV3.instance.emit(RoomListStoreV3Event.ListsUpdate); + expect(viewModel.getSnapshot().sections[0].roomIds).toEqual([ + "!room1:server", + "!room2:server", + "!room3:server", + ]); + }); + + it("should reshuffle to the real order when switching rooms with a click", async () => { + stubClient(); + viewModel = new RoomListViewModel({ client: matrixClient }); + await setUpStickyRoom3(); + + // Click room1: dispatches ViewRoom without show_room_tile. + jest.spyOn(SDKContextClass.instance.roomViewStore, "getRoomId").mockReturnValue("!room1:server"); + dispatcher.dispatch({ action: Action.ViewRoom, room_id: "!room1:server", metricsTrigger: undefined }); + dispatcher.dispatch({ + action: Action.ActiveRoomChanged, + oldRoomId: "!room3:server", + newRoomId: "!room1:server", + }); + await flushPromises(); + + // The no-op list update now lets the list settle to the store's real order (room1 sticky). + RoomListStoreV3.instance.emit(RoomListStoreV3Event.ListsUpdate); + expect(viewModel.getSnapshot().sections[0].roomIds).toEqual([ + "!room1:server", + "!room3:server", + "!room2:server", + ]); + }); }); describe("Filters", () => { From 61944f61df86a394971a5470735c734527d75035 Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Wed, 8 Jul 2026 16:53:36 +0200 Subject: [PATCH 2/2] Add e2e tests for alt+arrow up/down navigation --- .../room-list-panel/room-list.spec.ts | 40 ++++++++++++++++++- .../e2e/left-panel/room-list-panel/utils.ts | 19 +++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts b/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts index d1249977bcc..c745eeec02c 100644 --- a/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts +++ b/apps/web/playwright/e2e/left-panel/room-list-panel/room-list.spec.ts @@ -11,7 +11,7 @@ import { closeReleaseAnnouncement, rejectToast } from "@element-hq/element-web-p import { expect, test } from "../../../element-web-test"; import { type Bot } from "../../../pages/bot"; import { type ElementAppPage } from "../../../pages/ElementAppPage"; -import { getRoomList } from "./utils"; +import { assertRoomListOrder, getRoomList } from "./utils"; test.describe("Room list", () => { test.use({ @@ -313,6 +313,44 @@ test.describe("Room list", () => { }); }); + test.describe("Room navigation with the keyboard", () => { + // Alt+ArrowUp/Down steps through rooms in their displayed order. Selecting a room sends a read + // receipt that re-emits the (unchanged) list, which used to reshuffle it under the cursor so + // the next press opened the wrong room. + test("should step through the rooms in their displayed order", async ({ page, app, user, bot }) => { + const roomListView = getRoomList(page); + + // Creation order sets recency (newest on top): roomC, roomB, roomA. + const roomAId = await app.client.createRoom({ name: "roomA", invite: [bot.credentials.userId] }); + await bot.joinRoom(roomAId); + await app.client.createRoom({ name: "roomB" }); + await app.client.createRoom({ name: "roomC" }); + + const roomA = roomListView.getByRole("option", { name: "Open room roomA" }); + const roomB = roomListView.getByRole("option", { name: "Open room roomB" }); + const roomC = roomListView.getByRole("option", { name: "Open room roomC" }); + + // Open roomB, then bump roomA to the top. roomB is active so it stays put (sticky), giving + // a displayed order of roomA, roomB, roomC that differs from the recency order. + await roomB.click(); + await expect(roomB).toHaveAttribute("aria-selected", "true"); + await bot.sendMessage(roomAId, "bump"); + await assertRoomListOrder(page, ["roomA", "roomB", "roomC"]); + + // Up from roomB selects roomA (shown above it) in the room list. + await page.keyboard.press("Alt+ArrowUp"); + await expect(roomA).toHaveAttribute("aria-selected", "true"); + + // Down must select the next visible room (roomB), not roomC as it did when the list reshuffled. + await page.keyboard.press("Alt+ArrowDown"); + await expect(roomB).toHaveAttribute("aria-selected", "true"); + + // Once more selects roomC. + await page.keyboard.press("Alt+ArrowDown"); + await expect(roomC).toHaveAttribute("aria-selected", "true"); + }); + }); + test.describe("Avatar decoration", () => { test.use({ labsFlags: ["feature_video_rooms"] }); diff --git a/apps/web/playwright/e2e/left-panel/room-list-panel/utils.ts b/apps/web/playwright/e2e/left-panel/room-list-panel/utils.ts index 78d2a6c032b..1e3afcca02e 100644 --- a/apps/web/playwright/e2e/left-panel/room-list-panel/utils.ts +++ b/apps/web/playwright/e2e/left-panel/room-list-panel/utils.ts @@ -160,6 +160,25 @@ export async function assertSectionsOrder(page: Page, expectedOrder: string[]): } } +/** + * Assert that the given rooms appear in the room list in the given top-to-bottom order. + * Retries until the list settles so it is robust to async re-sorts. + * @param page + * @param roomNames the room names, in the expected top-to-bottom order + */ +export async function assertRoomListOrder(page: Page, roomNames: string[]): Promise { + const roomList = getRoomList(page); + await expect(async () => { + let previousY = -Infinity; + for (const name of roomNames) { + const box = await roomList.getByRole("option", { name: `Open room ${name}` }).boundingBox(); + expect(box, `expected room "${name}" to be visible in the room list`).not.toBeNull(); + expect(box!.y, `expected room "${name}" to be below the previous room`).toBeGreaterThan(previousY); + previousY = box!.y; + } + }).toPass(); +} + /** * Get the primary filters container * @param page