Room list: fix room options remaining on room item after mouse leaving (#31414)

* refactor(room list): remove extra component button and `setMenuOpen` callback

* fix(room list): replace js focus and hover handling by CSS to fix focus decoration

Closes https://github.com/element-hq/element-web/issues/31365

* test(room list): update jest test

* refactor(room list): remove irrelevant comment

* test(room list): update screenshots

res/css/views/rooms/RoomListPanel/_RoomListItemView.pcss

@@ -28,6 +28,41 @@
     padding-left: var(--cpd-space-3x);
     font: var(--cpd-font-body-md-regular);
 
+    /*  Hide the menu by default  */
+    .mx_RoomListItemView_menu {
+        display: none;
+    }
+
+    &:hover,
+    &:focus-visible,
+    /* When the context menu is opened  */
+    &[data-state="open"],
+    /*  When the options and notifications menu are opened */
+    &:has(.mx_RoomListItemMenuView > button[data-state="open"]) {
+        background-color: var(--cpd-color-bg-action-secondary-hovered);
+
+        .mx_RoomListItemView_menu {
+            display: flex;
+        }
+
+        &.mx_RoomListItemView_has_menu {
+            /**
+             * The figma uses 16px padding (--cpd-space-4x) but due to https://github.com/element-hq/compound-web/issues/331
+             * the icon size of the menu is 18px instead of 20px with a different internal padding
+             * We need to use 18px to align the icon with the others icons
+             * 18px is not available in compound spacing
+             */
+            .mx_RoomListItemView_content {
+                padding-right: 18px;
+            }
+
+            /*  When the menu is visible, hide the notification decoration to avoid clutter */
+            .mx_RoomListItemView_notificationDecoration {
+                display: none;
+            }
+        }
+    }
+
     .mx_RoomListItemView_content {
         height: 100%;
         flex: 1;
@@ -57,20 +92,6 @@
     }
 }
 
-.mx_RoomListItemView_hover {
-    background-color: var(--cpd-color-bg-action-secondary-hovered);
-}
-
-.mx_RoomListItemView_menu_open .mx_RoomListItemView_content {
-    /**
-     * The figma uses 16px padding (--cpd-space-4x) but due to https://github.com/element-hq/compound-web/issues/331
-     * the icon size of the menu is 18px instead of 20px with a different internal padding
-     * We need to use 18px to align the icon with the others icons
-     * 18px is not available in compound spacing
-     */
-    padding-right: 18px;
-}
-
 .mx_RoomListItemView_selected {
     background-color: var(--cpd-color-bg-action-secondary-pressed);
 }

src/components/views/rooms/RoomListPanel/RoomListItemContextMenuView.tsx

@@ -19,10 +19,6 @@ interface RoomListItemContextMenuViewProps {
      * The room to display the menu for.
      */
     room: Room;
-    /**
-     * Set the menu open state.
-     */
-    setMenuOpen: (isOpen: boolean) => void;
 }
 
 /**
@@ -30,7 +26,6 @@ interface RoomListItemContextMenuViewProps {
  */
 export function RoomListItemContextMenuView({
     room,
-    setMenuOpen,
     children,
 }: PropsWithChildren<RoomListItemContextMenuViewProps>): JSX.Element {
     const vm = useRoomListItemMenuViewModel(room);
@@ -42,7 +37,6 @@ export function RoomListItemContextMenuView({
             // To not mess with the roving tab index of the button
             hasAccessibleAlternative={true}
             trigger={children}
-            onOpenChange={setMenuOpen}
         >
             <MoreOptionContent vm={vm} />
         </ContextMenu>

src/components/views/rooms/RoomListPanel/RoomListItemMenuView.tsx

@@ -5,8 +5,8 @@
  * Please see LICENSE files in the repository root for full details.
  */
 
-import React, { type ComponentProps, type JSX, type Ref, useState } from "react";
+import React, { type JSX, useState } from "react";
-import { IconButton, Menu, MenuItem, Separator, ToggleMenuItem, Tooltip } from "@vector-im/compound-web";
+import { IconButton, Menu, MenuItem, Separator, ToggleMenuItem } from "@vector-im/compound-web";
 import MarkAsReadIcon from "@vector-im/compound-design-tokens/assets/web/icons/mark-as-read";
 import MarkAsUnreadIcon from "@vector-im/compound-design-tokens/assets/web/icons/mark-as-unread";
 import FavouriteIcon from "@vector-im/compound-design-tokens/assets/web/icons/favourite";
@@ -20,6 +20,7 @@ import NotificationOffIcon from "@vector-im/compound-design-tokens/assets/web/ic
 import CheckIcon from "@vector-im/compound-design-tokens/assets/web/icons/check";
 import { type Room } from "matrix-js-sdk/src/matrix";
 import { Flex } from "@element-hq/web-shared-components";
+import classNames from "classnames";
 
 import { _t } from "../../../../languageHandler";
 import {
@@ -29,26 +30,27 @@ import {
 import { RoomNotifState } from "../../../../RoomNotifs";
 
 interface RoomListItemMenuViewProps {
+    /**
+     * Additional class name for the root element.
+     */
+    className?: string;
+
     /**
      * The room to display the menu for.
      */
     room: Room;
-    /**
-     * Set the menu open state.
-     */
-    setMenuOpen: (isOpen: boolean) => void;
 }
 
 /**
  * A view for the room list item menu.
  */
-export function RoomListItemMenuView({ room, setMenuOpen }: RoomListItemMenuViewProps): JSX.Element {
+export function RoomListItemMenuView({ room, className }: RoomListItemMenuViewProps): JSX.Element {
     const vm = useRoomListItemMenuViewModel(room);
 
     return (
-        <Flex className="mx_RoomListItemMenuView" align="center" gap="var(--cpd-space-1x)">
+        <Flex className={classNames("mx_RoomListItemMenuView", className)} align="center" gap="var(--cpd-space-1x)">
-            {vm.showMoreOptionsMenu && <MoreOptionsMenu setMenuOpen={setMenuOpen} vm={vm} />}
+            {vm.showMoreOptionsMenu && <MoreOptionsMenu vm={vm} />}
-            {vm.showNotificationMenu && <NotificationMenu setMenuOpen={setMenuOpen} vm={vm} />}
+            {vm.showNotificationMenu && <NotificationMenu vm={vm} />}
         </Flex>
     );
 }
@@ -58,30 +60,30 @@ interface MoreOptionsMenuProps {
      * The view model state for the menu.
      */
     vm: RoomListItemMenuViewState;
-    /**
-     * Set the menu open state.
-     * @param isOpen
-     */
-    setMenuOpen: (isOpen: boolean) => void;
 }
 
 /**
  * The more options menu for the room list item.
  */
-function MoreOptionsMenu({ vm, setMenuOpen }: MoreOptionsMenuProps): JSX.Element {
+function MoreOptionsMenu({ vm }: MoreOptionsMenuProps): JSX.Element {
     const [open, setOpen] = useState(false);
 
     return (
         <Menu
             open={open}
-            onOpenChange={(isOpen) => {
+            onOpenChange={setOpen}
-                setOpen(isOpen);
-                setMenuOpen(isOpen);
-            }}
             title={_t("room_list|room|more_options")}
             showTitle={false}
             align="start"
-            trigger={<MoreOptionsButton size="24px" />}
+            trigger={
+                <IconButton
+                    tooltip={_t("room_list|room|more_options")}
+                    aria-label={_t("room_list|room|more_options")}
+                    size="24px"
+                >
+                    <OverflowIcon />
+                </IconButton>
+            }
         >
             <MoreOptionContent vm={vm} />
         </Menu>
@@ -164,55 +166,37 @@ export function MoreOptionContent({ vm }: MoreOptionContentProps): JSX.Element {
     );
 }
 
-interface MoreOptionsButtonProps extends ComponentProps<typeof IconButton> {
-    ref?: Ref<HTMLButtonElement>;
-}
-
-/**
- * A button to trigger the more options menu.
- */
-const MoreOptionsButton = function MoreOptionsButton(props: MoreOptionsButtonProps): JSX.Element {
-    return (
-        <Tooltip label={_t("room_list|room|more_options")}>
-            <IconButton aria-label={_t("room_list|room|more_options")} {...props}>
-                <OverflowIcon />
-            </IconButton>
-        </Tooltip>
-    );
-};
-
 interface NotificationMenuProps {
     /**
      * The view model state for the menu.
      */
     vm: RoomListItemMenuViewState;
-    /**
-     * Set the menu open state.
-     * @param isOpen
-     */
-    setMenuOpen: (isOpen: boolean) => void;
 }
 
-function NotificationMenu({ vm, setMenuOpen }: NotificationMenuProps): JSX.Element {
+function NotificationMenu({ vm }: NotificationMenuProps): JSX.Element {
     const [open, setOpen] = useState(false);
-
     const checkComponent = <CheckIcon width="24px" height="24px" color="var(--cpd-color-icon-primary)" />;
 
     return (
-        <div
+        <Menu
-            // We don't want keyboard navigation events to bubble up to the ListView changing the focused item
+            open={open}
-            onKeyDown={(e) => e.stopPropagation()}
+            onOpenChange={setOpen}
+            title={_t("room_list|notification_options")}
+            showTitle={false}
+            align="start"
+            trigger={
+                <IconButton
+                    size="24px"
+                    tooltip={_t("room_list|notification_options")}
+                    aria-label={_t("room_list|notification_options")}
+                >
+                    {vm.isNotificationMute ? <NotificationOffIcon /> : <NotificationIcon />}
+                </IconButton>
+            }
         >
-            <Menu
+            <div
-                open={open}
+                // We don't want keyboard navigation events to bubble up to the ListView changing the focused item
-                onOpenChange={(isOpen) => {
+                onKeyDown={(e) => e.stopPropagation()}
-                    setOpen(isOpen);
-                    setMenuOpen(isOpen);
-                }}
-                title={_t("room_list|notification_options")}
-                showTitle={false}
-                align="start"
-                trigger={<NotificationButton isRoomMuted={vm.isNotificationMute} size="24px" />}
             >
                 <MenuItem
                     aria-selected={vm.isNotificationAllMessage}
@@ -250,32 +234,7 @@ function NotificationMenu({ vm, setMenuOpen }: NotificationMenuProps): JSX.Eleme
                 >
                     {vm.isNotificationMute && checkComponent}
                 </MenuItem>
-            </Menu>
+            </div>
-        </div>
+        </Menu>
     );
 }
-
-interface NotificationButtonProps extends ComponentProps<typeof IconButton> {
-    /**
-     * Whether the room is muted.
-     */
-    isRoomMuted: boolean;
-    ref?: Ref<HTMLButtonElement>;
-}
-
-/**
- * A button to trigger the notification menu.
- */
-const NotificationButton = function MoreOptionsButton({
-    isRoomMuted,
-    ref,
-    ...props
-}: NotificationButtonProps): JSX.Element {
-    return (
-        <Tooltip label={_t("room_list|notification_options")}>
-            <IconButton aria-label={_t("room_list|notification_options")} {...props} ref={ref}>
-                {isRoomMuted ? <NotificationOffIcon /> : <NotificationIcon />}
-            </IconButton>
-        </Tooltip>
-    );
-};

src/components/views/rooms/RoomListPanel/RoomListItemView.tsx

@@ -5,7 +5,7 @@
  * Please see LICENSE files in the repository root for full details.
  */
 
-import React, { type JSX, memo, useCallback, useEffect, useRef, useState } from "react";
+import React, { type JSX, memo, useEffect, useRef } from "react";
 import { type Room } from "matrix-js-sdk/src/matrix";
 import classNames from "classnames";
 import { Flex } from "@element-hq/web-shared-components";
@@ -57,18 +57,6 @@ export const RoomListItemView = memo(function RoomListItemView({
 }: RoomListItemViewProps): JSX.Element {
     const ref = useRef<HTMLButtonElement>(null);
     const vm = useRoomListItemViewModel(room);
-    const [isHover, setHover] = useState(false);
-    const [isMenuOpen, setIsMenuOpen] = useState(false);
-    // The compound menu in RoomListItemMenuView needs to be rendered when the hover menu is shown
-    // Using display: none; and then display:flex when hovered in CSS causes the menu to be misaligned
-    const showHoverDecoration = isMenuOpen || isFocused || isHover;
-    const showHoverMenu = showHoverDecoration && vm.showHoverMenu;
-
-    const closeMenu = useCallback(() => {
-        // To avoid icon blinking when closing the menu, we delay the state update
-        // Also, let the focus move to the menu trigger before closing the menu
-        setTimeout(() => setIsMenuOpen(false), 10);
-    }, []);
 
     useEffect(() => {
         if (isFocused) {
@@ -81,8 +69,7 @@ export const RoomListItemView = memo(function RoomListItemView({
             as="button"
             ref={ref}
             className={classNames("mx_RoomListItemView", {
-                mx_RoomListItemView_hover: showHoverDecoration,
+                mx_RoomListItemView_has_menu: vm.showHoverMenu,
-                mx_RoomListItemView_menu_open: showHoverMenu,
                 mx_RoomListItemView_selected: isSelected,
                 mx_RoomListItemView_bold: vm.isBold,
             })}
@@ -96,8 +83,6 @@ export const RoomListItemView = memo(function RoomListItemView({
             aria-label={vm.a11yLabel}
             onClick={() => vm.openRoom()}
             onFocus={(e: React.FocusEvent<HTMLButtonElement>) => onFocus(room, e)}
-            onMouseOver={() => setHover(true)}
-            onMouseOut={() => setHover(false)}
             tabIndex={isFocused ? 0 : -1}
             {...props}
         >
@@ -119,44 +104,21 @@ export const RoomListItemView = memo(function RoomListItemView({
                         </div>
                     )}
                 </div>
-                {showHoverMenu ? (
+                {vm.showHoverMenu && <RoomListItemMenuView className="mx_RoomListItemView_menu" room={room} />}
-                    <RoomListItemMenuView
+
-                        room={room}
+                {/* aria-hidden because we summarise the unread count/notification status in a11yLabel variable */}
-                        setMenuOpen={(isOpen) => (isOpen ? setIsMenuOpen(true) : closeMenu())}
+                {vm.showNotificationDecoration && (
+                    <NotificationDecoration
+                        className="mx_RoomListItemView_notificationDecoration"
+                        notificationState={vm.notificationState}
+                        aria-hidden={true}
+                        callType={vm.callType}
                     />
-                ) : (
-                    <>
-                        {/* aria-hidden because we summarise the unread count/notification status in a11yLabel variable */}
-                        {vm.showNotificationDecoration && (
-                            <NotificationDecoration
-                                notificationState={vm.notificationState}
-                                aria-hidden={true}
-                                callType={vm.callType}
-                            />
-                        )}
-                    </>
                 )}
             </Flex>
         </Flex>
     );
 
-    // Rendering multiple context menus can causes crashes in radix upstream,
-    // See https://github.com/radix-ui/primitives/issues/2717.
     if (!vm.showContextMenu) return content;
-
+    return <RoomListItemContextMenuView room={room}>{content}</RoomListItemContextMenuView>;
-    return (
-        <RoomListItemContextMenuView
-            room={room}
-            setMenuOpen={(isOpen) => {
-                if (isOpen) {
-                    // To avoid icon blinking when the context menu is re-opened
-                    setTimeout(() => setIsMenuOpen(true), 0);
-                } else {
-                    closeMenu();
-                }
-            }}
-        >
-            {content}
-        </RoomListItemContextMenuView>
-    );
 });

test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemMenuView-test.tsx

@@ -56,8 +56,8 @@ describe("<RoomListItemMenuView />", () => {
         room = mkRoom(matrixClient, "room1");
     });
 
-    function renderMenu(setMenuOpen = jest.fn()) {
+    function renderMenu() {
-        return render(<RoomListItemMenuView room={room} setMenuOpen={setMenuOpen} />);
+        return render(<RoomListItemMenuView room={room} />);
     }
 
     it("should render the more options menu", () => {
@@ -84,18 +84,6 @@ describe("<RoomListItemMenuView />", () => {
         expect(screen.queryByRole("button", { name: "Notification options" })).toBeNull();
     });
 
-    it.each([["More Options"], ["Notification options"]])(
-        "should call setMenuOpen when the menu is opened for %s menu",
-        async (label) => {
-            const user = userEvent.setup();
-            const setMenuOpen = jest.fn();
-            renderMenu(setMenuOpen);
-
-            await user.click(screen.getByRole("button", { name: label }));
-            expect(setMenuOpen).toHaveBeenCalledWith(true);
-        },
-    );
-
     it("should display all the buttons and have the actions linked for the more options menu", async () => {
         const user = userEvent.setup();
         renderMenu();

test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx

@@ -102,41 +102,6 @@ describe("<RoomListItemView />", () => {
         expect(defaultValue.openRoom).toHaveBeenCalled();
     });
 
-    test("should hover decoration if hovered", async () => {
-        mocked(useRoomListItemViewModel).mockReturnValue({ ...defaultValue, showHoverMenu: true });
-
-        const user = userEvent.setup();
-        renderRoomListItem();
-
-        const listItem = screen.getByRole("option", { name: `Open room ${room.name}` });
-        expect(screen.queryByRole("button", { name: "More Options" })).toBeNull();
-
-        await user.hover(listItem);
-        await waitFor(() => expect(screen.getByRole("button", { name: "More Options" })).toBeInTheDocument());
-    });
-
-    test("should hover decoration if focused", async () => {
-        const { rerender } = renderRoomListItem({
-            isFocused: true,
-        });
-
-        const listItem = screen.getByRole("option", { name: `Open room ${room.name}` });
-        expect(listItem).toHaveClass("_flex_4dswl_9 mx_RoomListItemView mx_RoomListItemView_hover");
-
-        rerender(
-            <RoomListItemView
-                room={room}
-                isSelected={false}
-                isFocused={false}
-                onFocus={jest.fn()}
-                roomIndex={0}
-                roomCount={1}
-            />,
-        );
-
-        await waitFor(() => expect(listItem).not.toHaveClass("flex mx_RoomListItemView mx_RoomListItemView_hover"));
-    });
-
     test("should be selected if isSelected=true", async () => {
         const { asFragment } = renderRoomListItem({
             isSelected: true,

test/unit-tests/components/views/rooms/RoomListPanel/__snapshots__/RoomListItemMenuView-test.tsx.snap

@@ -38,43 +38,41 @@ exports[`<RoomListItemMenuView /> should render the more options menu 1`] = `
         </svg>
       </div>
     </button>
-    <div>
+    <button
-      <button
+      aria-disabled="false"
-        aria-disabled="false"
+      aria-expanded="false"
-        aria-expanded="false"
+      aria-haspopup="menu"
-        aria-haspopup="menu"
+      aria-label="Notification options"
-        aria-label="Notification options"
+      aria-labelledby="_r_9_"
-        aria-labelledby="_r_9_"
+      class="_icon-button_1pz9o_8"
-        class="_icon-button_1pz9o_8"
+      data-kind="primary"
-        data-kind="primary"
+      data-state="closed"
-        data-state="closed"
+      id="radix-_r_7_"
-        id="radix-_r_7_"
+      role="button"
-        role="button"
+      style="--cpd-icon-button-size: 24px;"
-        style="--cpd-icon-button-size: 24px;"
+      tabindex="0"
-        tabindex="0"
+      type="button"
-        type="button"
+    >
+      <div
+        class="_indicator-icon_147l5_17"
+        style="--cpd-icon-button-size: 100%;"
       >
-        <div
+        <svg
-          class="_indicator-icon_147l5_17"
+          fill="currentColor"
-          style="--cpd-icon-button-size: 100%;"
+          height="1em"
+          viewBox="0 0 24 24"
+          width="1em"
+          xmlns="http://www.w3.org/2000/svg"
         >
-          <svg
+          <path
-            fill="currentColor"
+            d="m4.917 2.083 17 17a1 1 0 0 1-1.414 1.414L19.006 19H4.414c-.89 0-1.337-1.077-.707-1.707L5 16v-6s0-2.034 1.096-3.91L3.504 3.498a1 1 0 0 1 1.414-1.414M19 13.35 9.136 3.484C9.93 3.181 10.874 3 12 3c7 0 7 7 7 7z"
-            height="1em"
+          />
-            viewBox="0 0 24 24"
+          <path
-            width="1em"
+            d="M10 20h4a2 2 0 0 1-4 0"
-            xmlns="http://www.w3.org/2000/svg"
+          />
-          >
+        </svg>
-            <path
+      </div>
-              d="m4.917 2.083 17 17a1 1 0 0 1-1.414 1.414L19.006 19H4.414c-.89 0-1.337-1.077-.707-1.707L5 16v-6s0-2.034 1.096-3.91L3.504 3.498a1 1 0 0 1 1.414-1.414M19 13.35 9.136 3.484C9.93 3.181 10.874 3 12 3c7 0 7 7 7 7z"
+    </button>
-            />
-            <path
-              d="M10 20h4a2 2 0 0 1-4 0"
-            />
-          </svg>
-        </div>
-      </button>
-    </div>
   </div>
 </DocumentFragment>
 `;
@@ -117,43 +115,41 @@ exports[`<RoomListItemMenuView /> should render the notification options menu 1`
         </svg>
       </div>
     </button>
-    <div>
+    <button
-      <button
+      aria-disabled="false"
-        aria-disabled="false"
+      aria-expanded="false"
-        aria-expanded="false"
+      aria-haspopup="menu"
-        aria-haspopup="menu"
+      aria-label="Notification options"
-        aria-label="Notification options"
+      aria-labelledby="_r_p_"
-        aria-labelledby="_r_p_"
+      class="_icon-button_1pz9o_8"
-        class="_icon-button_1pz9o_8"
+      data-kind="primary"
-        data-kind="primary"
+      data-state="closed"
-        data-state="closed"
+      id="radix-_r_n_"
-        id="radix-_r_n_"
+      role="button"
-        role="button"
+      style="--cpd-icon-button-size: 24px;"
-        style="--cpd-icon-button-size: 24px;"
+      tabindex="0"
-        tabindex="0"
+      type="button"
-        type="button"
+    >
+      <div
+        class="_indicator-icon_147l5_17"
+        style="--cpd-icon-button-size: 100%;"
       >
-        <div
+        <svg
-          class="_indicator-icon_147l5_17"
+          fill="currentColor"
-          style="--cpd-icon-button-size: 100%;"
+          height="1em"
+          viewBox="0 0 24 24"
+          width="1em"
+          xmlns="http://www.w3.org/2000/svg"
         >
-          <svg
+          <path
-            fill="currentColor"
+            d="m4.917 2.083 17 17a1 1 0 0 1-1.414 1.414L19.006 19H4.414c-.89 0-1.337-1.077-.707-1.707L5 16v-6s0-2.034 1.096-3.91L3.504 3.498a1 1 0 0 1 1.414-1.414M19 13.35 9.136 3.484C9.93 3.181 10.874 3 12 3c7 0 7 7 7 7z"
-            height="1em"
+          />
-            viewBox="0 0 24 24"
+          <path
-            width="1em"
+            d="M10 20h4a2 2 0 0 1-4 0"
-            xmlns="http://www.w3.org/2000/svg"
+          />
-          >
+        </svg>
-            <path
+      </div>
-              d="m4.917 2.083 17 17a1 1 0 0 1-1.414 1.414L19.006 19H4.414c-.89 0-1.337-1.077-.707-1.707L5 16v-6s0-2.034 1.096-3.91L3.504 3.498a1 1 0 0 1 1.414-1.414M19 13.35 9.136 3.484C9.93 3.181 10.874 3 12 3c7 0 7 7 7 7z"
+    </button>
-            />
-            <path
-              d="M10 20h4a2 2 0 0 1-4 0"
-            />
-          </svg>
-        </div>
-      </button>
-    </div>
   </div>
 </DocumentFragment>
 `;

test/unit-tests/components/views/rooms/RoomListPanel/__snapshots__/RoomListItemView-test.tsx.snap

@@ -99,7 +99,7 @@ exports[`<RoomListItemView /> should display notification decoration 1`] = `
       </div>
       <div
         aria-hidden="true"
-        class="_flex_4dswl_9"
+        class="_flex_4dswl_9 mx_RoomListItemView_notificationDecoration"
         data-testid="notification-decoration"
         style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: center; --mx-flex-gap: var(--cpd-space-1x); --mx-flex-wrap: nowrap;"
       >