Skip to content

Add aria-describedBy to room list menus #30126

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.

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 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,28 @@ interface RoomListItemMenuViewProps {
* @param isOpen
*/
setMenuOpen: (isOpen: boolean) => void;
/**
* If set, the ID of a DOM element that describes the menu buttons in this view.
* Note that this will be the same for each menu button: it is suggested that it is the
* ID of the room list item that this menu is for, ie. containing the room name as text.
*/
describedById?: string;
}

/**
* A view for the room list item menu.
*/
export function RoomListItemMenuView({ room, setMenuOpen }: RoomListItemMenuViewProps): JSX.Element {
export function RoomListItemMenuView({ room, setMenuOpen, describedById }: RoomListItemMenuViewProps): JSX.Element {
const vm = useRoomListItemMenuViewModel(room);

return (
<Flex className="mx_RoomListItemMenuView" align="center" gap="var(--cpd-space-1x)">
{vm.showMoreOptionsMenu && <MoreOptionsMenu setMenuOpen={setMenuOpen} vm={vm} />}
{vm.showNotificationMenu && <NotificationMenu setMenuOpen={setMenuOpen} vm={vm} />}
{vm.showMoreOptionsMenu && (
<MoreOptionsMenu setMenuOpen={setMenuOpen} vm={vm} describedById={describedById} />
)}
{vm.showNotificationMenu && (
<NotificationMenu setMenuOpen={setMenuOpen} vm={vm} describedById={describedById} />
)}
</Flex>
);
}
Expand All @@ -64,12 +74,18 @@ interface MoreOptionsMenuProps {
* @param isOpen
*/
setMenuOpen: (isOpen: boolean) => void;
/**
* If set, the ID of a DOM element that describes the menu buttons in this view.
* Note that this will be the same for each menu button: it is suggested that it is the
* ID of the room list item that this menu is for, ie. containing the room name as text.
*/
describedById?: string;
}

/**
* The more options menu for the room list item.
*/
function MoreOptionsMenu({ vm, setMenuOpen }: MoreOptionsMenuProps): JSX.Element {
function MoreOptionsMenu({ vm, setMenuOpen, describedById }: MoreOptionsMenuProps): JSX.Element {
const [open, setOpen] = useState(false);

return (
Expand All @@ -82,7 +98,7 @@ function MoreOptionsMenu({ vm, setMenuOpen }: MoreOptionsMenuProps): JSX.Element
title={_t("room_list|room|more_options")}
showTitle={false}
align="start"
trigger={<MoreOptionsButton size="24px" />}
trigger={<MoreOptionsButton size="24px" aria-describedby={describedById} />}
>
{vm.canMarkAsRead && (
<MenuItem
Expand Down Expand Up @@ -174,9 +190,15 @@ interface NotificationMenuProps {
* @param isOpen
*/
setMenuOpen: (isOpen: boolean) => void;
/**
* If set, the ID of a DOM element that describes the menu buttons in this view.
* Note that this will be the same for each menu button: it is suggested that it is the
* ID of the room list item that this menu is for, ie. containing the room name as text.
*/
describedById?: string;
}

function NotificationMenu({ vm, setMenuOpen }: NotificationMenuProps): JSX.Element {
function NotificationMenu({ vm, setMenuOpen, describedById }: NotificationMenuProps): JSX.Element {
const [open, setOpen] = useState(false);

const checkComponent = <CheckIcon width="24px" height="24px" color="var(--cpd-color-icon-primary)" />;
Expand All @@ -191,7 +213,9 @@ function NotificationMenu({ vm, setMenuOpen }: NotificationMenuProps): JSX.Eleme
title={_t("room_list|notification_options")}
showTitle={false}
align="start"
trigger={<NotificationButton isRoomMuted={vm.isNotificationMute} size="24px" />}
trigger={
<NotificationButton isRoomMuted={vm.isNotificationMute} size="24px" aria-describedby={describedById} />
}
>
<MenuItem
aria-selected={vm.isNotificationAllMessage}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* Please see LICENSE files in the repository root for full details.
*/

import React, { type JSX, memo, useCallback, useRef, useState } from "react";
import React, { type JSX, memo, useCallback, useId, useRef, useState } from "react";
import { type Room } from "matrix-js-sdk/src/matrix";
import classNames from "classnames";

Expand Down Expand Up @@ -46,6 +46,7 @@ export const RoomListItemView = memo(function RoomListItemView({
// Using display: none; and then display:flex when hovered in CSS causes the menu to be misaligned
const showHoverDecoration = isMenuOpen || isHover;
const showHoverMenu = showHoverDecoration && vm.showHoverMenu;
const roomNameId = useId();

return (
<button
Expand Down Expand Up @@ -84,14 +85,15 @@ export const RoomListItemView = memo(function RoomListItemView({
>
{/* We truncate the room name when too long. Title here is to show the full name on hover */}
<div className="mx_RoomListItemView_text">
<div className="mx_RoomListItemView_roomName" title={vm.name}>
<div className="mx_RoomListItemView_roomName" title={vm.name} id={roomNameId}>
{vm.name}
</div>
<div className="mx_RoomListItemView_messagePreview">{vm.messagePreview}</div>
</div>
{showHoverMenu ? (
<RoomListItemMenuView
room={room}
describedById={roomNameId}
setMenuOpen={(isOpen) => {
if (isOpen) {
setIsMenuOpen(isOpen);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ describe("<RoomListItemView />", () => {

await user.hover(listItem);
await waitFor(() => expect(screen.getByRole("button", { name: "More Options" })).toBeInTheDocument());

// also make another snapshot in the hover state (mostly to test the aria-describedBy on the buttons
// as there's no easy way to get the effective ARIA description in react testing library, surprisingly)
expect(listItem).toMatchSnapshot();
});

test("should hover decoration if focused", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r0»"
title="room0"
>
room0
Expand Down Expand Up @@ -115,6 +116,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r1»"
title="room1"
>
room1
Expand Down Expand Up @@ -167,6 +169,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r2»"
title="room2"
>
room2
Expand Down Expand Up @@ -219,6 +222,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r3»"
title="room3"
>
room3
Expand Down Expand Up @@ -271,6 +275,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r4»"
title="room4"
>
room4
Expand Down Expand Up @@ -323,6 +328,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r5»"
title="room5"
>
room5
Expand Down Expand Up @@ -375,6 +381,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r6»"
title="room6"
>
room6
Expand Down Expand Up @@ -427,6 +434,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r7»"
title="room7"
>
room7
Expand Down Expand Up @@ -479,6 +487,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r8»"
title="room8"
>
room8
Expand Down Expand Up @@ -531,6 +540,7 @@ exports[`<RoomList /> should render a room list 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r9»"
title="room9"
>
room9
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ exports[`<RoomListItemView /> should be selected if isSelected=true 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«rl»"
title="room1"
>
room1
Expand Down Expand Up @@ -96,6 +97,7 @@ exports[`<RoomListItemView /> should display notification decoration 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«rm»"
title="room1"
>
room1
Expand All @@ -122,6 +124,129 @@ exports[`<RoomListItemView /> should display notification decoration 1`] = `
</DocumentFragment>
`;

exports[`<RoomListItemView /> should hover decoration if hovered 1`] = `
<button
aria-label="Open room room1"
aria-selected="false"
class="mx_RoomListItemView mx_RoomListItemView_hover mx_RoomListItemView_menu_open"
tabindex="-1"
type="button"
>
<div
class="mx_Flex mx_RoomListItemView_container"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: start; --mx-flex-gap: var(--cpd-space-3x); --mx-flex-wrap: nowrap;"
>
<span
aria-label="Avatar"
class="_avatar_1qbcf_8 mx_BaseAvatar"
data-color="3"
data-testid="avatar-img"
data-type="round"
style="--cpd-avatar-size: 32px;"
>
<img
alt=""
class="_image_1qbcf_41"
data-type="round"
height="32px"
loading="lazy"
referrerpolicy="no-referrer"
src="http://this.is.a.url/avatar.url/room.png"
width="32px"
/>
</span>
<div
class="mx_Flex mx_RoomListItemView_content"
style="--mx-flex-display: flex; --mx-flex-direction: row; --mx-flex-align: center; --mx-flex-justify: space-between; --mx-flex-gap: var(--cpd-space-2x); --mx-flex-wrap: nowrap;"
>
<div
class="mx_RoomListItemView_text"
>
<div
class="mx_RoomListItemView_roomName"
id="«r3»"
title="room1"
>
room1
</div>
<div
class="mx_RoomListItemView_messagePreview"
/>
</div>
<div
class="mx_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-describedby="«r3»"
aria-disabled="false"
aria-expanded="false"
aria-haspopup="menu"
aria-label="More Options"
aria-labelledby="«r6»"
class="_icon-button_m2erp_8"
data-state="closed"
id="radix-«r4»"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
type="button"
>
<div
class="_indicator-icon_zr2a0_17"
style="--cpd-icon-button-size: 100%;"
>
<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>
<button
aria-describedby="«r3»"
aria-disabled="false"
aria-expanded="false"
aria-haspopup="menu"
aria-label="Notification options"
aria-labelledby="«rd»"
class="_icon-button_m2erp_8"
data-state="closed"
id="radix-«rb»"
role="button"
style="--cpd-icon-button-size: 24px;"
tabindex="0"
type="button"
>
<div
class="_indicator-icon_zr2a0_17"
style="--cpd-icon-button-size: 100%;"
>
<svg
fill="currentColor"
height="1em"
viewBox="0 0 24 24"
width="1em"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M20.293 17.293c.63.63.184 1.707-.707 1.707H4.414c-.89 0-1.337-1.077-.707-1.707L5 16v-6s0-7 7-7 7 7 7 7v6zM12 22a2 2 0 0 1-2-2h4a2 2 0 0 1-2 2"
/>
</svg>
</div>
</button>
</div>
</div>
</div>
</button>
`;

exports[`<RoomListItemView /> should render a room item 1`] = `
<DocumentFragment>
<button
Expand Down Expand Up @@ -163,6 +288,7 @@ exports[`<RoomListItemView /> should render a room item 1`] = `
>
<div
class="mx_RoomListItemView_roomName"
id="«r0»"
title="room1"
>
room1
Expand Down Expand Up @@ -218,6 +344,7 @@ exports[`<RoomListItemView /> should render a room item with a message preview 1
>
<div
class="mx_RoomListItemView_roomName"
id="«r1»"
title="room1"
>
room1
Expand Down
Loading