Change avatar setting component to use a menu

playwright/e2e/settings/general-user-settings-tab.spec.ts

@@ -45,14 +45,6 @@ test.describe("General user settings tab", () => {
         // Assert that a userId is rendered
         expect(uut.getByLabel("Username")).toHaveText(user.userId);
 
-        // Check avatar setting
-        const avatar = profile.locator(".mx_AvatarSetting_avatar");
-        await avatar.hover();
-
-        // Hover effect
-        await expect(avatar.locator(".mx_AvatarSetting_hoverBg")).toBeVisible();
-        await expect(avatar.locator(".mx_AvatarSetting_hover span").getByText("Upload")).toBeVisible();
-
         // Wait until spinners disappear
         await expect(uut.getByTestId("accountSection").locator(".mx_Spinner")).not.toBeVisible();
         await expect(uut.getByTestId("discoverySection").locator(".mx_Spinner")).not.toBeVisible();
@@ -128,21 +120,20 @@ test.describe("General user settings tab", () => {
         await expect(uut).toMatchScreenshot("general-smallscreen.png");
     });
 
-    test("should support adding and removing a profile picture", async ({ uut }) => {
+    test("should support adding and removing a profile picture", async ({ uut, page }) => {
         const profileSettings = uut.locator(".mx_UserProfileSettings");
         // Upload a picture
         await profileSettings.getByAltText("Upload").setInputFiles("playwright/sample-files/riot.png");
 
-        // Find and click "Remove" link button
-        await profileSettings
-            .locator(".mx_UserProfileSettings_profile")
-            .getByRole("button", { name: "Remove" })
-            .click();
+        // Image should be visible
+        await expect(profileSettings.locator(".mx_AvatarSetting_avatar img")).toBeVisible();
 
-        // Assert that the link button disappeared
-        await expect(
-            profileSettings.locator(".mx_AvatarSetting_avatar .mx_AccessibleButton_kind_link_sm"),
-        ).not.toBeVisible();
+        // Open the menu & click remove
+        await profileSettings.getByRole("button", { name: "Profile Picture" }).click();
+        await page.getByRole("menuitem", { name: "Remove" }).click();
+
+        // Assert that the image disappeared
+        await expect(profileSettings.locator(".mx_AvatarSetting_avatar img")).not.toBeVisible();
     });
 
     test("should set a country calling code based on default_country_code", async ({ uut }) => {

res/css/views/settings/_AvatarSetting.pcss

@@ -21,36 +21,6 @@ limitations under the License.
     margin-top: 8px;
     position: relative;
 
-    .mx_AvatarSetting_hover {
-        transition: opacity var(--hover-transition);
-        opacity: 0;
-
-        /* position to place the hover bg over the entire thing */
-        position: absolute;
-        inset: 0;
-
-        pointer-events: none; /* let the pointer fall through the underlying thing */
-
-        line-height: 90px;
-        text-align: center;
-
-        > span {
-            color: $primary-content;
-            position: relative; /* tricks the layout engine into putting this on top of the bg */
-            font-weight: 500;
-        }
-
-        .mx_AvatarSetting_hoverBg {
-            /* absolute position to lazily fill the entire container */
-            position: absolute;
-            inset: 0;
-
-            opacity: 0.5;
-            background-color: $quinary-content;
-            border-radius: 90px;
-        }
-    }
-
     &.mx_AvatarSetting_avatarDisplay:hover .mx_AvatarSetting_hover {
         opacity: 1;
     }
@@ -77,25 +47,26 @@ limitations under the License.
     }
 
     .mx_AvatarSetting_uploadButton {
-        width: 32px;
-        height: 32px;
+        width: 28px;
+        height: 28px;
         border-radius: 32px;
-        background-color: $secondary-content;
+        border: 1px solid var(--cpd-color-bg-canvas-default);
+        background-color: var(--cpd-color-bg-subtle-primary);
 
         position: absolute;
         bottom: 0;
         right: 0;
-    }
+        text-align: center;
+        cursor: pointer;
 
-    .mx_AvatarSetting_uploadButton::before {
-        content: "";
-        display: block;
-        width: 100%;
-        height: 100%;
-        mask-repeat: no-repeat;
-        mask-position: center;
-        mask-size: 55%;
-        background-color: $quinary-content;
-        mask-image: url("$(res)/img/feather-customised/edit.svg");
+        svg {
+            position: relative;
+            top: 3px;
+        }
     }
 }
+
+.mx_AvatarSetting_removeMenuItem svg,
+.mx_AvatarSetting_removeMenuItem span {
+    color: var(--cpd-color-text-critical-primary) !important;
+}

src/components/views/room_settings/RoomProfileSettings.tsx

@@ -218,6 +218,10 @@ export default class RoomProfileSettings extends React.Component<IProps, IState>
             );
         }
 
+        const canRemove = this.state.profileFieldsTouched.avatar
+            ? Boolean(this.state.avatarFile)
+            : Boolean(this.state.originalAvatarUrl);
+
         return (
             <form onSubmit={this.saveProfile} autoComplete="off" noValidate={true} className="mx_RoomProfileSettings">
                 <div className="mx_RoomProfileSettings_profile">
@@ -254,9 +258,9 @@ export default class RoomProfileSettings extends React.Component<IProps, IState>
                         avatarAltText={_t("room_settings|general|avatar_field_label")}
                         disabled={!this.state.canSetAvatar}
                         onChange={this.onAvatarChanged}
-                        removeAvatar={this.removeAvatar}
+                        removeAvatar={canRemove ? this.removeAvatar : undefined}
                         placeholderId={idNameForRoom(MatrixClientPeg.safeGet().getRoom(this.props.roomId)!)}
-                        placeholderName={this.state.displayName}
+                        placeholderName={MatrixClientPeg.safeGet().getRoom(this.props.roomId)!.name}
                     />
                 </div>
                 {profileSettingsButtons}

src/components/views/settings/AvatarSetting.tsx

@@ -14,15 +14,59 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import React, { createRef, useCallback, useEffect, useState } from "react";
+import React, { ReactNode, createRef, useCallback, useEffect, useState } from "react";
+import { Icon as EditIcon } from "@vector-im/compound-design-tokens/icons/edit.svg";
+import { Icon as UploadIcon } from "@vector-im/compound-design-tokens/icons/share.svg";
+import { Icon as DeleteIcon } from "@vector-im/compound-design-tokens/icons/delete.svg";
+import { Menu, MenuItem } from "@vector-im/compound-web";
 
 import { _t } from "../../../languageHandler";
-import AccessibleButton from "../elements/AccessibleButton";
 import { mediaFromMxc } from "../../../customisations/Media";
 import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds";
 import { useId } from "../../../utils/useId";
+import AccessibleButton from "../elements/AccessibleButton";
 import BaseAvatar from "../avatars/BaseAvatar";
 
+interface MenuProps {
+    trigger: ReactNode;
+    onUploadSelect: () => void;
+    onRemoveSelect?: () => void;
+}
+
+const AvatarSettingContextMenu: React.FC<MenuProps> = ({ trigger, onUploadSelect, onRemoveSelect }) => {
+    const [menuOpen, setMenuOpen] = useState(false);
+
+    const onOpenChange = useCallback((newOpen: boolean) => {
+        setMenuOpen(newOpen);
+    }, []);
+
+    return (
+        <Menu
+            trigger={trigger}
+            title={_t("action|set_avatar")}
+            showTitle={false}
+            open={menuOpen}
+            onOpenChange={onOpenChange}
+        >
+            <MenuItem
+                as="div"
+                Icon={<UploadIcon width="24px" height="24px" />}
+                label={_t("action|upload_file")}
+                onSelect={onUploadSelect}
+            />
+            {onRemoveSelect && (
+                <MenuItem
+                    as="div"
+                    Icon={<DeleteIcon width="24px" height="24px" />}
+                    className="mx_AvatarSetting_removeMenuItem"
+                    label={_t("action|remove")}
+                    onSelect={onRemoveSelect}
+                />
+            )}
+        </Menu>
+    );
+};
+
 interface IProps {
     /**
      * The current value of the avatar URL, as an mxc URL or a File.
@@ -136,48 +180,39 @@ const AvatarSetting: React.FC<IProps> = ({
     }
 
     let uploadAvatarBtn: JSX.Element | undefined;
-    if (uploadAvatar) {
-        // insert an empty div to be the host for a css mask containing the upload.svg
+    if (!disabled) {
         uploadAvatarBtn = (
-            <>
-                <AccessibleButton
-                    onClick={uploadAvatar}
-                    className="mx_AvatarSetting_uploadButton"
-                    aria-labelledby={a11yId}
-                />
-                <input
-                    type="file"
-                    style={{ display: "none" }}
-                    ref={fileInputRef}
-                    onClick={chromeFileInputFix}
-                    onChange={onFileChanged}
-                    accept="image/*"
-                    alt={_t("action|upload")}
-                />
-            </>
-        );
-    }
-
-    let removeAvatarBtn: JSX.Element | undefined;
-    if (avatarURL && removeAvatar && !disabled) {
-        removeAvatarBtn = (
-            <AccessibleButton onClick={removeAvatar} kind="link_sm">
-                {_t("action|remove")}
-            </AccessibleButton>
+            <div className="mx_AvatarSetting_uploadButton">
+                <EditIcon width="20px" height="20px" />
+            </div>
         );
     }
 
-    return (
+    const content = (
         <div className="mx_AvatarSetting_avatar" role="group" aria-label={avatarAltText}>
             {avatarElement}
-            <div className="mx_AvatarSetting_hover" aria-hidden="true">
-                <div className="mx_AvatarSetting_hoverBg" />
-                {!disabled && <span id={a11yId}>{_t("action|upload")}</span>}
-            </div>
             {uploadAvatarBtn}
-            {removeAvatarBtn}
         </div>
     );
+
+    if (disabled) {
+        return content;
+    }
+
+    return (
+        <>
+            <AvatarSettingContextMenu trigger={content} onUploadSelect={uploadAvatar} onRemoveSelect={removeAvatar} />
+            <input
+                type="file"
+                style={{ display: "none" }}
+                ref={fileInputRef}
+                onClick={chromeFileInputFix}
+                onChange={onFileChanged}
+                accept="image/*"
+                alt={_t("action|upload")}
+            />
+        </>
+    );
 };
 
 export default AvatarSetting;

src/components/views/settings/UserProfileSettings.tsx

@@ -151,7 +151,7 @@ const UserProfileSettings: React.FC = () => {
                     avatar={avatarURL ?? undefined}
                     avatarAltText={_t("common|user_avatar")}
                     onChange={onAvatarChange}
-                    removeAvatar={onAvatarRemove}
+                    removeAvatar={avatarURL ? onAvatarRemove : undefined}
                     placeholderName={displayName}
                     placeholderId={client.getUserId() ?? ""}
                 />

src/i18n/strings/en_EN.json

@@ -109,6 +109,7 @@
         "save": "Save",
         "search": "Search",
         "send_report": "Send report",
+        "set_avatar": "Set profile picture",
         "share": "Share",
         "show": "Show",
         "show_advanced": "Show advanced",
@@ -132,6 +133,7 @@
         "update": "Update",
         "upgrade": "Upgrade",
         "upload": "Upload",
+        "upload_file": "Upload file",
         "verify": "Verify",
         "view": "View",
         "view_all": "View all",

test/components/views/room_settings/RoomProfileSettings-test.tsx

@@ -85,7 +85,8 @@ describe("RoomProfileSetting", () => {
 
         render(<RoomProfileSettings roomId="!floob:itty" />);
 
-        await user.click(screen.getByRole("button", { name: "Remove" }));
+        await user.click(screen.getByRole("button", { name: "Room avatar" }));
+        await user.click(screen.getByRole("menuitem", { name: "Remove" }));
         await user.click(screen.getByRole("button", { name: "Save" }));
 
         await waitFor(() =>

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

@@ -44,35 +44,6 @@ describe("<AvatarSetting />", () => {
         expect(imgElement).toBeInTheDocument();
     });
 
-    it("renders avatar with remove button", async () => {
-        const { queryByText } = render(
-            <AvatarSetting
-                avatarAltText="Avatar of Peter Fox"
-                avatar="mxc://example.org/my-avatar"
-                removeAvatar={jest.fn()}
-                placeholderId="blee"
-                placeholderName="boo"
-            />,
-        );
-
-        const removeButton = queryByText("Remove");
-        expect(removeButton).toBeInTheDocument();
-    });
-
-    it("renders avatar without remove button", async () => {
-        const { queryByText } = render(
-            <AvatarSetting
-                placeholderId="blee"
-                placeholderName="boo"
-                disabled={true}
-                avatarAltText="Avatar of Peter Fox"
-            />,
-        );
-
-        const removeButton = queryByText("Remove");
-        expect(removeButton).toBeNull();
-    });
-
     it("renders a file as the avatar when supplied", async () => {
         render(
             <AvatarSetting
@@ -102,9 +73,6 @@ describe("<AvatarSetting />", () => {
             />,
         );
 
-        // not really necessary, but to follow the expected user flow as much as possible
-        await user.click(screen.getByRole("button", { name: "Avatar of Peter Fox" }));
-
         const fileInput = screen.getByAltText("Upload");
         await user.upload(fileInput, AVATAR_FILE);
 

test/components/views/settings/UserProfileSettings-test.tsx

@@ -85,6 +85,7 @@ describe("ProfileSettings", () => {
     });
 
     it("removes avatar", async () => {
+        jest.spyOn(OwnProfileStore.instance, "avatarMxc", "get").mockReturnValue("mxc://example.org/my-avatar");
         renderProfileSettings(toastRack, client);
 
         expect(await screen.findByText("Mocked AvatarSetting")).toBeInTheDocument();