Improve accessibility of the `<AvatarSetting> component (#30907)

* Always use an accessible button with base avatar rendered inside it

* Rename avatarAltText to accessibleName

* Improve accessibility

* Fix tests

Author: MidhunSureshR

src/components/views/room_settings/RoomProfileSettings.tsx

@@ -262,7 +262,7 @@ export default class RoomProfileSettings extends React.Component<IProps, IState>
                                 ? undefined
                                 : (this.state.avatarFile ?? this.state.originalAvatarUrl ?? undefined)
                         }
-                        avatarAltText={_t("room_settings|general|avatar_field_label")}
+                        avatarAccessibleName={_t("room_settings|general|avatar_field_label")}
                         disabled={!this.state.canSetAvatar}
                         onChange={this.onAvatarChanged}
                         removeAvatar={canRemove ? this.removeAvatar : undefined}

src/components/views/settings/AvatarSetting.tsx

@@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
 Please see LICENSE files in the repository root for full details.
 */
 
-import React, { type JSX, type ReactNode, createRef, useCallback, useEffect, useState, useId } from "react";
+import React, { type JSX, type ReactNode, createRef, useCallback, useEffect, useState } from "react";
 import EditIcon from "@vector-im/compound-design-tokens/assets/web/icons/edit";
 import UploadIcon from "@vector-im/compound-design-tokens/assets/web/icons/share";
 import DeleteIcon from "@vector-im/compound-design-tokens/assets/web/icons/delete";
@@ -89,9 +89,9 @@ interface IProps {
     removeAvatar?: () => void;
 
     /**
-     * The alt text for the avatar
+     * The accessible name for the avatar, eg: "Foo's Profile Picture"
      */
-    avatarAltText: string;
+    avatarAccessibleName: string;
 
     /**
      * String to use for computing the colour of the placeholder avatar if no avatar is set
@@ -121,7 +121,7 @@ export function getFileChanged(e: React.ChangeEvent<HTMLInputElement>): File | n
  */
 const AvatarSetting: React.FC<IProps> = ({
     avatar,
-    avatarAltText,
+    avatarAccessibleName,
     onChange,
     removeAvatar,
     disabled,
@@ -147,9 +147,6 @@ const AvatarSetting: React.FC<IProps> = ({
         }
     }, [avatar]);
 
-    // Prevents ID collisions when this component is used more than once on the same page.
-    const a11yId = useId();
-
     const onFileChanged = useCallback(
         (e: React.ChangeEvent<HTMLInputElement>) => {
             const file = getFileChanged(e);
@@ -170,48 +167,47 @@ const AvatarSetting: React.FC<IProps> = ({
         setMenuOpen(newOpen);
     }, []);
 
-    let avatarElement = (
+    const avatarElement = (
         <AccessibleButton
             element="div"
-            onClick={uploadAvatar}
+            /**
+             * This button will open a menu. That is done by passing this element as trigger
+             * to the menu component, hence the empty onClick.
+             */
+            onClick={() => {}}
             className="mx_AvatarSetting_avatarPlaceholder mx_AvatarSetting_avatarDisplay"
-            aria-labelledby={disabled ? undefined : a11yId}
-            // Inhibit tab stop as we have explicit upload/remove buttons
-            tabIndex={-1}
             disabled={disabled}
         >
-            <BaseAvatar idName={placeholderId} name={placeholderName} size="90px" />
+            <BaseAvatar
+                idName={placeholderId}
+                name={placeholderName}
+                size="90px"
+                url={avatarURL}
+                altText={avatarAccessibleName}
+            />
         </AccessibleButton>
     );
-    if (avatarURL) {
-        avatarElement = (
-            <AccessibleButton
-                element="img"
-                className="mx_AvatarSetting_avatarDisplay"
-                src={avatarURL}
-                alt={avatarAltText}
-                onClick={uploadAvatar}
-                // Inhibit tab stop as we have explicit upload/remove buttons
-                tabIndex={-1}
-                disabled={disabled}
-            />
-        );
-    }
 
     let uploadAvatarBtn: JSX.Element | undefined;
     if (!disabled) {
         const uploadButtonClasses = classNames("mx_AvatarSetting_uploadButton", {
             mx_AvatarSetting_uploadButton_active: menuOpen,
         });
         uploadAvatarBtn = (
-            <div className={uploadButtonClasses}>
-                <EditIcon width="20px" height="20px" />
+            <div
+                className={uploadButtonClasses}
+                role="button"
+                aria-label={_t("settings|general|avatar_open_menu")}
+                tabIndex={0}
+                aria-haspopup="menu"
+            >
+                <EditIcon aria-hidden={true} width="20px" height="20px" />
             </div>
         );
     }
 
     const content = (
-        <div className="mx_AvatarSetting_avatar" role="group" aria-label={avatarAltText}>
+        <div className="mx_AvatarSetting_avatar" role="group" aria-label={avatarAccessibleName}>
             {avatarElement}
             {uploadAvatarBtn}
         </div>

src/components/views/settings/UserProfileSettings.tsx

@@ -203,7 +203,7 @@ const UserProfileSettings: React.FC<UserProfileSettingsProps> = ({
             <div className="mx_UserProfileSettings_profile">
                 <AvatarSetting
                     avatar={avatarURL ?? undefined}
-                    avatarAltText={_t("common|user_avatar")}
+                    avatarAccessibleName={_t("common|user_avatar")}
                     onChange={onAvatarChange}
                     removeAvatar={avatarURL ? onAvatarRemove : undefined}
                     placeholderName={displayName}

src/i18n/strings/en_EN.json

@@ -2664,6 +2664,7 @@
             "allow_spellcheck": "Allow spell check",
             "application_language": "Application language",
             "application_language_reload_hint": "The app will reload after selecting another language",
+            "avatar_open_menu": "Open avatar menu",
             "avatar_remove_progress": "Removing image...",
             "avatar_save_progress": "Uploading image...",
             "avatar_upload_error_text": "The file format is not supported or the image is larger than %(size)s.",

test/unit-tests/components/views/settings/AvatarSetting-test.tsx

@@ -25,31 +25,32 @@ describe("<AvatarSetting />", () => {
         stubClient();
     });
 
-    it("renders avatar with specified alt text", async () => {
-        const { queryByAltText } = render(
+    it("renders avatar with specified accessible name", async () => {
+        const { getByRole } = render(
             <AvatarSetting
                 placeholderId="blee"
                 placeholderName="boo"
-                avatarAltText="Avatar of Peter Fox"
+                avatarAccessibleName="Avatar of Peter Fox"
                 avatar="mxc://example.org/my-avatar"
             />,
         );
 
-        const imgElement = queryByAltText("Avatar of Peter Fox");
-        expect(imgElement).toBeInTheDocument();
+        const avatarButton = getByRole("button", { name: "Avatar of Peter Fox" });
+        expect(avatarButton).toBeInTheDocument();
     });
 
     it("renders a file as the avatar when supplied", async () => {
         render(
             <AvatarSetting
                 placeholderId="blee"
                 placeholderName="boo"
-                avatarAltText="Avatar of Peter Fox"
+                avatarAccessibleName="Avatar of Peter Fox"
                 avatar={AVATAR_FILE}
             />,
         );
 
-        const imgElement = await screen.findByRole("button", { name: "Avatar of Peter Fox" });
+        const avatarButton = await screen.findByRole("button", { name: "Avatar of Peter Fox" });
+        const imgElement = avatarButton.querySelector("img");
         expect(imgElement).toBeInTheDocument();
         expect(imgElement).toHaveAttribute("src", "data:image/gif;base64," + BASE64_GIF);
     });
@@ -63,7 +64,7 @@ describe("<AvatarSetting />", () => {
                 placeholderId="blee"
                 placeholderName="boo"
                 avatar="mxc://example.org/my-avatar"
-                avatarAltText="Avatar of Peter Fox"
+                avatarAccessibleName="Avatar of Peter Fox"
                 onChange={onChange}
             />,
         );
@@ -82,7 +83,7 @@ describe("<AvatarSetting />", () => {
                 placeholderId="blee"
                 placeholderName="boo"
                 avatar="mxc://example.org/my-avatar"
-                avatarAltText="Avatar of Peter Fox"
+                avatarAccessibleName="Avatar of Peter Fox"
                 onChange={onChange}
             />,
         );
@@ -102,7 +103,7 @@ describe("<AvatarSetting />", () => {
                 placeholderId="blee"
                 placeholderName="boo"
                 avatar="mxc://example.org/my-avatar"
-                avatarAltText="Avatar of Peter Fox"
+                avatarAccessibleName="Avatar of Peter Fox"
                 onChange={onChange}
             />,
         );