From d6f11d828b1c8d31232b62bb2ca5339594ef2429 Mon Sep 17 00:00:00 2001 From: David Langley Date: Mon, 26 Jan 2026 17:58:46 +0000 Subject: [PATCH] Migrate ListView to shared components (#31860) * Migrate ListView to shared components * Add stories * lint * Update name of component * Use compound spacing * lint * VirtualizedList * Simplify story * Add git diff check before uploading artifacts * Fix git diff workaround for vis * Ignore coverage report in .gitignore Add coverage report to .gitignore * Add screenshot test * Fix package and lock files * clear unneeded lock file changes --------- Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> --- .../shared-component-visual-tests.yaml | 6 + jest.config.ts | 2 +- package.json | 1 - packages/shared-components/.gitignore | 3 + .../default-auto.png | Bin 0 -> 8074 bytes packages/shared-components/package.json | 1 + packages/shared-components/src/index.ts | 1 + .../VirtualizedList.stories.tsx | 52 +++++ .../VirtualizedList/VirtualizedList.test.tsx | 193 ++++++++++-------- .../utils/VirtualizedList/VirtualizedList.tsx | 50 +++-- .../src/utils/VirtualizedList/index.ts | 13 ++ packages/shared-components/vite.config.ts | 8 +- packages/shared-components/yarn.lock | 5 + .../views/rooms/MemberList/MemberListView.tsx | 7 +- .../views/rooms/RoomListPanel/RoomList.tsx | 10 +- .../rooms/RoomListPanel/RoomList-test.tsx | 2 +- .../views/rooms/memberlist/common.tsx | 4 +- 17 files changed, 245 insertions(+), 113 deletions(-) create mode 100644 packages/shared-components/__vis__/linux/__baselines__/utils/VirtualizedList/VirtualizedList.stories.tsx/default-auto.png create mode 100644 packages/shared-components/src/utils/VirtualizedList/VirtualizedList.stories.tsx rename test/unit-tests/components/views/utils/ListView-test.tsx => packages/shared-components/src/utils/VirtualizedList/VirtualizedList.test.tsx (72%) rename src/components/utils/ListView.tsx => packages/shared-components/src/utils/VirtualizedList/VirtualizedList.tsx (90%) create mode 100644 packages/shared-components/src/utils/VirtualizedList/index.ts diff --git a/.github/workflows/shared-component-visual-tests.yaml b/.github/workflows/shared-component-visual-tests.yaml index f07e015639..b8f0afdaa1 100644 --- a/.github/workflows/shared-component-visual-tests.yaml +++ b/.github/workflows/shared-component-visual-tests.yaml @@ -56,6 +56,12 @@ jobs: working-directory: packages/shared-components run: "yarn test:storybook --run" + # Workaround for vis silently adding new baselines if they didn't exist + # Can be removed once https://github.com/repobuddy/visual-testing/issues/516 is released + - run: | + git add -N . + git diff --exit-code + - name: Upload received images & diffs if: always() uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6 diff --git a/jest.config.ts b/jest.config.ts index 5cbe01973c..8662a89ca7 100644 --- a/jest.config.ts +++ b/jest.config.ts @@ -43,7 +43,7 @@ const config: Config = { "@vector-im/compound-web": "/node_modules/@vector-im/compound-web", }, transformIgnorePatterns: [ - "/node_modules/(?!(mime|matrix-js-sdk|uuid|p-retry|is-network-error|react-merge-refs|is-ip|ip-regex|super-regex|function-timeout|time-span|convert-hrtime|clone-regexp|is-regexp|matrix-web-i18n|await-lock)).+$", + "/node_modules/(?!(mime|matrix-js-sdk|uuid|p-retry|is-network-error|react-merge-refs|is-ip|ip-regex|super-regex|function-timeout|time-span|convert-hrtime|clone-regexp|is-regexp|matrix-web-i18n|await-lock|@element-hq/web-shared-components|react-virtuoso)).+$", ], collectCoverageFrom: [ "/src/**/*.{js,ts,tsx}", diff --git a/package.json b/package.json index 59e05b2554..029c1af07d 100644 --- a/package.json +++ b/package.json @@ -148,7 +148,6 @@ "react-focus-lock": "^2.5.1", "react-string-replace": "^2.0.0", "react-transition-group": "^4.4.1", - "react-virtuoso": "^4.14.0", "rfc4648": "^1.4.0", "sanitize-filename": "^1.6.3", "sanitize-html": "2.17.0", diff --git a/packages/shared-components/.gitignore b/packages/shared-components/.gitignore index 78e081f843..b1f29b1f0f 100644 --- a/packages/shared-components/.gitignore +++ b/packages/shared-components/.gitignore @@ -5,3 +5,6 @@ /__vis__/**/__diffs__ /__vis__/**/__results__ /__vis__/local + +# Ignore coverage report +/coverage/ diff --git a/packages/shared-components/__vis__/linux/__baselines__/utils/VirtualizedList/VirtualizedList.stories.tsx/default-auto.png b/packages/shared-components/__vis__/linux/__baselines__/utils/VirtualizedList/VirtualizedList.stories.tsx/default-auto.png new file mode 100644 index 0000000000000000000000000000000000000000..03ef57533ef2042a084ab5b833c6c92f0532531a GIT binary patch literal 8074 zcmeI1X;@R|w#PTpQ?yj+sRLF(+6M&{gesIFAq0nt2!h2PP(b2<1Ox#g5Xgx2QtAK{ zC}LzTD0(nNWNLsU4n<^0f*>;q^N=tE0wiRlpRQl`v-dl#-}=A*wf4nB zZcZ!T+wdL$04tsM@AUux&HDhbtY!H!@JY+sq+tN~0C3*B>&uv|DbB9IEHP(buKp8$ z)CNQ}Zf;A$m70O4dQWa?r{6nN{12BEwlllK*Xhd!4&3#YoxZT$f68O^KQg*v@{L{2 zme+Pb_LgKM@-Kuu*w?Xn7I5hw|JsXe)RJ%H@K{O)g5lxZ+vyXm-K3W|!rN~n@Pi12-1CQteFp#_ zuQzH#OY$uW=Ld%><(JDKoNJl(Q3YR^jqO|k0CHfjm+gjhQrls)!);2EAtjgbsrP}7 zP`6{XbWis**TF=~ai;{K^J(eCkBZ$}eYR(qE=~S;p*HnU#`_Ba(EY|=A5QkCAvZt|;5qHY56C(@lPg_~c__Eb)ynJ5Ws4Nm0&1OW- zh#rfQoXR}UF1WJ@s)$(3sSz5q$;B&Svdq#YlPS=zDzjmfnP>oUhL`G(3@&v9 z#O9}R;MTr@HnSz?PdUxE*F}(zwZNzk4{hyF^PjKN6-sD(>O{}%d?wh))~|P20suVF zUAQjEirg7IdgFoxUBx8tE=^g}o7?Rw3iDx-Dk&v7E*^^af%`hWY44zn)0#%LjFArO z`{$o**jFYz2Qow~+ITAd6gDTzILOi&>fV#E8+gYjc{&Be_$ zEUvXQUH!2$Y>|!%U021N6$myKBL%em7bJr$fvUfy9evFlGuH{XTUjIn2?nd zPR}|+eRF+?8#Fj~SZ-N4Y-NTXaT^8qX-+m2Epj`!TN8ZmdsF?y#%A7aXznww`s7YF zV*U2b!8S$aeu&~C=bN*SN%IAJB!xTis3AiBcjT*0>6qwn6fS(eLw=^>>Y92k9d&Yx z_(>3nZp0uIS=A!FQm3R1`*%dDVm z7Yi^kio8mXgW?6(U^y(ue724G%UhB72Xf<@> z=HhV#!J$%E6lV@k$TZaHKoX;*p@ZbIM-nhN)m>?|@kR$H7rkS71D6HF$l^mG zo9aYJ>og_~Z`vkmZ;y1A!W4ad)#yP(HhC?#S}K~PRoZ|X40Zpoh2(0UVR$V|avaAt z8tlD;)3-e}$qh46VlgAqz6W2$ybI*XV2|Ic7unk#Yz;_VxVmb=W?cyfyTreuHQIf9 zeqINi2W`K@Qt}y|P6XJE91URU{5M)C_Y%srqonm4?JDlJV~_JWk9f_cDPAH0HcI|z zU#UedPhJ?XsGP>;nk)GgFt)BxNA;dlSSTjh0AxGl|A6heI$br}AFWoS-1YQK1sLKy zOXJ@Z&EIGGB^my8bqxv(<<>+DIr5z1A2v~PvyO7q5?lO|#9A$@rV@s|7)N=TwF+XR zsS0IFg2JL|u7a7Td$}asYMW(I(G1Q5VF34o6Hk zo?ev2dsKRxd-1y;Je_;M^qDVi_x8}o%33ex_ZbzPmL<8aS~WGvA#H*ZAs#V;s&vf! zak^S0&DUl-B4ykxebecU5IcNSd3j{y;M9*OOd#f*MA=8;%nPz=Xr;8_r+5CPr${R^ z+|D#$lYMiz>th8hVQc%KbgH>EU_Rv$MtH&K5guB_xNB_#E6Xv^-*yWLV%dG_PB<>O ztoBgDjG-R4gnLZ0K54A@@~C%B9yTIA-vj$?7E>4TWL&&zZSUdnkbHQqvz|&d<`g;I zI?Ldo!-qV_S}5)-16G5ep8PFUww_-kyY_l{u{ZO?A#UP@T;*`C4n;9HmfjSW{lu4H z?QQj6Ai=?~hIb%o4WmNMG_=Z>tQehSV395)mQKuFI!?wFc{sV#RRccAfd?4 zv_Qf?N$oJ7$niaK>pOw-J;{)^M9$4WiREW)Ul_Lx8m`GSOl&kH%%2c--5%U+tad1I zCb8O1*E=RFgsl&B!PD@}@7jN9njK;+tJ=gW$44#trV@&B4exMjKg}?Zan5sd{7qkc zw_?K8IHO6&m4*A1UNqiY!)&b2#>2>D^S)!Mg&FqcB09ayYC!;}3mHYkQuRkO3+qkw zd*?^2oHLzUeIPCUM}CN=RMNWSdG^pK0-<0&BgkKxWbsVtq2Py2qh;DM0$%LS{|JaH zS*HW;3bCVX=MMXbGbf70_n*}yw2@93QKtIE{H27%lL+ok&^8g-ZxY$(Ms{`zp^|p0 z%|~<6Pgc0}A!vpFyz6}>5nhbyn!x_dCUj%0WYILe98asLyj%Npe~xdNH5L`7lsxQL z`auXaIcf#QURi;{Pd{Gl-D9B$akq~3NVH-a$~nwptw^;W{$Q_W^%o>NSk!o$l)p45 zm1q~-@Sh%gRyP^EXw|TWYa2;GZOQ%eX0ltK#dr7GBd8wiRY^%Yq1@{Ta!2Y=S8s;f z`>@u=pz3tv?vUq5eB+AYBdjldXqiD!$UoGkNu0~*DD~# zh$uuwuzJXVpKZ>OK~SJyv#5J@)SHjSuj|<)*UF{o2YTpSZ=WOFz)kL-m(g*9vvlH? zH3Z!8g@+IKS%QKP+7aXk|DGN_EbWsXUNM0h)+s9TMA$AeX2_t&_Q4CPh<7%FUa9Vl zc$N9 zndTV@n#JLEapwYmPoSS}PL#r{nra-x9ka>W5<|sEOSezD7MP%@vj3<7C!AAXH6V18 zhOx^QBlFmVOVb*Wl`XmK!bB$<`1Kpn0C(izyQbpW7asqO`|-qrz+X4W%wQBLa5 zIJ-+-f8>m8*AMchbT+zZA#kcppZJqTTPxvZq>!;mpBIHjqG}Gf8GmvlgvJXlMhcANjPb~~)LGwmhHTQ5$LjV#s);czblTl^sNdje>g zGN%J8yq_Cp_~>gj8(!h^m#)M>H`_sGrtW5x869i`H}{gVYyO=nVtyU{J4dO638#4Z ze7|yS61OizU368vR9(KO!D&N>%&0triLp!KaV!IWRxr@wLVX?>PgH!t-<9`Ky10)C z_pA1lH9!UH7QuUxV+J|T64C7JJbPfM2q`}>XngEp_eI`9%fgUQLA9(I@3%u^Phsl| zjuu->O^|T!>(Cq@sy%IokefYvqxjKcH|d6;zon}hJ<2b_*Vk*IpV!Q_AJqv$*6g*%D2f7T56G@PDyNPRd`_#&Ck@r05_)y3tf9D zv9L?o9B_|B2|L|Guf~`=Gbo@bVRMfN3>1qSd-cFPqnW(qNR)vxkYJO4GJEPx6Nip$ z%~e}G?3J}i)3Fi1Dd3PoKSf~*{zUR3NC9E_kOn7h(lo%+_-iM(?IDXBuVKXDkjRjc zbcM{%Ky~+vsV3xC^=Ad=3%hY(Zw$0j24qjr<^Yi4qT0XHCHnViUM@q^r7} zcKm0VzV=rea4!wrHWAXD@eTT?sd5bB0t1?D4(eBg9+DMp2!nZ2!C>#|v3IFQkcb~ld zgQ+tH*24{~+5hdwrbf|e&_BA)1gBIrfxzIAKGCQ)$ zrV?#`z2Zy(F+x;ugE~x2Z)(#s5^#?CRYQDpvyb-RP|ij^`eucbql?j2HdP&7|0cl{ zl5F+rDX$^b)Mzu^=beyjW^#`FL0bJi8}1aAes{cIAq+hsp9*&X7i|7p&cxpm(ck!! zy+%h$AMoQ5$JqKRMPqPX2p&H(L_M5=MnIv*m#+l5#iYiu{3EFH`y>ng@S?nnex4h>Q((4oP}PV5axfwUZr4MXt*GpdM1%1KwAv>4UBS= z+vX*nh6Gx5MAg;p^luNd)iI256pu(Rk?rX1Gxp=sUZ!m?QUda3vX1jXQwg{1k&>sv}m3UcUPB?Q$ODc z(7)d{`6Va*pnLuH#W&O5eS3~?&+)JSLwZZkTYBEo^LqbX|9|F;N(TT`Dv{k`1!x}L dRhQ42z~g?wj>+3jdEgVk`E$3uCA*J*_1`ZA)Yt$3 literal 0 HcmV?d00001 diff --git a/packages/shared-components/package.json b/packages/shared-components/package.json index 55a61a6e9f..f87d497502 100644 --- a/packages/shared-components/package.json +++ b/packages/shared-components/package.json @@ -59,6 +59,7 @@ "lodash": "^4.17.21", "matrix-web-i18n": "3.6.0", "react-merge-refs": "^3.0.2", + "react-virtuoso": "^4.14.0", "temporal-polyfill": "^0.3.0" }, "devDependencies": { diff --git a/packages/shared-components/src/index.ts b/packages/shared-components/src/index.ts index 62fd02c84b..49f6b4389f 100644 --- a/packages/shared-components/src/index.ts +++ b/packages/shared-components/src/index.ts @@ -24,6 +24,7 @@ export * from "./room-list/RoomListHeaderView"; export * from "./room-list/RoomListSearchView"; export * from "./utils/Box"; export * from "./utils/Flex"; +export * from "./utils/VirtualizedList"; // Utils export * from "./utils/i18n"; diff --git a/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.stories.tsx b/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.stories.tsx new file mode 100644 index 0000000000..a20fe188a3 --- /dev/null +++ b/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.stories.tsx @@ -0,0 +1,52 @@ +/* +Copyright 2026 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 React from "react"; + +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { VirtualizedList, type IVirtualizedListProps, type VirtualizedListContext } from "./VirtualizedList"; + +interface SimpleItem { + id: string; + label: string; +} + +const items: SimpleItem[] = Array.from({ length: 50 }, (_, i) => ({ + id: `item-${i}`, + label: `Item ${i + 1}`, +})); + +const meta = { + title: "Utils/VirtualizedList", + component: VirtualizedList, + args: { + items, + getItemComponent: ( + _index: number, + item: SimpleItem, + context: VirtualizedListContext, + onFocus: (item: SimpleItem, e: React.FocusEvent) => void, + ) => ( +
onFocus(item, e)} + > + {item.label} +
+ ), + isItemFocusable: () => true, + getItemKey: (item) => item.id, + style: { height: "400px" }, + }, +} satisfies Meta>; + +export default meta; +type Story = StoryObj; + +export const Default: Story = {}; diff --git a/test/unit-tests/components/views/utils/ListView-test.tsx b/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.test.tsx similarity index 72% rename from test/unit-tests/components/views/utils/ListView-test.tsx rename to packages/shared-components/src/utils/VirtualizedList/VirtualizedList.test.tsx index 0cae681f73..4dd5125b6f 100644 --- a/test/unit-tests/components/views/utils/ListView-test.tsx +++ b/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.test.tsx @@ -1,15 +1,24 @@ /* -Copyright 2024 New Vector Ltd. + * Copyright 2026 Element Creations 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. + */ -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 React from "react"; -import { render, screen, fireEvent } from "jest-matrix-react"; +import React, { type PropsWithChildren } from "react"; +import { render, screen, fireEvent } from "@test-utils"; import { VirtuosoMockContext } from "react-virtuoso"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { ListView, type IListViewProps } from "../../../../../src/components/utils/ListView"; +import { VirtualizedList, type IVirtualizedListProps } from "./VirtualizedList"; + +const expectTabIndex = (element: Element, expected: string): void => { + expect(element.getAttribute("tabindex")).toBe(expected); +}; + +const expectAttribute = (element: Element, attr: string, expected: string): void => { + expect(element.getAttribute(attr)).toBe(expected); +}; interface TestItem { id: string; @@ -20,9 +29,9 @@ interface TestItem { const SEPARATOR_ITEM = "SEPARATOR" as const; type TestItemWithSeparator = TestItem | typeof SEPARATOR_ITEM; -describe("ListView", () => { - const mockGetItemComponent = jest.fn(); - const mockIsItemFocusable = jest.fn(); +describe("VirtualizedList", () => { + const mockGetItemComponent = vi.fn(); + const mockIsItemFocusable = vi.fn(); const defaultItems: TestItemWithSeparator[] = [ { id: "1", name: "Item 1" }, @@ -31,22 +40,26 @@ describe("ListView", () => { { id: "3", name: "Item 3" }, ]; - const defaultProps: IListViewProps = { + const defaultProps: IVirtualizedListProps = { items: defaultItems, getItemComponent: mockGetItemComponent, isItemFocusable: mockIsItemFocusable, getItemKey: (item) => (typeof item === "string" ? item : item.id), }; - const getListViewComponent = (props: Partial> = {}) => { + const getListComponent = ( + props: Partial> = {}, + ): React.JSX.Element => { const mergedProps = { ...defaultProps, ...props }; - return ; + return ; }; - const renderListViewWithHeight = (props: Partial> = {}) => { + const renderListWithHeight = ( + props: Partial> = {}, + ): ReturnType => { const mergedProps = { ...defaultProps, ...props }; - return render(getListViewComponent(mergedProps), { - wrapper: ({ children }) => ( + return render(getListComponent(mergedProps), { + wrapper: ({ children }: PropsWithChildren) => ( <>{children} @@ -55,12 +68,12 @@ describe("ListView", () => { }; beforeEach(() => { - jest.clearAllMocks(); + vi.clearAllMocks(); mockGetItemComponent.mockImplementation((index: number, item: TestItemWithSeparator, context: any) => { const itemKey = typeof item === "string" ? item : item.id; const isFocused = context.tabIndexKey === itemKey; return ( -
+
{item === SEPARATOR_ITEM ? "---" : (item as TestItem).name}
); @@ -69,24 +82,24 @@ describe("ListView", () => { }); afterEach(() => { - jest.restoreAllMocks(); + vi.restoreAllMocks(); }); describe("Rendering", () => { - it("should render the ListView component", () => { - renderListViewWithHeight(); - expect(screen.getByRole("grid")).toBeInTheDocument(); + it("should render the VirtualizedList component", () => { + renderListWithHeight(); + expect(screen.getByRole("grid")).toBeDefined(); }); it("should render with empty items array", () => { - renderListViewWithHeight({ items: [] }); - expect(screen.getByRole("grid")).toBeInTheDocument(); + renderListWithHeight({ items: [] }); + expect(screen.getByRole("grid")).toBeDefined(); }); }); describe("Keyboard Navigation", () => { it("should handle ArrowDown key navigation", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); fireEvent.focus(container); @@ -94,13 +107,13 @@ describe("ListView", () => { // ArrowDown should skip the non-focusable item at index 1 and go to index 2 const items = container.querySelectorAll(".mx_item"); - expect(items[2]).toHaveAttribute("tabindex", "0"); - expect(items[0]).toHaveAttribute("tabindex", "-1"); - expect(items[1]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[2], "0"); + expectTabIndex(items[0], "-1"); + expectTabIndex(items[1], "-1"); }); it("should handle ArrowUp key navigation", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // First focus and navigate down to second item @@ -112,12 +125,12 @@ describe("ListView", () => { // Verify focus moved back to first item const items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); - expect(items[1]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[0], "0"); + expectTabIndex(items[1], "-1"); }); it("should handle Home key navigation", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // First focus and navigate to a later item @@ -130,15 +143,15 @@ describe("ListView", () => { // Verify focus moved to first item const items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); + expectTabIndex(items[0], "0"); // Check that other items are not focused for (let i = 1; i < items.length; i++) { - expect(items[i]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[i], "-1"); } }); it("should handle End key navigation", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // First focus on the list (starts at first item) @@ -151,15 +164,15 @@ describe("ListView", () => { const items = container.querySelectorAll(".mx_item"); // Should focus on the last visible item const lastIndex = items.length - 1; - expect(items[lastIndex]).toHaveAttribute("tabindex", "0"); + expectTabIndex(items[lastIndex], "0"); // Check that other items are not focused for (let i = 0; i < lastIndex; i++) { - expect(items[i]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[i], "-1"); } }); it("should handle PageDown key navigation", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // First focus on the list (starts at first item) @@ -172,12 +185,12 @@ describe("ListView", () => { const items = container.querySelectorAll(".mx_item"); // PageDown should move to the last visible item since we only have 4 items const lastIndex = items.length - 1; - expect(items[lastIndex]).toHaveAttribute("tabindex", "0"); - expect(items[0]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[lastIndex], "0"); + expectTabIndex(items[0], "-1"); }); it("should handle PageUp key navigation", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // First focus and navigate to last item to have something to page up from @@ -190,56 +203,56 @@ describe("ListView", () => { // Verify focus moved up const items = container.querySelectorAll(".mx_item"); // PageUp should move back to the first item since we only have 4 items - expect(items[0]).toHaveAttribute("tabindex", "0"); + expectTabIndex(items[0], "0"); const lastIndex = items.length - 1; - expect(items[lastIndex]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[lastIndex], "-1"); }); it("should not handle keyboard navigation when modifier keys are pressed", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); fireEvent.focus(container); // Store initial state - first item should be focused const initialItems = container.querySelectorAll(".mx_item"); - expect(initialItems[0]).toHaveAttribute("tabindex", "0"); - expect(initialItems[2]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(initialItems[0], "0"); + expectTabIndex(initialItems[2], "-1"); // Test ArrowDown with Ctrl modifier - should NOT navigate fireEvent.keyDown(container, { code: "ArrowDown", ctrlKey: true }); let items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); // Should still be on first item - expect(items[2]).toHaveAttribute("tabindex", "-1"); // Should not have moved to third item + expectTabIndex(items[0], "0"); // Should still be on first item + expectTabIndex(items[2], "-1"); // Should not have moved to third item // Test ArrowDown with Alt modifier - should NOT navigate fireEvent.keyDown(container, { code: "ArrowDown", altKey: true }); items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); // Should still be on first item - expect(items[2]).toHaveAttribute("tabindex", "-1"); // Should not have moved to third item + expectTabIndex(items[0], "0"); // Should still be on first item + expectTabIndex(items[2], "-1"); // Should not have moved to third item // Test ArrowDown with Shift modifier - should NOT navigate fireEvent.keyDown(container, { code: "ArrowDown", shiftKey: true }); items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); // Should still be on first item - expect(items[2]).toHaveAttribute("tabindex", "-1"); // Should not have moved to third item + expectTabIndex(items[0], "0"); // Should still be on first item + expectTabIndex(items[2], "-1"); // Should not have moved to third item // Test ArrowDown with Meta/Cmd modifier - should NOT navigate fireEvent.keyDown(container, { code: "ArrowDown", metaKey: true }); items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); // Should still be on first item - expect(items[2]).toHaveAttribute("tabindex", "-1"); // Should not have moved to third item + expectTabIndex(items[0], "0"); // Should still be on first item + expectTabIndex(items[2], "-1"); // Should not have moved to third item // Test normal ArrowDown without modifiers - SHOULD navigate fireEvent.keyDown(container, { code: "ArrowDown" }); items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "-1"); // Should have moved from first item - expect(items[2]).toHaveAttribute("tabindex", "0"); // Should have moved to third item (skipping separator) + expectTabIndex(items[0], "-1"); // Should have moved from first item + expectTabIndex(items[2], "0"); // Should have moved to third item (skipping separator) }); it("should skip non-focusable items when navigating down", async () => { @@ -257,7 +270,7 @@ describe("ListView", () => { return (item as TestItem).isFocusable !== false; }); - renderListViewWithHeight({ items: mixedItems }); + renderListWithHeight({ items: mixedItems }); const container = screen.getByRole("grid"); fireEvent.focus(container); @@ -266,9 +279,9 @@ describe("ListView", () => { // Verify it skipped the non-focusable item at index 1 // and went directly to the focusable item at index 2 const items = container.querySelectorAll(".mx_item"); - expect(items[2]).toHaveAttribute("tabindex", "0"); // Item 3 is focused - expect(items[0]).toHaveAttribute("tabindex", "-1"); // Item 1 is not focused - expect(items[1]).toHaveAttribute("tabindex", "-1"); // Item 2 (non-focusable) is not focused + expectTabIndex(items[2], "0"); // Item 3 is focused + expectTabIndex(items[0], "-1"); // Item 1 is not focused + expectTabIndex(items[1], "-1"); // Item 2 (non-focusable) is not focused }); it("should skip non-focusable items when navigating up", () => { @@ -284,7 +297,7 @@ describe("ListView", () => { return (item as TestItem).isFocusable !== false; }); - renderListViewWithHeight({ items: mixedItems }); + renderListWithHeight({ items: mixedItems }); const container = screen.getByRole("grid"); // Focus and go to last item first, then navigate up @@ -295,14 +308,14 @@ describe("ListView", () => { // Verify it skipped non-focusable items // and went to the first focusable item const items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); // Item 1 is focused - expect(items[3]).toHaveAttribute("tabindex", "-1"); // Item 3 is not focused anymore + expectTabIndex(items[0], "0"); // Item 1 is focused + expectTabIndex(items[3], "-1"); // Item 3 is not focused anymore }); }); describe("Focus Management", () => { it("should focus first item when list gains focus for the first time", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // Initial focus should go to first item @@ -310,15 +323,15 @@ describe("ListView", () => { // Verify first item gets focus const items = container.querySelectorAll(".mx_item"); - expect(items[0]).toHaveAttribute("tabindex", "0"); + expectTabIndex(items[0], "0"); // Other items should not be focused for (let i = 1; i < items.length; i++) { - expect(items[i]).toHaveAttribute("tabindex", "-1"); + expectTabIndex(items[i], "-1"); } }); it("should restore last focused item when regaining focus", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // Focus and navigate to simulate previous usage @@ -327,7 +340,7 @@ describe("ListView", () => { // Verify item 2 is focused let items = container.querySelectorAll(".mx_item"); - expect(items[2]).toHaveAttribute("tabindex", "0"); // ArrowDown skips to item 2 + expectTabIndex(items[2], "0"); // ArrowDown skips to item 2 // Simulate blur by focusing elsewhere fireEvent.blur(container); @@ -337,11 +350,11 @@ describe("ListView", () => { // Verify focus is restored to the previously focused item items = container.querySelectorAll(".mx_item"); - expect(items[2]).toHaveAttribute("tabindex", "0"); // Should still be item 2 + expectTabIndex(items[2], "0"); // Should still be item 2 }); it("should not interfere with focus if item is already focused", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); // Focus once @@ -350,7 +363,7 @@ describe("ListView", () => { // Focus again when already focused fireEvent.focus(container); - expect(container).toBeInTheDocument(); + expect(container).toBeDefined(); }); it("should not scroll to top when clicking an item after manual scroll", () => { @@ -360,7 +373,7 @@ describe("ListView", () => { name: `Item ${i}`, })); - const mockOnClick = jest.fn(); + const mockOnClick = vi.fn(); mockGetItemComponent.mockImplementation( ( @@ -376,7 +389,13 @@ describe("ListView", () => { className="mx_item" data-testid={`row-${index}`} tabIndex={isFocused ? 0 : -1} + role="button" onClick={() => mockOnClick(item)} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + mockOnClick(item); + } + }} onFocus={(e) => onFocus(item, e)} > {item === SEPARATOR_ITEM ? "---" : (item as TestItem).name} @@ -385,7 +404,7 @@ describe("ListView", () => { }, ); - const { container } = renderListViewWithHeight({ items: largerItems }); + const { container } = renderListWithHeight({ items: largerItems }); const listContainer = screen.getByRole("grid"); // Step 1: Focus the list initially (this sets tabIndexKey to first item: "item-0") @@ -393,8 +412,8 @@ describe("ListView", () => { // 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"); + expectTabIndex(items[0], "0"); + expectAttribute(items[0], "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 @@ -426,7 +445,7 @@ describe("ListView", () => { // 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"); + expectTabIndex(clickTargetItem, "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) @@ -437,18 +456,18 @@ describe("ListView", () => { describe("Accessibility", () => { it("should set correct ARIA attributes", () => { - renderListViewWithHeight(); + renderListWithHeight(); const container = screen.getByRole("grid"); - expect(container).toHaveAttribute("role", "grid"); - expect(container).toHaveAttribute("aria-rowcount", "4"); - expect(container).toHaveAttribute("aria-colcount", "1"); + expectAttribute(container, "role", "grid"); + expectAttribute(container, "aria-rowcount", "4"); + expectAttribute(container, "aria-colcount", "1"); }); it("should update aria-rowcount when items change", () => { - const { rerender } = renderListViewWithHeight(); + const { rerender } = renderListWithHeight(); let container = screen.getByRole("grid"); - expect(container).toHaveAttribute("aria-rowcount", "4"); + expectAttribute(container, "aria-rowcount", "4"); // Update with fewer items const fewerItems = [ @@ -456,21 +475,21 @@ describe("ListView", () => { { id: "2", name: "Item 2" }, ]; rerender( - getListViewComponent({ + getListComponent({ ...defaultProps, items: fewerItems, }), ); container = screen.getByRole("grid"); - expect(container).toHaveAttribute("aria-rowcount", "2"); + expectAttribute(container, "aria-rowcount", "2"); }); it("should handle custom ARIA label", () => { - renderListViewWithHeight({ "aria-label": "Custom list label" }); + renderListWithHeight({ "aria-label": "Custom list label" }); const container = screen.getByRole("grid"); - expect(container).toHaveAttribute("aria-label", "Custom list label"); + expectAttribute(container, "aria-label", "Custom list label"); }); }); }); diff --git a/src/components/utils/ListView.tsx b/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.tsx similarity index 90% rename from src/components/utils/ListView.tsx rename to packages/shared-components/src/utils/VirtualizedList/VirtualizedList.tsx index 36b33186c9..20e191ba38 100644 --- a/src/components/utils/ListView.tsx +++ b/packages/shared-components/src/utils/VirtualizedList/VirtualizedList.tsx @@ -8,12 +8,32 @@ Please see LICENSE files in the repository root for full details. import React, { useRef, type JSX, useCallback, useEffect, useState, useMemo } from "react"; import { type VirtuosoHandle, type ListRange, Virtuoso, type VirtuosoProps } from "react-virtuoso"; -import { isModifiedKeyEvent, Key } from "../../Keyboard"; +/** + * Keyboard key codes + */ +export const Key = { + ARROW_UP: "ArrowUp", + ARROW_DOWN: "ArrowDown", + HOME: "Home", + END: "End", + PAGE_UP: "PageUp", + PAGE_DOWN: "PageDown", + ENTER: "Enter", + SPACE: "Space", +} as const; + +/** + * Check if a keyboard event includes modifier keys + */ +export function isModifiedKeyEvent(event: React.KeyboardEvent): boolean { + return event.ctrlKey || event.metaKey || event.shiftKey || event.altKey; +} + /** * Context object passed to each list item containing the currently focused key * and any additional context data from the parent component. */ -export type ListContext = { +export type VirtualizedListContext = { /** The key of item that should have tabIndex == 0 */ tabIndexKey?: string; /** Whether an item in the list is currently focused */ @@ -22,8 +42,8 @@ export type ListContext = { context: Context; }; -export interface IListViewProps extends Omit< - VirtuosoProps>, +export interface IVirtualizedListProps extends Omit< + VirtuosoProps>, "data" | "itemContent" | "context" > { /** @@ -43,13 +63,13 @@ export interface IListViewProps extends Omit< getItemComponent: ( index: number, item: Item, - context: ListContext, + context: VirtualizedListContext, onFocus: (item: Item, e: React.FocusEvent) => void, ) => JSX.Element; /** * Optional additional context data to pass to each rendered item. - * This will be available in the ListContext passed to getItemComponent. + * This will be available in the VirtualizedListContext passed to getItemComponent. */ context?: Context; @@ -66,9 +86,10 @@ export interface IListViewProps extends Omit< * @return The key to use for focusing the item */ getItemKey: (item: Item) => string; + /** * Callback function to handle key down events on the list container. - * ListView handles keyboard navigation for focus(up, down, home, end, pageUp, pageDown) + * List handles keyboard navigation for focus(up, down, home, end, pageUp, pageDown) * and stops propagation otherwise the event bubbles and this callback is called for the use of the parent. * @param e - The keyboard event * @returns @@ -80,7 +101,7 @@ export interface IListViewProps extends Omit< * Utility type for the prop scrollIntoViewOnChange allowing it to be memoised by a caller without repeating types */ export type ScrollIntoViewOnChange = NonNullable< - VirtuosoProps>["scrollIntoViewOnChange"] + VirtuosoProps>["scrollIntoViewOnChange"] >; /** @@ -90,7 +111,7 @@ export type ScrollIntoViewOnChange = NonNullable< * @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 function VirtualizedList(props: IVirtualizedListProps): React.ReactElement { // Extract our custom props to avoid conflicts with Virtuoso props const { items, getItemComponent, isItemFocusable, getItemKey, context, onKeyDown, ...virtuosoProps } = props; /** Reference to the Virtuoso component for programmatic scrolling */ @@ -213,11 +234,11 @@ export function ListView(props: IListViewProps(props: IListViewProps { // 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. + // and stop propagation so the List's onFocus doesn't also handle it. const key = getItemKey(item); setIsFocused(true); setTabIndexKey(key); @@ -256,10 +277,11 @@ export function ListView(props: IListViewProps): JSX.Element => + (index: number, item: Item, context: VirtualizedListContext): JSX.Element => getItemComponent(index, item, context, onFocusForGetItemComponent), [getItemComponent, onFocusForGetItemComponent], ); + /** * Handles focus events on the list. * Sets the focused state and scrolls to the focused item if it is not currently visible. @@ -293,7 +315,7 @@ export function ListView(props: IListViewProps = useMemo( + const listContext: VirtualizedListContext = useMemo( () => ({ tabIndexKey: tabIndexKey, focused: isFocused, diff --git a/packages/shared-components/src/utils/VirtualizedList/index.ts b/packages/shared-components/src/utils/VirtualizedList/index.ts new file mode 100644 index 0000000000..72476c231a --- /dev/null +++ b/packages/shared-components/src/utils/VirtualizedList/index.ts @@ -0,0 +1,13 @@ +/* + * Copyright 2026 Element Creations 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. + */ + +export { VirtualizedList } from "./VirtualizedList"; +export type { IVirtualizedListProps, VirtualizedListContext, ScrollIntoViewOnChange } from "./VirtualizedList"; + +// Re-export VirtuosoMockContext for testing purposes +// Tests should import this from shared-components to ensure context compatibility +export { VirtuosoMockContext } from "react-virtuoso"; diff --git a/packages/shared-components/vite.config.ts b/packages/shared-components/vite.config.ts index 3ca56f848a..7bb922b0f2 100644 --- a/packages/shared-components/vite.config.ts +++ b/packages/shared-components/vite.config.ts @@ -25,7 +25,13 @@ export default defineConfig({ rollupOptions: { // make sure to externalize deps that shouldn't be bundled // into your library - external: ["react", "react-dom", "@vector-im/compound-design-tokens", "@vector-im/compound-web"], + external: [ + "react", + "react-dom", + "@vector-im/compound-design-tokens", + "@vector-im/compound-web", + "react-virtuoso", + ], output: { // Provide global variables to use in the UMD build // for externalized deps diff --git a/packages/shared-components/yarn.lock b/packages/shared-components/yarn.lock index 7b919379d0..2634dfd9d6 100644 --- a/packages/shared-components/yarn.lock +++ b/packages/shared-components/yarn.lock @@ -5634,6 +5634,11 @@ react-style-singleton@^2.2.2, react-style-singleton@^2.2.3: get-nonce "^1.0.0" tslib "^2.0.0" +react-virtuoso@^4.14.0: + version "4.18.1" + resolved "https://registry.yarnpkg.com/react-virtuoso/-/react-virtuoso-4.18.1.tgz#3eb7078f2739a31b96c723374019e587deeb6ebc" + integrity sha512-KF474cDwaSb9+SJ380xruBB4P+yGWcVkcu26HtMqYNMTYlYbrNy8vqMkE+GpAApPPufJqgOLMoWMFG/3pJMXUA== + "react@^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0": version "19.2.3" resolved "https://registry.yarnpkg.com/react/-/react-19.2.3.tgz#d83e5e8e7a258cf6b4fe28640515f99b87cd19b8" diff --git a/src/components/views/rooms/MemberList/MemberListView.tsx b/src/components/views/rooms/MemberList/MemberListView.tsx index 298a9ae19c..3a78d271d9 100644 --- a/src/components/views/rooms/MemberList/MemberListView.tsx +++ b/src/components/views/rooms/MemberList/MemberListView.tsx @@ -7,7 +7,7 @@ Please see LICENSE files in the repository root for full details. import { Form } from "@vector-im/compound-web"; import React, { type JSX, useCallback } from "react"; -import { Flex } from "@element-hq/web-shared-components"; +import { Flex, type VirtualizedListContext, VirtualizedList } from "@element-hq/web-shared-components"; import { type MemberWithSeparator, @@ -19,7 +19,6 @@ import { ThreePidInviteTileView } from "./tiles/ThreePidInviteTileView"; import { MemberListHeaderView } from "./MemberListHeaderView"; import BaseCard from "../../right_panel/BaseCard"; import { _t } from "../../../../languageHandler"; -import { type ListContext, ListView } from "../../../utils/ListView"; interface IProps { roomId: string; @@ -54,7 +53,7 @@ const MemberListView: React.FC = (props: IProps) => { ( index: number, item: MemberWithSeparator, - context: ListContext, + context: VirtualizedListContext, onFocus: (item: MemberWithSeparator, e: React.FocusEvent) => void, ): JSX.Element => { const itemKey = getItemKey(item); @@ -109,7 +108,7 @@ const MemberListView: React.FC = (props: IProps) => { e.preventDefault()}> - , + context: VirtualizedListContext, onFocus: (item: Room, e: React.FocusEvent) => void, ): JSX.Element => { const itemKey = item.roomId; @@ -118,7 +122,7 @@ export function RoomList({ vm: { roomsResult, activeIndex } }: RoomListProps): J ); return ( -