Skip to content

Commit 184bd45

Browse files
qmorozovqmorozov
qmorozov
authored and
qmorozov
committed
Fix duplicate context menu on right-click in room list item menus
1 parent c087755 commit 184bd45

File tree

6 files changed

+174
-71
lines changed

6 files changed

+174
-71
lines changed

‎res/css/views/rooms/RoomListPanel/_RoomListItemMenuView.pcss‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,8 @@
1010
fill: var(--cpd-color-icon-primary);
1111
}
1212
}
13+
14+
.mx_RoomListItemMenuView_menuWrapper {
15+
/* Make wrapper div invisible to layout to prevent visual changes */
16+
display: contents;
17+
}

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

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -72,19 +72,27 @@ function MoreOptionsMenu({ vm, setMenuOpen }: MoreOptionsMenuProps): JSX.Element
7272
const [open, setOpen] = useState(false);
7373

7474
return (
75-
<Menu
76-
open={open}
77-
onOpenChange={(isOpen) => {
78-
setOpen(isOpen);
79-
setMenuOpen(isOpen);
75+
<div
76+
className="mx_RoomListItemMenuView_menuWrapper"
77+
onContextMenu={(e) => {
78+
// Prevent opening duplicate more options menu via right-click
79+
if (open) e.stopPropagation();
8080
}}
81-
title={_t("room_list|room|more_options")}
82-
showTitle={false}
83-
align="start"
84-
trigger={<MoreOptionsButton size="24px" />}
8581
>
86-
<MoreOptionContent vm={vm} />
87-
</Menu>
82+
<Menu
83+
open={open}
84+
onOpenChange={(isOpen) => {
85+
setOpen(isOpen);
86+
setMenuOpen(isOpen);
87+
}}
88+
title={_t("room_list|room|more_options")}
89+
showTitle={false}
90+
align="start"
91+
trigger={<MoreOptionsButton size="24px" />}
92+
>
93+
<MoreOptionContent vm={vm} />
94+
</Menu>
95+
</div>
8896
);
8997
}
9098

@@ -200,8 +208,19 @@ function NotificationMenu({ vm, setMenuOpen }: NotificationMenuProps): JSX.Eleme
200208

201209
return (
202210
<div
211+
className="mx_RoomListItemMenuView_menuWrapper"
203212
// We don't want keyboard navigation events to bubble up to the ListView changing the focused item
204213
onKeyDown={(e) => e.stopPropagation()}
214+
onContextMenu={() => {
215+
if (open) {
216+
// Close notification menu and allow context menu to open via event bubbling
217+
// setTimeout ensures the menu closes after the current event completes bubbling
218+
setTimeout(() => {
219+
setOpen(false);
220+
setMenuOpen(false);
221+
}, 0);
222+
}
223+
}}
205224
>
206225
<Menu
207226
open={open}

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,4 +153,43 @@ describe("<RoomListItemMenuView />", () => {
153153
await user.click(screen.getByRole("menuitem", { name: "Mute room" }));
154154
expect(defaultValue.setRoomNotifState).toHaveBeenCalledWith(RoomNotifState.Mute);
155155
});
156+
157+
it("should prevent context menu event bubbling when right-clicking on open more options menu", async () => {
158+
const user = userEvent.setup();
159+
const { container } = renderMenu();
160+
161+
await user.click(screen.getByRole("button", { name: "More Options" }));
162+
await screen.findByRole("menuitem", { name: "Mark as read" });
163+
164+
const contextMenuHandler = jest.fn();
165+
container.addEventListener("contextmenu", contextMenuHandler);
166+
167+
const openMenu = screen.getByRole("menu", { name: "More Options" });
168+
const menuWrapper = openMenu.parentElement;
169+
const contextMenuEvent = new MouseEvent("contextmenu", { bubbles: true, cancelable: true });
170+
menuWrapper?.dispatchEvent(contextMenuEvent);
171+
172+
expect(contextMenuHandler).not.toHaveBeenCalled();
173+
container.removeEventListener("contextmenu", contextMenuHandler);
174+
});
175+
176+
it("should close notification menu when right-clicking on open notification menu", async () => {
177+
const user = userEvent.setup();
178+
const setMenuOpen = jest.fn();
179+
renderMenu(setMenuOpen);
180+
181+
await user.click(screen.getByRole("button", { name: "Notification options" }));
182+
await screen.findByRole("menuitem", { name: "Match default settings" });
183+
184+
setMenuOpen.mockClear();
185+
186+
const openNotificationMenu = screen.getByRole("menu", { name: "Notification options" });
187+
const notificationMenuWrapper = openNotificationMenu.parentElement; // This should be the div with onContextMenu
188+
const contextMenuEvent = new MouseEvent("contextmenu", { bubbles: true, cancelable: true });
189+
notificationMenuWrapper?.dispatchEvent(contextMenuEvent);
190+
191+
await new Promise((resolve) => setTimeout(resolve, 10));
192+
193+
expect(setMenuOpen).toHaveBeenCalledWith(false);
194+
});
156195
});

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,4 +215,31 @@ describe("<RoomListItemView />", () => {
215215
// But the room item itself should still be rendered
216216
expect(button).toBeInTheDocument();
217217
});
218+
219+
test("should hide hover menu when context menu opens", async () => {
220+
const user = userEvent.setup();
221+
222+
mocked(useRoomListItemViewModel).mockReturnValue({
223+
...defaultValue,
224+
showContextMenu: true,
225+
showHoverMenu: true,
226+
});
227+
228+
renderRoomListItem();
229+
230+
const button = screen.getByRole("option", { name: `Open room ${room.name}` });
231+
232+
// First hover to show hover menu
233+
await user.hover(button);
234+
await waitFor(() => expect(screen.getByRole("button", { name: "More Options" })).toBeInTheDocument());
235+
236+
// Then right-click to open context menu
237+
await user.pointer([{ target: button }, { keys: "[MouseRight]", target: button }]);
238+
239+
// Context menu should be open
240+
await waitFor(() => expect(screen.getByRole("menu")).toBeInTheDocument());
241+
242+
// Hover menu (More Options button) should be hidden when context menu is open
243+
expect(screen.queryByRole("button", { name: "More Options" })).toBeNull();
244+
});
218245
});

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ exports[`<RoomList /> should render a room list 1`] = `
44
<DocumentFragment>
55
<div
66
aria-label="Room list"
7+
context="[object Object]"
78
data-testid="room-list"
89
data-virtuoso-scroller="true"
910
role="listbox"

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

Lines changed: 72 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -6,39 +6,45 @@ exports[`<RoomListItemMenuView /> should render the more options menu 1`] = `
66
class="flex mx_RoomListItemMenuView"
77
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;"
88
>
9-
<button
10-
aria-disabled="false"
11-
aria-expanded="false"
12-
aria-haspopup="menu"
13-
aria-label="More Options"
14-
aria-labelledby="«r2»"
15-
class="_icon-button_1pz9o_8"
16-
data-kind="primary"
17-
data-state="closed"
18-
id="radix-«r0»"
19-
role="button"
20-
style="--cpd-icon-button-size: 24px;"
21-
tabindex="0"
22-
type="button"
9+
<div
10+
class="mx_RoomListItemMenuView_menuWrapper"
2311
>
24-
<div
25-
class="_indicator-icon_zr2a0_17"
26-
style="--cpd-icon-button-size: 100%;"
12+
<button
13+
aria-disabled="false"
14+
aria-expanded="false"
15+
aria-haspopup="menu"
16+
aria-label="More Options"
17+
aria-labelledby="«r2»"
18+
class="_icon-button_1pz9o_8"
19+
data-kind="primary"
20+
data-state="closed"
21+
id="radix-«r0»"
22+
role="button"
23+
style="--cpd-icon-button-size: 24px;"
24+
tabindex="0"
25+
type="button"
2726
>
28-
<svg
29-
fill="currentColor"
30-
height="1em"
31-
viewBox="0 0 24 24"
32-
width="1em"
33-
xmlns="http://www.w3.org/2000/svg"
27+
<div
28+
class="_indicator-icon_zr2a0_17"
29+
style="--cpd-icon-button-size: 100%;"
3430
>
35-
<path
36-
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"
37-
/>
38-
</svg>
39-
</div>
40-
</button>
41-
<div>
31+
<svg
32+
fill="currentColor"
33+
height="1em"
34+
viewBox="0 0 24 24"
35+
width="1em"
36+
xmlns="http://www.w3.org/2000/svg"
37+
>
38+
<path
39+
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"
40+
/>
41+
</svg>
42+
</div>
43+
</button>
44+
</div>
45+
<div
46+
class="mx_RoomListItemMenuView_menuWrapper"
47+
>
4248
<button
4349
aria-disabled="false"
4450
aria-expanded="false"
@@ -85,39 +91,45 @@ exports[`<RoomListItemMenuView /> should render the notification options menu 1`
8591
class="flex mx_RoomListItemMenuView"
8692
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;"
8793
>
88-
<button
89-
aria-disabled="false"
90-
aria-expanded="false"
91-
aria-haspopup="menu"
92-
aria-label="More Options"
93-
aria-labelledby="«ri»"
94-
class="_icon-button_1pz9o_8"
95-
data-kind="primary"
96-
data-state="closed"
97-
id="radix-«rg»"
98-
role="button"
99-
style="--cpd-icon-button-size: 24px;"
100-
tabindex="0"
101-
type="button"
94+
<div
95+
class="mx_RoomListItemMenuView_menuWrapper"
10296
>
103-
<div
104-
class="_indicator-icon_zr2a0_17"
105-
style="--cpd-icon-button-size: 100%;"
97+
<button
98+
aria-disabled="false"
99+
aria-expanded="false"
100+
aria-haspopup="menu"
101+
aria-label="More Options"
102+
aria-labelledby="«ri»"
103+
class="_icon-button_1pz9o_8"
104+
data-kind="primary"
105+
data-state="closed"
106+
id="radix-«rg»"
107+
role="button"
108+
style="--cpd-icon-button-size: 24px;"
109+
tabindex="0"
110+
type="button"
106111
>
107-
<svg
108-
fill="currentColor"
109-
height="1em"
110-
viewBox="0 0 24 24"
111-
width="1em"
112-
xmlns="http://www.w3.org/2000/svg"
112+
<div
113+
class="_indicator-icon_zr2a0_17"
114+
style="--cpd-icon-button-size: 100%;"
113115
>
114-
<path
115-
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"
116-
/>
117-
</svg>
118-
</div>
119-
</button>
120-
<div>
116+
<svg
117+
fill="currentColor"
118+
height="1em"
119+
viewBox="0 0 24 24"
120+
width="1em"
121+
xmlns="http://www.w3.org/2000/svg"
122+
>
123+
<path
124+
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"
125+
/>
126+
</svg>
127+
</div>
128+
</button>
129+
</div>
130+
<div
131+
class="mx_RoomListItemMenuView_menuWrapper"
132+
>
121133
<button
122134
aria-disabled="false"
123135
aria-expanded="false"

0 commit comments

Comments
 (0)