Skip to content

Fix duplicate context menu on right-click in room list item menus #30892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix duplicate context menu on right-click in room list item menus #30892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 28 additions & 11 deletions src/components/views/rooms/RoomListPanel/RoomListItemMenuView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,19 +72,26 @@ function MoreOptionsMenu({ vm, setMenuOpen }: MoreOptionsMenuProps): JSX.Element
const [open, setOpen] = useState(false);

return (
<Menu
open={open}
onOpenChange={(isOpen) => {
setOpen(isOpen);
setMenuOpen(isOpen);
<div
onContextMenu={(e) => {
// Prevent opening duplicate more options menu via right-click
if (open) e.stopPropagation();
}}
title={_t("room_list|room|more_options")}
showTitle={false}
align="start"
trigger={<MoreOptionsButton size="24px" />}
>
<MoreOptionContent vm={vm} />
</Menu>
<Menu
open={open}
onOpenChange={(isOpen) => {
setOpen(isOpen);
setMenuOpen(isOpen);
}}
title={_t("room_list|room|more_options")}
showTitle={false}
align="start"
trigger={<MoreOptionsButton size="24px" />}
>
<MoreOptionContent vm={vm} />
</Menu>
</div>
);
}

Expand Down Expand Up @@ -202,6 +209,16 @@ function NotificationMenu({ vm, setMenuOpen }: NotificationMenuProps): JSX.Eleme
<div
// We don't want keyboard navigation events to bubble up to the ListView changing the focused item
onKeyDown={(e) => e.stopPropagation()}
onContextMenu={() => {
if (open) {
// Close notification menu and allow context menu to open via event bubbling
// setTimeout ensures the menu closes after the current event completes bubbling
setTimeout(() => {
setOpen(false);
setMenuOpen(false);
}, 0);
}
}}
>
<Menu
open={open}
Expand Down
39 changes: 39 additions & 0 deletions test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemMenuView-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,43 @@ describe("<RoomListItemMenuView />", () => {
await user.click(screen.getByRole("menuitem", { name: "Mute room" }));
expect(defaultValue.setRoomNotifState).toHaveBeenCalledWith(RoomNotifState.Mute);
});

it("should prevent context menu event bubbling when right-clicking on open more options menu", async () => {
const user = userEvent.setup();
const { container } = renderMenu();

await user.click(screen.getByRole("button", { name: "More Options" }));
await screen.findByRole("menuitem", { name: "Mark as read" });

const contextMenuHandler = jest.fn();
container.addEventListener("contextmenu", contextMenuHandler);

const openMenu = screen.getByRole("menu", { name: "More Options" });
const menuWrapper = openMenu.parentElement;
const contextMenuEvent = new MouseEvent("contextmenu", { bubbles: true, cancelable: true });
menuWrapper?.dispatchEvent(contextMenuEvent);

expect(contextMenuHandler).not.toHaveBeenCalled();
container.removeEventListener("contextmenu", contextMenuHandler);
});

it("should close notification menu when right-clicking on open notification menu", async () => {
const user = userEvent.setup();
const setMenuOpen = jest.fn();
renderMenu(setMenuOpen);

await user.click(screen.getByRole("button", { name: "Notification options" }));
await screen.findByRole("menuitem", { name: "Match default settings" });

setMenuOpen.mockClear();

const openNotificationMenu = screen.getByRole("menu", { name: "Notification options" });
const notificationMenuWrapper = openNotificationMenu.parentElement; // This should be the div with onContextMenu
const contextMenuEvent = new MouseEvent("contextmenu", { bubbles: true, cancelable: true });
notificationMenuWrapper?.dispatchEvent(contextMenuEvent);

await new Promise((resolve) => setTimeout(resolve, 10));

expect(setMenuOpen).toHaveBeenCalledWith(false);
});
});
27 changes: 27 additions & 0 deletions test/unit-tests/components/views/rooms/RoomListPanel/RoomListItemView-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,4 +215,31 @@ describe("<RoomListItemView />", () => {
// But the room item itself should still be rendered
expect(button).toBeInTheDocument();
});

test("should hide hover menu when context menu opens", async () => {
const user = userEvent.setup();

mocked(useRoomListItemViewModel).mockReturnValue({
...defaultValue,
showContextMenu: true,
showHoverMenu: true,
});

renderRoomListItem();

const button = screen.getByRole("option", { name: `Open room ${room.name}` });

// First hover to show hover menu
await user.hover(button);
await waitFor(() => expect(screen.getByRole("button", { name: "More Options" })).toBeInTheDocument());

// Then right-click to open context menu
await user.pointer([{ target: button }, { keys: "[MouseRight]", target: button }]);

// Context menu should be open
await waitFor(() => expect(screen.getByRole("menu")).toBeInTheDocument());

// Hover menu (More Options button) should be hidden when context menu is open
expect(screen.queryByRole("button", { name: "More Options" })).toBeNull();
});
});
1 change: 1 addition & 0 deletions test/unit-tests/components/views/rooms/RoomListPanel/__snapshots__/RoomList-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ exports[`<RoomList /> should render a room list 1`] = `
<DocumentFragment>
<div
aria-label="Room list"
context="[object Object]"
data-testid="room-list"
data-virtuoso-scroller="true"
role="listbox"
Expand Down
124 changes: 64 additions & 60 deletions ...sts/components/views/rooms/RoomListPanel/__snapshots__/RoomListItemMenuView-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,40 @@ exports[`<RoomListItemMenuView /> should render the more options menu 1`] = `
class="flex mx_RoomListItemMenuView"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: start; --mx-flex-gap: var(--cpd-space-1x); --mx-flex-wrap: nowrap;"
>
<button
aria-disabled="false"
aria-expanded="false"
aria-haspopup="menu"
aria-label="More Options"
aria-labelledby="«r2»"
class="_icon-button_1pz9o_8"
data-kind="primary"
data-state="closed"
id="radix-«r0»"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
type="button"
>
<div
class="_indicator-icon_zr2a0_17"
style="--cpd-icon-button-size: 100%;"
<div>
<button
aria-disabled="false"
aria-expanded="false"
aria-haspopup="menu"
aria-label="More Options"
aria-labelledby="«r2»"
class="_icon-button_1pz9o_8"
data-kind="primary"
data-state="closed"
id="radix-«r0»"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
type="button"
>
<svg
fill="currentColor"
height="1em"
viewBox="0 0 24 24"
width="1em"
xmlns="http://www.w3.org/2000/svg"
<div
class="_indicator-icon_zr2a0_17"
style="--cpd-icon-button-size: 100%;"
>
<path
d="M6 14q-.824 0-1.412-.588A1.93 1.93 0 0 1 4 12q0-.825.588-1.412A1.93 1.93 0 0 1 6 10q.824 0 1.412.588Q8 11.175 8 12t-.588 1.412A1.93 1.93 0 0 1 6 14m6 0q-.825 0-1.412-.588A1.93 1.93 0 0 1 10 12q0-.825.588-1.412A1.93 1.93 0 0 1 12 10q.825 0 1.412.588Q14 11.175 14 12t-.588 1.412A1.93 1.93 0 0 1 12 14m6 0q-.824 0-1.413-.588A1.93 1.93 0 0 1 16 12q0-.825.587-1.412A1.93 1.93 0 0 1 18 10q.824 0 1.413.588Q20 11.175 20 12t-.587 1.412A1.93 1.93 0 0 1 18 14"
/>
</svg>
</div>
</button>
<svg
fill="currentColor"
height="1em"
viewBox="0 0 24 24"
width="1em"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M6 14q-.824 0-1.412-.588A1.93 1.93 0 0 1 4 12q0-.825.588-1.412A1.93 1.93 0 0 1 6 10q.824 0 1.412.588Q8 11.175 8 12t-.588 1.412A1.93 1.93 0 0 1 6 14m6 0q-.825 0-1.412-.588A1.93 1.93 0 0 1 10 12q0-.825.588-1.412A1.93 1.93 0 0 1 12 10q.825 0 1.412.588Q14 11.175 14 12t-.588 1.412A1.93 1.93 0 0 1 12 14m6 0q-.824 0-1.413-.588A1.93 1.93 0 0 1 16 12q0-.825.587-1.412A1.93 1.93 0 0 1 18 10q.824 0 1.413.588Q20 11.175 20 12t-.587 1.412A1.93 1.93 0 0 1 18 14"
/>
</svg>
</div>
</button>
</div>
<div>
<button
aria-disabled="false"
Expand Down Expand Up @@ -85,38 +87,40 @@ exports[`<RoomListItemMenuView /> should render the notification options menu 1`
class="flex mx_RoomListItemMenuView"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: start; --mx-flex-gap: var(--cpd-space-1x); --mx-flex-wrap: nowrap;"
>
<button
aria-disabled="false"
aria-expanded="false"
aria-haspopup="menu"
aria-label="More Options"
aria-labelledby="«ri»"
class="_icon-button_1pz9o_8"
data-kind="primary"
data-state="closed"
id="radix-«rg»"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
type="button"
>
<div
class="_indicator-icon_zr2a0_17"
style="--cpd-icon-button-size: 100%;"
<div>
<button
aria-disabled="false"
aria-expanded="false"
aria-haspopup="menu"
aria-label="More Options"
aria-labelledby="«ri»"
class="_icon-button_1pz9o_8"
data-kind="primary"
data-state="closed"
id="radix-«rg»"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
type="button"
>
<svg
fill="currentColor"
height="1em"
viewBox="0 0 24 24"
width="1em"
xmlns="http://www.w3.org/2000/svg"
<div
class="_indicator-icon_zr2a0_17"
style="--cpd-icon-button-size: 100%;"
>
<path
d="M6 14q-.824 0-1.412-.588A1.93 1.93 0 0 1 4 12q0-.825.588-1.412A1.93 1.93 0 0 1 6 10q.824 0 1.412.588Q8 11.175 8 12t-.588 1.412A1.93 1.93 0 0 1 6 14m6 0q-.825 0-1.412-.588A1.93 1.93 0 0 1 10 12q0-.825.588-1.412A1.93 1.93 0 0 1 12 10q.825 0 1.412.588Q14 11.175 14 12t-.588 1.412A1.93 1.93 0 0 1 12 14m6 0q-.824 0-1.413-.588A1.93 1.93 0 0 1 16 12q0-.825.587-1.412A1.93 1.93 0 0 1 18 10q.824 0 1.413.588Q20 11.175 20 12t-.587 1.412A1.93 1.93 0 0 1 18 14"
/>
</svg>
</div>
</button>
<svg
fill="currentColor"
height="1em"
viewBox="0 0 24 24"
width="1em"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M6 14q-.824 0-1.412-.588A1.93 1.93 0 0 1 4 12q0-.825.588-1.412A1.93 1.93 0 0 1 6 10q.824 0 1.412.588Q8 11.175 8 12t-.588 1.412A1.93 1.93 0 0 1 6 14m6 0q-.825 0-1.412-.588A1.93 1.93 0 0 1 10 12q0-.825.588-1.412A1.93 1.93 0 0 1 12 10q.825 0 1.412.588Q14 11.175 14 12t-.588 1.412A1.93 1.93 0 0 1 12 14m6 0q-.824 0-1.413-.588A1.93 1.93 0 0 1 16 12q0-.825.587-1.412A1.93 1.93 0 0 1 18 10q.824 0 1.413.588Q20 11.175 20 12t-.587 1.412A1.93 1.93 0 0 1 18 14"
/>
</svg>
</div>
</button>
</div>
<div>
<button
aria-disabled="false"
Expand Down
Loading