From 1d3e326e6a7b82abc46f1ea58ca7eeeb7f394bda Mon Sep 17 00:00:00 2001 From: Clark Fischer Date: Wed, 25 Jan 2023 08:40:01 -0800 Subject: [PATCH 1/5] Strict typechecking fixes for Base/Member/Avatar Update the core avatar files to pass `--strict --noImplicitAny` typechecks. Signed-off-by: Clark Fischer --- src/Avatar.ts | 31 +++++----- src/components/views/avatars/BaseAvatar.tsx | 56 +++++++++++++------ src/components/views/avatars/MemberAvatar.tsx | 17 +++--- src/components/views/avatars/RoomAvatar.tsx | 3 +- src/editor/parts.ts | 8 +-- test/Avatar-test.ts | 6 +- .../views/avatars/MemberAvatar-test.tsx | 6 +- 7 files changed, 75 insertions(+), 52 deletions(-) diff --git a/src/Avatar.ts b/src/Avatar.ts index 8a3f10a22ca..b23d06a55cf 100644 --- a/src/Avatar.ts +++ b/src/Avatar.ts @@ -1,5 +1,5 @@ /* -Copyright 2015, 2016 OpenMarket Ltd +Copyright 2015, 2016, 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -26,14 +26,15 @@ import { isLocalRoom } from "./utils/localRoom/isLocalRoom"; // Not to be used for BaseAvatar urls as that has similar default avatar fallback already export function avatarUrlForMember( - member: RoomMember, + member: RoomMember | null | undefined, width: number, height: number, resizeMethod: ResizeMethod, ): string { - let url: string; - if (member?.getMxcAvatarUrl()) { - url = mediaFromMxc(member.getMxcAvatarUrl()).getThumbnailOfSourceHttp(width, height, resizeMethod); + let url: string | undefined; + const mxcUrl = member?.getMxcAvatarUrl(); + if (mxcUrl) { + url = mediaFromMxc(mxcUrl).getThumbnailOfSourceHttp(width, height, resizeMethod); } if (!url) { // member can be null here currently since on invites, the JS SDK @@ -86,7 +87,7 @@ function urlForColor(color: string): string { // hard to install a listener here, even if there were a clear event to listen to const colorToDataURLCache = new Map(); -export function defaultAvatarUrlForString(s: string): string { +export function defaultAvatarUrlForString(s: string | undefined): string { if (!s) return ""; // XXX: should never happen but empirically does by evidence of a rageshake const defaultColors = ["#0DBD8B", "#368bd6", "#ac3ba8"]; let total = 0; @@ -97,7 +98,7 @@ export function defaultAvatarUrlForString(s: string): string { // overwritten color value in custom themes const cssVariable = `--avatar-background-colors_${colorIndex}`; const cssValue = document.body.style.getPropertyValue(cssVariable); - const color = cssValue || defaultColors[colorIndex]; + const color = cssValue || defaultColors[colorIndex]!; let dataUrl = colorToDataURLCache.get(color); if (!dataUrl) { // validate color as this can come from account_data @@ -118,7 +119,7 @@ export function defaultAvatarUrlForString(s: string): string { * @param {string} name * @return {string} the first letter */ -export function getInitialLetter(name: string): string { +export function getInitialLetter(name: string): string | undefined { if (!name) { // XXX: We should find out what causes the name to sometimes be falsy. console.trace("`name` argument to `getInitialLetter` not supplied"); @@ -134,19 +135,20 @@ export function getInitialLetter(name: string): string { } // rely on the grapheme cluster splitter in lodash so that we don't break apart compound emojis - return split(name, "", 1)[0].toUpperCase(); + return split(name, "", 1)[0]!.toUpperCase(); } export function avatarUrlForRoom( - room: Room, + room: Room | undefined, width: number, height: number, resizeMethod?: ResizeMethod, ): string | null { if (!room) return null; // null-guard - if (room.getMxcAvatarUrl()) { - return mediaFromMxc(room.getMxcAvatarUrl()).getThumbnailOfSourceHttp(width, height, resizeMethod); + const mxcUrl = room.getMxcAvatarUrl(); + if (mxcUrl) { + return mediaFromMxc(mxcUrl).getThumbnailOfSourceHttp(width, height, resizeMethod); } // space rooms cannot be DMs so skip the rest @@ -159,8 +161,9 @@ export function avatarUrlForRoom( // If there are only two members in the DM use the avatar of the other member const otherMember = room.getAvatarFallbackMember(); - if (otherMember?.getMxcAvatarUrl()) { - return mediaFromMxc(otherMember.getMxcAvatarUrl()).getThumbnailOfSourceHttp(width, height, resizeMethod); + const otherMemberMxc = otherMember?.getMxcAvatarUrl(); + if (otherMemberMxc) { + return mediaFromMxc(otherMemberMxc).getThumbnailOfSourceHttp(width, height, resizeMethod); } return null; } diff --git a/src/components/views/avatars/BaseAvatar.tsx b/src/components/views/avatars/BaseAvatar.tsx index 025cb9d2711..963c867f4d0 100644 --- a/src/components/views/avatars/BaseAvatar.tsx +++ b/src/components/views/avatars/BaseAvatar.tsx @@ -1,8 +1,6 @@ /* -Copyright 2015, 2016 OpenMarket Ltd -Copyright 2018 New Vector Ltd Copyright 2019 Michael Telatynski <7t3chguy@gmail.com> -Copyright 2019, 2020 The Matrix.org Foundation C.I.C. +Copyright 2015, 2016, 2018, 2019, 2020, 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -21,34 +19,41 @@ import React, { useCallback, useContext, useEffect, useState } from "react"; import classNames from "classnames"; import { ResizeMethod } from "matrix-js-sdk/src/@types/partials"; import { ClientEvent } from "matrix-js-sdk/src/client"; +import { SyncState } from "matrix-js-sdk/src/sync"; import * as AvatarLogic from "../../../Avatar"; -import SettingsStore from "../../../settings/SettingsStore"; import AccessibleButton from "../elements/AccessibleButton"; import RoomContext from "../../../contexts/RoomContext"; import MatrixClientContext from "../../../contexts/MatrixClientContext"; import { useTypedEventEmitter } from "../../../hooks/useEventEmitter"; import { toPx } from "../../../utils/units"; import { _t } from "../../../languageHandler"; +import AccessibleTooltipButton from "../elements/AccessibleTooltipButton"; interface IProps { - name: string; // The name (first initial used as default) - idName?: string; // ID for generating hash colours - title?: string; // onHover title text - url?: string; // highest priority of them all, shortcut to set in urls[0] - urls?: string[]; // [highest_priority, ... , lowest_priority] + /** The name (first initial used as default) */ + name: string; + /** ID for generating hash colours */ + idName?: string; + /** onHover title text */ + title?: string; + /** highest priority of them all, shortcut to set in urls[0] */ + url?: string; + /** [highest_priority, ... , lowest_priority] */ + urls?: string[]; width?: number; height?: number; - // XXX: resizeMethod not actually used. + /** @deprecated not actually used */ resizeMethod?: ResizeMethod; - defaultToInitialLetter?: boolean; // true to add default url - onClick?: React.MouseEventHandler; + /** true to add default url */ + defaultToInitialLetter?: boolean; + onClick?: React.ComponentPropsWithoutRef["onClick"]; inputRef?: React.RefObject; className?: string; tabIndex?: number; } -const calculateUrls = (url: string, urls: string[], lowBandwidth: boolean): string[] => { +const calculateUrls = (url: string | undefined, urls: string[] | undefined, lowBandwidth: boolean): string[] => { // work out the full set of urls to try to load. This is formed like so: // imageUrls: [ props.url, ...props.urls ] @@ -66,11 +71,26 @@ const calculateUrls = (url: string, urls: string[], lowBandwidth: boolean): stri return Array.from(new Set(_urls)); }; -const useImageUrl = ({ url, urls }): [string, () => void] => { +/** + * Hook for cycling through a changing set of images. + * + * The set of images is updated whenever `url` or `urls` change, the user's + * `lowBandwidth` preference changes, or the client reconnects. + * + * Returns `[imageUrl, onError]`. When `onError` is called, the next image in + * the set will be displayed. + */ +const useImageUrl = ({ + url, + urls, +}: { + url: string | undefined; + urls: string[] | undefined; +}): [string | undefined, () => void] => { // Since this is a hot code path and the settings store can be slow, we // use the cached lowBandwidth value from the room context if it exists const roomContext = useContext(RoomContext); - const lowBandwidth = roomContext ? roomContext.lowBandwidth : SettingsStore.getValue("lowBandwidth"); + const lowBandwidth = roomContext.lowBandwidth; const [imageUrls, setUrls] = useState(calculateUrls(url, urls, lowBandwidth)); const [urlsIndex, setIndex] = useState(0); @@ -85,10 +105,10 @@ const useImageUrl = ({ url, urls }): [string, () => void] => { }, [url, JSON.stringify(urls)]); // eslint-disable-line react-hooks/exhaustive-deps const cli = useContext(MatrixClientContext); - const onClientSync = useCallback((syncState, prevState) => { + const onClientSync = useCallback((syncState: SyncState, prevState: SyncState | null) => { // Consider the client reconnected if there is no error with syncing. // This means the state could be RECONNECTING, SYNCING, PREPARED or CATCHUP. - const reconnected = syncState !== "ERROR" && prevState !== syncState; + const reconnected = syncState !== SyncState.Error && prevState !== syncState; if (reconnected) { setIndex(0); } @@ -108,11 +128,11 @@ const BaseAvatar: React.FC = (props) => { urls, width = 40, height = 40, - resizeMethod = "crop", // eslint-disable-line @typescript-eslint/no-unused-vars defaultToInitialLetter = true, onClick, inputRef, className, + resizeMethod: _unused, // to keep it from being in `otherProps` ...otherProps } = props; diff --git a/src/components/views/avatars/MemberAvatar.tsx b/src/components/views/avatars/MemberAvatar.tsx index 48138714559..b37e65c5d43 100644 --- a/src/components/views/avatars/MemberAvatar.tsx +++ b/src/components/views/avatars/MemberAvatar.tsx @@ -1,6 +1,5 @@ /* -Copyright 2015, 2016 OpenMarket Ltd -Copyright 2019 - 2022 The Matrix.org Foundation C.I.C. +Copyright 2015, 2016, 2019 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -33,14 +32,13 @@ interface IProps extends Omit, "name" | width: number; height: number; resizeMethod?: ResizeMethod; - // The onClick to give the avatar - onClick?: React.MouseEventHandler; - // Whether the onClick of the avatar should be overridden to dispatch `Action.ViewUser` + /** Whether the onClick of the avatar should be overridden to dispatch `Action.ViewUser` */ viewUserOnClick?: boolean; pushUserOnClick?: boolean; title?: string; - style?: any; - forceHistorical?: boolean; // true to deny `useOnlyCurrentProfiles` usage. Default false. + style?: React.CSSProperties; + /** true to deny `useOnlyCurrentProfiles` usage. Default false. */ + forceHistorical?: boolean; hideTitle?: boolean; } @@ -77,8 +75,8 @@ export default function MemberAvatar({ if (!title) { title = - UserIdentifierCustomisations.getDisplayUserIdentifier(member?.userId ?? "", { - roomId: member?.roomId ?? "", + UserIdentifierCustomisations.getDisplayUserIdentifier!(member.userId, { + roomId: member.roomId, }) ?? fallbackUserId; } } @@ -88,7 +86,6 @@ export default function MemberAvatar({ {...props} width={width} height={height} - resizeMethod={resizeMethod} name={name ?? ""} title={hideTitle ? undefined : title} idName={member?.userId ?? fallbackUserId} diff --git a/src/components/views/avatars/RoomAvatar.tsx b/src/components/views/avatars/RoomAvatar.tsx index 50389c77491..4abfdbbf67e 100644 --- a/src/components/views/avatars/RoomAvatar.tsx +++ b/src/components/views/avatars/RoomAvatar.tsx @@ -109,7 +109,8 @@ export default class RoomAvatar extends React.Component { } private onRoomAvatarClick = (): void => { - const avatarUrl = Avatar.avatarUrlForRoom(this.props.room, null, null, null); + const avatarMxc = this.props.room?.getMxcAvatarUrl(); + const avatarUrl = avatarMxc ? mediaFromMxc(avatarMxc).srcHttp : null; const params = { src: avatarUrl, name: this.props.room.name, diff --git a/src/editor/parts.ts b/src/editor/parts.ts index 306d86dbc94..c9b756686f2 100644 --- a/src/editor/parts.ts +++ b/src/editor/parts.ts @@ -295,9 +295,9 @@ export abstract class PillPart extends BasePart implements IPillPart { } // helper method for subclasses - protected setAvatarVars(node: HTMLElement, avatarUrl: string, initialLetter: string): void { + protected setAvatarVars(node: HTMLElement, avatarUrl: string, initialLetter: string | undefined): void { const avatarBackground = `url('${avatarUrl}')`; - const avatarLetter = `'${initialLetter}'`; + const avatarLetter = `'${initialLetter || ""}'`; // check if the value is changing, // otherwise the avatars flicker on every keystroke while updating. if (node.style.getPropertyValue("--avatar-background") !== avatarBackground) { @@ -413,7 +413,7 @@ class RoomPillPart extends PillPart { } protected setAvatar(node: HTMLElement): void { - let initialLetter = ""; + let initialLetter: string | undefined = ""; let avatarUrl = Avatar.avatarUrlForRoom(this.room, 16, 16, "crop"); if (!avatarUrl) { initialLetter = Avatar.getInitialLetter(this.room?.name || this.resourceId); @@ -468,7 +468,7 @@ class UserPillPart extends PillPart { const name = this.member.name || this.member.userId; const defaultAvatarUrl = Avatar.defaultAvatarUrlForString(this.member.userId); const avatarUrl = Avatar.avatarUrlForMember(this.member, 16, 16, "crop"); - let initialLetter = ""; + let initialLetter: string | undefined = ""; if (avatarUrl === defaultAvatarUrl) { initialLetter = Avatar.getInitialLetter(name); } diff --git a/test/Avatar-test.ts b/test/Avatar-test.ts index 0ff064ed57d..4582095b562 100644 --- a/test/Avatar-test.ts +++ b/test/Avatar-test.ts @@ -1,5 +1,5 @@ /* -Copyright 2022 The Matrix.org Foundation C.I.C. +Copyright 2022 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -59,7 +59,7 @@ describe("avatarUrlForRoom", () => { }); it("should return null for a null room", () => { - expect(avatarUrlForRoom(null, 128, 128)).toBeNull(); + expect(avatarUrlForRoom(undefined, 128, 128)).toBeNull(); }); it("should return the HTTP source if the room provides a MXC url", () => { @@ -83,7 +83,7 @@ describe("avatarUrlForRoom", () => { it("should return null if there is no other member in the room", () => { mocked(dmRoomMap).getUserIdForRoomId.mockReturnValue("@user:example.com"); - mocked(room.getAvatarFallbackMember).mockReturnValue(null); + mocked(room.getAvatarFallbackMember).mockReturnValue(undefined); expect(avatarUrlForRoom(room, 128, 128)).toBeNull(); }); diff --git a/test/components/views/avatars/MemberAvatar-test.tsx b/test/components/views/avatars/MemberAvatar-test.tsx index 4895b70f217..bb57c1ae508 100644 --- a/test/components/views/avatars/MemberAvatar-test.tsx +++ b/test/components/views/avatars/MemberAvatar-test.tsx @@ -1,5 +1,5 @@ /* -Copyright 2022 The Matrix.org Foundation C.I.C. +Copyright 2022 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -28,6 +28,8 @@ import SettingsStore from "../../../../src/settings/SettingsStore"; import { getRoomContext } from "../../../test-utils/room"; import { stubClient } from "../../../test-utils/test-utils"; +type Props = React.ComponentPropsWithoutRef; + describe("MemberAvatar", () => { const ROOM_ID = "roomId"; @@ -35,7 +37,7 @@ describe("MemberAvatar", () => { let room: Room; let member: RoomMember; - function getComponent(props) { + function getComponent(props: Partial) { return ( From e3c0e8f2e72192b7ef185348a053fce77f499cb9 Mon Sep 17 00:00:00 2001 From: Clark Fischer Date: Wed, 25 Jan 2023 09:38:28 -0800 Subject: [PATCH 2/5] Add tests for Base/Member/Avatar More thoroughly test the core avatar files. Not necessarily the most thorough, but an improvement. Signed-off-by: Clark Fischer --- src/components/views/avatars/MemberAvatar.tsx | 5 +- src/dispatcher/payloads/ViewUserPayload.ts | 7 + test/Avatar-test.ts | 104 +++++++-- .../views/avatars/BaseAvatar-test.tsx | 201 ++++++++++++++++++ .../views/avatars/MemberAvatar-test.tsx | 89 ++++++-- .../__snapshots__/BaseAvatar-test.tsx.snap | 84 ++++++++ .../__snapshots__/MemberAvatar-test.tsx.snap | 14 ++ 7 files changed, 465 insertions(+), 39 deletions(-) create mode 100644 test/components/views/avatars/BaseAvatar-test.tsx create mode 100644 test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap create mode 100644 test/components/views/avatars/__snapshots__/MemberAvatar-test.tsx.snap diff --git a/src/components/views/avatars/MemberAvatar.tsx b/src/components/views/avatars/MemberAvatar.tsx index b37e65c5d43..f493c58f8cf 100644 --- a/src/components/views/avatars/MemberAvatar.tsx +++ b/src/components/views/avatars/MemberAvatar.tsx @@ -25,6 +25,7 @@ import { mediaFromMxc } from "../../../customisations/Media"; import { CardContext } from "../right_panel/context"; import UserIdentifierCustomisations from "../../../customisations/UserIdentifier"; import { useRoomMemberProfile } from "../../../hooks/room/useRoomMemberProfile"; +import { ViewUserPayload } from "../../../dispatcher/payloads/ViewUserPayload"; interface IProps extends Omit, "name" | "idName" | "url"> { member: RoomMember | null; @@ -93,9 +94,9 @@ export default function MemberAvatar({ onClick={ viewUserOnClick ? () => { - dis.dispatch({ + dis.dispatch({ action: Action.ViewUser, - member: propsMember, + member: propsMember || undefined, push: card.isCard, }); } diff --git a/src/dispatcher/payloads/ViewUserPayload.ts b/src/dispatcher/payloads/ViewUserPayload.ts index 20df21beb49..a09804babee 100644 --- a/src/dispatcher/payloads/ViewUserPayload.ts +++ b/src/dispatcher/payloads/ViewUserPayload.ts @@ -28,4 +28,11 @@ export interface ViewUserPayload extends ActionPayload { * should be shown (hide whichever relevant components). */ member?: RoomMember | User; + + /** + * Should this event be pushed as a card into the right panel? + * + * @see RightPanelStore#pushCard + */ + push?: boolean; } diff --git a/test/Avatar-test.ts b/test/Avatar-test.ts index 4582095b562..3a5b16104fd 100644 --- a/test/Avatar-test.ts +++ b/test/Avatar-test.ts @@ -15,33 +15,95 @@ limitations under the License. */ import { mocked } from "jest-mock"; -import { Room, RoomMember, RoomType } from "matrix-js-sdk/src/matrix"; - -import { avatarUrlForRoom } from "../src/Avatar"; -import { Media, mediaFromMxc } from "../src/customisations/Media"; +import { Room, RoomMember, RoomType, User } from "matrix-js-sdk/src/matrix"; + +import { + avatarUrlForMember, + avatarUrlForRoom, + avatarUrlForUser, + defaultAvatarUrlForString, + getInitialLetter, +} from "../src/Avatar"; +import { mediaFromMxc } from "../src/customisations/Media"; import DMRoomMap from "../src/utils/DMRoomMap"; - -jest.mock("../src/customisations/Media", () => ({ - mediaFromMxc: jest.fn(), -})); +import { filterConsole, stubClient } from "./test-utils"; const roomId = "!room:example.com"; const avatarUrl1 = "https://example.com/avatar1"; const avatarUrl2 = "https://example.com/avatar2"; +describe("avatarUrlForMember", () => { + let member: RoomMember; + + beforeEach(() => { + stubClient(); + member = new RoomMember(roomId, "@user:example.com"); + }); + + it("returns the member's url", () => { + const mxc = "mxc://example.com/a/b/c/d/avatar.gif"; + jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue(mxc); + + expect(avatarUrlForMember(member, 32, 32, "crop")).toBe( + mediaFromMxc(mxc).getThumbnailOfSourceHttp(32, 32, "crop"), + ); + }); + + it("returns a default if the member has no avatar", () => { + jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue(undefined); + + expect(avatarUrlForMember(member, 32, 32, "crop")).toMatch(/^data:/); + }); +}); + +describe("avatarUrlForUser", () => { + let user: User; + + beforeEach(() => { + stubClient(); + user = new User("@user:example.com"); + }); + + it("should return the user's avatar", () => { + const mxc = "mxc://example.com/a/b/c/d/avatar.gif"; + user.avatarUrl = mxc; + + expect(avatarUrlForUser(user, 64, 64, "scale")).toBe( + mediaFromMxc(mxc).getThumbnailOfSourceHttp(64, 64, "scale"), + ); + }); + + it("should not provide a fallback", () => { + expect(avatarUrlForUser(user, 64, 64, "scale")).toBeNull(); + }); +}); + +describe("defaultAvatarUrlForString", () => { + it.each(["a", "abc", "abcde", "@".repeat(150)])("should return a value for %s", (s) => { + expect(defaultAvatarUrlForString(s)).not.toBe(""); + }); +}); + +describe("getInitialLetter", () => { + filterConsole("argument to `getInitialLetter` not supplied"); + + it.each(["a", "abc", "abcde", "@".repeat(150)])("should return a value for %s", (s) => { + expect(getInitialLetter(s)).not.toBe(""); + }); + + it("should return undefined for empty strings", () => { + expect(getInitialLetter("")).toBeUndefined(); + }); +}); + describe("avatarUrlForRoom", () => { - let getThumbnailOfSourceHttp: jest.Mock; let room: Room; let roomMember: RoomMember; let dmRoomMap: DMRoomMap; beforeEach(() => { - getThumbnailOfSourceHttp = jest.fn(); - mocked(mediaFromMxc).mockImplementation((): Media => { - return { - getThumbnailOfSourceHttp, - } as unknown as Media; - }); + stubClient(); + room = { roomId, getMxcAvatarUrl: jest.fn(), @@ -64,9 +126,9 @@ describe("avatarUrlForRoom", () => { it("should return the HTTP source if the room provides a MXC url", () => { mocked(room.getMxcAvatarUrl).mockReturnValue(avatarUrl1); - getThumbnailOfSourceHttp.mockReturnValue(avatarUrl2); - expect(avatarUrlForRoom(room, 128, 256, "crop")).toEqual(avatarUrl2); - expect(getThumbnailOfSourceHttp).toHaveBeenCalledWith(128, 256, "crop"); + expect(avatarUrlForRoom(room, 128, 256, "crop")).toBe( + mediaFromMxc(avatarUrl1).getThumbnailOfSourceHttp(128, 256, "crop"), + ); }); it("should return null for a space room", () => { @@ -97,8 +159,8 @@ describe("avatarUrlForRoom", () => { mocked(dmRoomMap).getUserIdForRoomId.mockReturnValue("@user:example.com"); mocked(room.getAvatarFallbackMember).mockReturnValue(roomMember); mocked(roomMember.getMxcAvatarUrl).mockReturnValue(avatarUrl2); - getThumbnailOfSourceHttp.mockReturnValue(avatarUrl2); - expect(avatarUrlForRoom(room, 128, 256, "crop")).toEqual(avatarUrl2); - expect(getThumbnailOfSourceHttp).toHaveBeenCalledWith(128, 256, "crop"); + expect(avatarUrlForRoom(room, 128, 256, "crop")).toEqual( + mediaFromMxc(avatarUrl2).getThumbnailOfSourceHttp(128, 256, "crop"), + ); }); }); diff --git a/test/components/views/avatars/BaseAvatar-test.tsx b/test/components/views/avatars/BaseAvatar-test.tsx new file mode 100644 index 00000000000..294a64c4362 --- /dev/null +++ b/test/components/views/avatars/BaseAvatar-test.tsx @@ -0,0 +1,201 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { fireEvent, render } from "@testing-library/react"; +import { ClientEvent, PendingEventOrdering } from "matrix-js-sdk/src/client"; +import { Room } from "matrix-js-sdk/src/models/room"; +import { RoomMember } from "matrix-js-sdk/src/models/room-member"; +import React from "react"; +import { act } from "react-dom/test-utils"; +import { SyncState } from "matrix-js-sdk/src/sync"; + +import type { MatrixClient } from "matrix-js-sdk/src/client"; +import RoomContext from "../../../../src/contexts/RoomContext"; +import { getRoomContext } from "../../../test-utils/room"; +import { stubClient } from "../../../test-utils/test-utils"; +import BaseAvatar from "../../../../src/components/views/avatars/BaseAvatar"; +import MatrixClientContext from "../../../../src/contexts/MatrixClientContext"; + +type Props = React.ComponentPropsWithoutRef; + +describe("", () => { + let client: MatrixClient; + let room: Room; + let member: RoomMember; + + function getComponent(props: Partial) { + return ( + + + + + + ); + } + + function failLoadingImg(container: HTMLElement): void { + const img = container.querySelector("img")!; + expect(img).not.toBeNull(); + act(() => { + fireEvent.error(img); + }); + } + + function emitReconnect(): void { + act(() => { + client.emit(ClientEvent.Sync, SyncState.Prepared, SyncState.Reconnecting); + }); + } + + beforeEach(() => { + client = stubClient(); + + room = new Room("!room:example.com", client, client.getUserId() ?? "", { + pendingEventOrdering: PendingEventOrdering.Detached, + }); + + member = new RoomMember(room.roomId, "@bob:example.org"); + jest.spyOn(room, "getMember").mockReturnValue(member); + }); + + it("renders with minimal properties", () => { + const { container } = render(getComponent({})); + + expect(container.querySelector(".mx_BaseAvatar")).not.toBeNull(); + }); + + it("matches snapshot (avatar)", () => { + const { container } = render( + getComponent({ + name: "CoolUser22", + title: "Hover title", + url: "https://example.com/images/avatar.gif", + className: "mx_SomethingArbitrary", + }), + ); + + expect(container).toMatchSnapshot(); + }); + + it("matches snapshot (avatar + click)", () => { + const { container } = render( + getComponent({ + name: "CoolUser22", + title: "Hover title", + url: "https://example.com/images/avatar.gif", + className: "mx_SomethingArbitrary", + onClick: () => {}, + }), + ); + + expect(container).toMatchSnapshot(); + }); + + it("matches snapshot (no avatar)", () => { + const { container } = render( + getComponent({ + name: "xX_Element_User_Xx", + title: ":kiss:", + defaultToInitialLetter: true, + className: "big-and-bold", + }), + ); + + expect(container).toMatchSnapshot(); + }); + + it("matches snapshot (no avatar + click)", () => { + const { container } = render( + getComponent({ + name: "xX_Element_User_Xx", + title: ":kiss:", + defaultToInitialLetter: true, + className: "big-and-bold", + onClick: () => {}, + }), + ); + + expect(container).toMatchSnapshot(); + }); + + it("uses fallback images", () => { + const images = [...Array(10)].map((_, i) => `https://example.com/images/${i}.webp`); + + const { container } = render( + getComponent({ + url: images[0], + urls: images.slice(1), + }), + ); + + for (const image of images) { + expect(container.querySelector("img")!.src).toBe(image); + failLoadingImg(container); + } + }); + + it("re-renders on reconnect", () => { + const primary = "https://example.com/image.jpeg"; + const fallback = "https://example.com/fallback.png"; + const { container } = render( + getComponent({ + url: primary, + urls: [fallback], + }), + ); + + failLoadingImg(container); + expect(container.querySelector("img")!.src).toBe(fallback); + + emitReconnect(); + expect(container.querySelector("img")!.src).toBe(primary); + }); + + it("renders with an image", () => { + const url = "https://example.com/images/small/avatar.gif?size=realBig"; + const { container } = render(getComponent({ url })); + + const img = container.querySelector("img"); + expect(img!.src).toBe(url); + }); + + it("renders the initial letter", () => { + const { container } = render(getComponent({ name: "Yellow", defaultToInitialLetter: true })); + + const avatar = container.querySelector(".mx_BaseAvatar_initial")!; + expect(avatar.innerHTML).toBe("Y"); + }); + + it.each([{}, { name: "CoolUser22" }, { name: "XxElement_FanxX", defaultToInitialLetter: true }])( + "includes a click handler", + (props: Partial) => { + const onClick = jest.fn(); + + const { container } = render( + getComponent({ + ...props, + onClick, + }), + ); + + act(() => { + fireEvent.click(container.querySelector(".mx_BaseAvatar")!); + }); + + expect(onClick).toHaveBeenCalled(); + }, + ); +}); diff --git a/test/components/views/avatars/MemberAvatar-test.tsx b/test/components/views/avatars/MemberAvatar-test.tsx index bb57c1ae508..dc282c8adc9 100644 --- a/test/components/views/avatars/MemberAvatar-test.tsx +++ b/test/components/views/avatars/MemberAvatar-test.tsx @@ -14,19 +14,23 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { getByTestId, render, waitFor } from "@testing-library/react"; -import { mocked } from "jest-mock"; +import { fireEvent, getByTestId, render } from "@testing-library/react"; import { MatrixClient, PendingEventOrdering } from "matrix-js-sdk/src/client"; import { Room } from "matrix-js-sdk/src/models/room"; import { RoomMember } from "matrix-js-sdk/src/models/room-member"; import React from "react"; +import { act } from "react-dom/test-utils"; import MemberAvatar from "../../../../src/components/views/avatars/MemberAvatar"; import RoomContext from "../../../../src/contexts/RoomContext"; -import { MatrixClientPeg } from "../../../../src/MatrixClientPeg"; +import { mediaFromMxc } from "../../../../src/customisations/Media"; +import { ViewUserPayload } from "../../../../src/dispatcher/payloads/ViewUserPayload"; +import defaultDispatcher from "../../../../src/dispatcher/dispatcher"; +import { SettingLevel } from "../../../../src/settings/SettingLevel"; import SettingsStore from "../../../../src/settings/SettingsStore"; import { getRoomContext } from "../../../test-utils/room"; import { stubClient } from "../../../test-utils/test-utils"; +import { Action } from "../../../../src/dispatcher/actions"; type Props = React.ComponentPropsWithoutRef; @@ -46,10 +50,7 @@ describe("MemberAvatar", () => { } beforeEach(() => { - jest.clearAllMocks(); - - stubClient(); - mockClient = mocked(MatrixClientPeg.get()); + mockClient = stubClient(); room = new Room(ROOM_ID, mockClient, mockClient.getUserId() ?? "", { pendingEventOrdering: PendingEventOrdering.Detached, @@ -57,22 +58,78 @@ describe("MemberAvatar", () => { member = new RoomMember(ROOM_ID, "@bob:example.org"); jest.spyOn(room, "getMember").mockReturnValue(member); + }); + + it("supports 'null' members", () => { + const { container } = render(getComponent({ member: null })); + + expect(container.querySelector("img")).not.toBeNull(); + }); + + it("matches the snapshot", () => { jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue("http://placekitten.com/400/400"); + const { container } = render( + getComponent({ + member, + fallbackUserId: "Fallback User ID", + title: "Hover title", + style: { + color: "pink", + }, + }), + ); + + expect(container).toMatchSnapshot(); }); - it("shows an avatar for useOnlyCurrentProfiles", async () => { - jest.spyOn(SettingsStore, "getValue").mockImplementation((settingName: string) => { - return settingName === "useOnlyCurrentProfiles"; - }); + it("shows an avatar for useOnlyCurrentProfiles", () => { + jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue("http://placekitten.com/400/400"); + + SettingsStore.setValue("useOnlyCurrentProfiles", null, SettingLevel.DEVICE, true); const { container } = render(getComponent({})); - let avatar: HTMLElement; - await waitFor(() => { - avatar = getByTestId(container, "avatar-img"); - expect(avatar).toBeInTheDocument(); + const avatar = getByTestId(container, "avatar-img"); + expect(avatar).toBeInTheDocument(); + expect(avatar.getAttribute("src")).not.toBe(""); + }); + + it("uses the member's configured avatar", () => { + const mxcUrl = "mxc://example.com/avatars/user.tiff"; + jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue(mxcUrl); + + const { container } = render(getComponent({ member })); + + const img = container.querySelector("img"); + expect(img).not.toBeNull(); + expect(img!.src).toBe(mediaFromMxc(mxcUrl).srcHttp); + }); + + it("uses a fallback when the member has no avatar", () => { + jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue(undefined); + + const { container } = render(getComponent({ member })); + + const img = container.querySelector("img"); + expect(img).not.toBeNull(); + expect(img!.src).toMatch(/^data:/); + }); + + it("dispatches on click", () => { + const { container } = render(getComponent({ member, viewUserOnClick: true })); + + const spy = jest.spyOn(defaultDispatcher, "dispatch"); + + act(() => { + fireEvent.click(container.querySelector(".mx_BaseAvatar")!); }); - expect(avatar!.getAttribute("src")).not.toBe(""); + expect(spy).toHaveBeenCalled(); + const [payload] = spy.mock.lastCall!; + expect(payload).toStrictEqual({ + action: Action.ViewUser, + member, + push: false, + }); }); }); diff --git a/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap b/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap new file mode 100644 index 00000000000..38202a82ead --- /dev/null +++ b/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap @@ -0,0 +1,84 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[` matches snapshot (avatar + click) 1`] = ` +
+ Avatar +
+`; + +exports[` matches snapshot (avatar) 1`] = ` +
+ +
+`; + +exports[` matches snapshot (no avatar + click) 1`] = ` +
+ + + + +
+`; + +exports[` matches snapshot (no avatar) 1`] = ` +
+ + + + +
+`; diff --git a/test/components/views/avatars/__snapshots__/MemberAvatar-test.tsx.snap b/test/components/views/avatars/__snapshots__/MemberAvatar-test.tsx.snap new file mode 100644 index 00000000000..feda79035cd --- /dev/null +++ b/test/components/views/avatars/__snapshots__/MemberAvatar-test.tsx.snap @@ -0,0 +1,14 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`MemberAvatar matches the snapshot 1`] = ` +
+ +
+`; From 60efce198ddbe8901d5494cdb7b651e5169684c1 Mon Sep 17 00:00:00 2001 From: Clark Fischer Date: Wed, 25 Jan 2023 13:44:25 -0800 Subject: [PATCH 3/5] Extract TextAvatar from BaseAvatar Extracted the fallback/textual avatar into its own component. Signed-off-by: Clark Fischer --- src/components/views/avatars/BaseAvatar.tsx | 80 ++++++++++++--------- 1 file changed, 47 insertions(+), 33 deletions(-) diff --git a/src/components/views/avatars/BaseAvatar.tsx b/src/components/views/avatars/BaseAvatar.tsx index 963c867f4d0..92c469a0bff 100644 --- a/src/components/views/avatars/BaseAvatar.tsx +++ b/src/components/views/avatars/BaseAvatar.tsx @@ -139,35 +139,7 @@ const BaseAvatar: React.FC = (props) => { const [imageUrl, onError] = useImageUrl({ url, urls }); if (!imageUrl && defaultToInitialLetter && name) { - const initialLetter = AvatarLogic.getInitialLetter(name); - const textNode = ( - - ); - const imgNode = ( - - ); + const avatar = ; if (onClick) { return ( @@ -180,8 +152,7 @@ const BaseAvatar: React.FC = (props) => { onClick={onClick} inputRef={inputRef} > - {textNode} - {imgNode} + {avatar} ); } else { @@ -192,8 +163,7 @@ const BaseAvatar: React.FC = (props) => { {...otherProps} role="presentation" > - {textNode} - {imgNode} + {avatar} ); } @@ -240,3 +210,47 @@ const BaseAvatar: React.FC = (props) => { export default BaseAvatar; export type BaseAvatarType = React.FC; + +const TextAvatar: React.FC<{ + name: string; + idName?: string; + width: number; + height: number; + title?: string; +}> = ({ name, idName, width, height, title }) => { + const initialLetter = AvatarLogic.getInitialLetter(name); + const textNode = ( + + ); + const imgNode = ( + + ); + + return ( + <> + {textNode} + {imgNode} + + ); +}; From b085248e786bc3621131d2a8f1ea419cdecc1b56 Mon Sep 17 00:00:00 2001 From: Clark Fischer Date: Wed, 25 Jan 2023 13:44:48 -0800 Subject: [PATCH 4/5] Use standard HTML for non-image avatars Firefox users with `resistFingerprinting` enabled were seeing random noise for rooms and users without avatars. There's no real reason to use data URLs to present flat colors. This converts non-image avatars to inline blocks with background colors. See https://github.com/vector-im/element-web/issues/23936 Signed-off-by: Clark Fischer --- src/Avatar.ts | 24 +++-- src/components/views/avatars/BaseAvatar.tsx | 40 +++----- test/Avatar-test.ts | 11 +++ .../__snapshots__/RoomView-test.tsx.snap | 98 ++++++------------- .../__snapshots__/UserMenu-test.tsx.snap | 14 +-- .../views/avatars/MemberAvatar-test.tsx | 3 +- .../views/avatars/RoomAvatar-test.tsx | 10 +- .../__snapshots__/BaseAvatar-test.tsx.snap | 32 ++---- .../__snapshots__/RoomAvatar-test.tsx.snap | 42 +++----- .../__snapshots__/BeaconMarker-test.tsx.snap | 16 +-- .../views/rooms/RoomHeader-test.tsx | 10 +- .../RoomPreviewBar-test.tsx.snap | 28 ++---- .../__snapshots__/RoomTile-test.tsx.snap | 14 +-- 13 files changed, 123 insertions(+), 219 deletions(-) diff --git a/src/Avatar.ts b/src/Avatar.ts index b23d06a55cf..32bc4f6544e 100644 --- a/src/Avatar.ts +++ b/src/Avatar.ts @@ -24,6 +24,8 @@ import DMRoomMap from "./utils/DMRoomMap"; import { mediaFromMxc } from "./customisations/Media"; import { isLocalRoom } from "./utils/localRoom/isLocalRoom"; +const DEFAULT_COLORS: Readonly = ["#0DBD8B", "#368bd6", "#ac3ba8"]; + // Not to be used for BaseAvatar urls as that has similar default avatar fallback already export function avatarUrlForMember( member: RoomMember | null | undefined, @@ -89,16 +91,8 @@ const colorToDataURLCache = new Map(); export function defaultAvatarUrlForString(s: string | undefined): string { if (!s) return ""; // XXX: should never happen but empirically does by evidence of a rageshake - const defaultColors = ["#0DBD8B", "#368bd6", "#ac3ba8"]; - let total = 0; - for (let i = 0; i < s.length; ++i) { - total += s.charCodeAt(i); - } - const colorIndex = total % defaultColors.length; - // overwritten color value in custom themes - const cssVariable = `--avatar-background-colors_${colorIndex}`; - const cssValue = document.body.style.getPropertyValue(cssVariable); - const color = cssValue || defaultColors[colorIndex]!; + + const color = getColorForString(s); let dataUrl = colorToDataURLCache.get(color); if (!dataUrl) { // validate color as this can come from account_data @@ -113,6 +107,16 @@ export function defaultAvatarUrlForString(s: string | undefined): string { return dataUrl; } +export function getColorForString(input: string): string { + const charSum = [...input].reduce((s, c) => s + c.charCodeAt(0), 0); + const index = charSum % DEFAULT_COLORS.length; + + // overwritten color value in custom themes + const cssVariable = `--avatar-background-colors_${index}`; + const cssValue = document.body.style.getPropertyValue(cssVariable); + return cssValue || DEFAULT_COLORS[index]!; +} + /** * returns the first (non-sigil) character of 'name', * converted to uppercase diff --git a/src/components/views/avatars/BaseAvatar.tsx b/src/components/views/avatars/BaseAvatar.tsx index 92c469a0bff..d1dbe7743de 100644 --- a/src/components/views/avatars/BaseAvatar.tsx +++ b/src/components/views/avatars/BaseAvatar.tsx @@ -151,6 +151,10 @@ const BaseAvatar: React.FC = (props) => { className={classNames("mx_BaseAvatar", className)} onClick={onClick} inputRef={inputRef} + style={{ + width: toPx(width), + height: toPx(height), + }} > {avatar} @@ -161,6 +165,10 @@ const BaseAvatar: React.FC = (props) => { className={classNames("mx_BaseAvatar", className)} ref={inputRef} {...otherProps} + style={{ + width: toPx(width), + height: toPx(height), + }} role="presentation" > {avatar} @@ -219,38 +227,22 @@ const TextAvatar: React.FC<{ title?: string; }> = ({ name, idName, width, height, title }) => { const initialLetter = AvatarLogic.getInitialLetter(name); - const textNode = ( + + return ( ); - const imgNode = ( - - ); - - return ( - <> - {textNode} - {imgNode} - - ); }; diff --git a/test/Avatar-test.ts b/test/Avatar-test.ts index 3a5b16104fd..8b4ee03b7fc 100644 --- a/test/Avatar-test.ts +++ b/test/Avatar-test.ts @@ -22,6 +22,7 @@ import { avatarUrlForRoom, avatarUrlForUser, defaultAvatarUrlForString, + getColorForString, getInitialLetter, } from "../src/Avatar"; import { mediaFromMxc } from "../src/customisations/Media"; @@ -84,6 +85,16 @@ describe("defaultAvatarUrlForString", () => { }); }); +describe("getColorForString", () => { + it.each(["a", "abc", "abcde", "@".repeat(150)])("should return a value for %s", (s) => { + expect(getColorForString(s)).toMatch(/^#\w+$/); + }); + + it("should return different values for different strings", () => { + expect(getColorForString("a")).not.toBe(getColorForString("b")); + }); +}); + describe("getInitialLetter", () => { filterConsole("argument to `getInitialLetter` not supplied"); diff --git a/test/components/structures/__snapshots__/RoomView-test.tsx.snap b/test/components/structures/__snapshots__/RoomView-test.tsx.snap index 47318525d56..c81e180c421 100644 --- a/test/components/structures/__snapshots__/RoomView-test.tsx.snap +++ b/test/components/structures/__snapshots__/RoomView-test.tsx.snap @@ -20,22 +20,16 @@ exports[`RoomView for a local room in state CREATING should match the snapshot 1 - @@ -119,22 +113,16 @@ exports[`RoomView for a local room in state ERROR should match the snapshot 1`] - @@ -215,23 +203,17 @@ exports[`RoomView for a local room in state ERROR should match the snapshot 1`] aria-live="off" class="mx_AccessibleButton mx_BaseAvatar" role="button" + style="width: 52px; height: 52px;" tabindex="0" > -

@user:example.com @@ -314,22 +296,16 @@ exports[`RoomView for a local room in state NEW should match the snapshot 1`] = - @@ -410,23 +386,17 @@ exports[`RoomView for a local room in state NEW should match the snapshot 1`] = aria-live="off" class="mx_AccessibleButton mx_BaseAvatar" role="button" + style="width: 52px; height: 52px;" tabindex="0" > -

@user:example.com @@ -581,22 +551,16 @@ exports[`RoomView for a local room in state NEW that is encrypted should match t - @@ -672,23 +636,17 @@ exports[`RoomView for a local room in state NEW that is encrypted should match t aria-live="off" class="mx_AccessibleButton mx_BaseAvatar" role="button" + style="width: 52px; height: 52px;" tabindex="0" > -

@user:example.com diff --git a/test/components/structures/__snapshots__/UserMenu-test.tsx.snap b/test/components/structures/__snapshots__/UserMenu-test.tsx.snap index 769711434a8..0546900abb3 100644 --- a/test/components/structures/__snapshots__/UserMenu-test.tsx.snap +++ b/test/components/structures/__snapshots__/UserMenu-test.tsx.snap @@ -20,22 +20,16 @@ exports[` when rendered should render as expected 1`] = ` - diff --git a/test/components/views/avatars/MemberAvatar-test.tsx b/test/components/views/avatars/MemberAvatar-test.tsx index dc282c8adc9..3dc793bd929 100644 --- a/test/components/views/avatars/MemberAvatar-test.tsx +++ b/test/components/views/avatars/MemberAvatar-test.tsx @@ -110,9 +110,8 @@ describe("MemberAvatar", () => { const { container } = render(getComponent({ member })); - const img = container.querySelector("img"); + const img = container.querySelector(".mx_BaseAvatar_image"); expect(img).not.toBeNull(); - expect(img!.src).toMatch(/^data:/); }); it("dispatches on click", () => { diff --git a/test/components/views/avatars/RoomAvatar-test.tsx b/test/components/views/avatars/RoomAvatar-test.tsx index e23cd96f02d..7be7dd65e90 100644 --- a/test/components/views/avatars/RoomAvatar-test.tsx +++ b/test/components/views/avatars/RoomAvatar-test.tsx @@ -39,7 +39,7 @@ describe("RoomAvatar", () => { const dmRoomMap = new DMRoomMap(client); jest.spyOn(dmRoomMap, "getUserIdForRoomId"); jest.spyOn(DMRoomMap, "shared").mockReturnValue(dmRoomMap); - jest.spyOn(AvatarModule, "defaultAvatarUrlForString"); + jest.spyOn(AvatarModule, "getColorForString"); }); afterAll(() => { @@ -48,14 +48,14 @@ describe("RoomAvatar", () => { afterEach(() => { mocked(DMRoomMap.shared().getUserIdForRoomId).mockReset(); - mocked(AvatarModule.defaultAvatarUrlForString).mockClear(); + mocked(AvatarModule.getColorForString).mockClear(); }); it("should render as expected for a Room", () => { const room = new Room("!room:example.com", client, client.getSafeUserId()); room.name = "test room"; expect(render().container).toMatchSnapshot(); - expect(AvatarModule.defaultAvatarUrlForString).toHaveBeenCalledWith(room.roomId); + expect(AvatarModule.getColorForString).toHaveBeenCalledWith(room.roomId); }); it("should render as expected for a DM room", () => { @@ -64,7 +64,7 @@ describe("RoomAvatar", () => { room.name = "DM room"; mocked(DMRoomMap.shared().getUserIdForRoomId).mockReturnValue(userId); expect(render().container).toMatchSnapshot(); - expect(AvatarModule.defaultAvatarUrlForString).toHaveBeenCalledWith(userId); + expect(AvatarModule.getColorForString).toHaveBeenCalledWith(userId); }); it("should render as expected for a LocalRoom", () => { @@ -73,6 +73,6 @@ describe("RoomAvatar", () => { localRoom.name = "local test room"; localRoom.targets.push(new DirectoryMember({ user_id: userId })); expect(render().container).toMatchSnapshot(); - expect(AvatarModule.defaultAvatarUrlForString).toHaveBeenCalledWith(userId); + expect(AvatarModule.getColorForString).toHaveBeenCalledWith(userId); }); }); diff --git a/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap b/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap index 38202a82ead..da62540b90e 100644 --- a/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap +++ b/test/components/views/avatars/__snapshots__/BaseAvatar-test.tsx.snap @@ -35,24 +35,18 @@ exports[` matches snapshot (no avatar + click) 1`] = ` aria-live="off" class="mx_AccessibleButton mx_BaseAvatar big-and-bold" role="button" + style="width: 40px; height: 40px;" tabindex="0" > - `; @@ -62,23 +56,17 @@ exports[` matches snapshot (no avatar) 1`] = ` - `; diff --git a/test/components/views/avatars/__snapshots__/RoomAvatar-test.tsx.snap b/test/components/views/avatars/__snapshots__/RoomAvatar-test.tsx.snap index 6bffa157b63..699113689e4 100644 --- a/test/components/views/avatars/__snapshots__/RoomAvatar-test.tsx.snap +++ b/test/components/views/avatars/__snapshots__/RoomAvatar-test.tsx.snap @@ -5,22 +5,16 @@ exports[`RoomAvatar should render as expected for a DM room 1`] = ` - `; @@ -30,22 +24,16 @@ exports[`RoomAvatar should render as expected for a LocalRoom 1`] = ` - `; @@ -55,22 +43,16 @@ exports[`RoomAvatar should render as expected for a Room 1`] = ` - `; diff --git a/test/components/views/beacon/__snapshots__/BeaconMarker-test.tsx.snap b/test/components/views/beacon/__snapshots__/BeaconMarker-test.tsx.snap index b42ccb83ee3..b965f50b2f9 100644 --- a/test/components/views/beacon/__snapshots__/BeaconMarker-test.tsx.snap +++ b/test/components/views/beacon/__snapshots__/BeaconMarker-test.tsx.snap @@ -13,23 +13,17 @@ exports[` renders marker when beacon has location 1`] = ` - diff --git a/test/components/views/rooms/RoomHeader-test.tsx b/test/components/views/rooms/RoomHeader-test.tsx index 5857e282957..c27d4c0c20f 100644 --- a/test/components/views/rooms/RoomHeader-test.tsx +++ b/test/components/views/rooms/RoomHeader-test.tsx @@ -72,7 +72,7 @@ describe("RoomHeader (Enzyme)", () => { // And there is no image avatar (because it's not set on this room) const image = findImg(rendered, ".mx_BaseAvatar_image"); - expect(image.prop("src")).toEqual("data:image/png;base64,00"); + expect(image).toBeTruthy(); }); it("shows the room avatar in a room with 2 people", () => { @@ -86,7 +86,7 @@ describe("RoomHeader (Enzyme)", () => { // And there is no image avatar (because it's not set on this room) const image = findImg(rendered, ".mx_BaseAvatar_image"); - expect(image.prop("src")).toEqual("data:image/png;base64,00"); + expect(image).toBeTruthy(); }); it("shows the room avatar in a room with >2 people", () => { @@ -100,7 +100,7 @@ describe("RoomHeader (Enzyme)", () => { // And there is no image avatar (because it's not set on this room) const image = findImg(rendered, ".mx_BaseAvatar_image"); - expect(image.prop("src")).toEqual("data:image/png;base64,00"); + expect(image).toBeTruthy(); }); it("shows the room avatar in a DM with only ourselves", () => { @@ -114,7 +114,7 @@ describe("RoomHeader (Enzyme)", () => { // And there is no image avatar (because it's not set on this room) const image = findImg(rendered, ".mx_BaseAvatar_image"); - expect(image.prop("src")).toEqual("data:image/png;base64,00"); + expect(image).toBeTruthy(); }); it("shows the user avatar in a DM with 2 people", () => { @@ -148,7 +148,7 @@ describe("RoomHeader (Enzyme)", () => { // And there is no image avatar (because it's not set on this room) const image = findImg(rendered, ".mx_BaseAvatar_image"); - expect(image.prop("src")).toEqual("data:image/png;base64,00"); + expect(image).toBeTruthy(); }); it("renders call buttons normally", () => { diff --git a/test/components/views/rooms/__snapshots__/RoomPreviewBar-test.tsx.snap b/test/components/views/rooms/__snapshots__/RoomPreviewBar-test.tsx.snap index f35467e1efd..a78a452e890 100644 --- a/test/components/views/rooms/__snapshots__/RoomPreviewBar-test.tsx.snap +++ b/test/components/views/rooms/__snapshots__/RoomPreviewBar-test.tsx.snap @@ -161,22 +161,16 @@ exports[` with an invite without an invited email for a dm roo -

@@ -236,22 +230,16 @@ exports[` with an invite without an invited email for a non-dm -

diff --git a/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap b/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap index bcbb7932c69..557d97c243e 100644 --- a/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap +++ b/test/components/views/rooms/__snapshots__/RoomTile-test.tsx.snap @@ -15,22 +15,16 @@ exports[`RoomTile should render the room 1`] = ` -

Date: Wed, 25 Jan 2023 15:45:24 -0800 Subject: [PATCH 5/5] Have pills use solid backgrounds rather than colored images Similar to room and member avatars, pills now use colored pseudo-elements rather than background images. Signed-off-by: Clark Fischer --- .../views/rooms/_BasicMessageComposer.pcss | 2 +- src/Avatar.ts | 11 +++ src/editor/parts.ts | 38 +++++----- test/editor/__snapshots__/parts-test.ts.snap | 45 ++++++++++++ test/editor/parts-test.ts | 72 ++++++++++++++++++- 5 files changed, 148 insertions(+), 20 deletions(-) create mode 100644 test/editor/__snapshots__/parts-test.ts.snap diff --git a/res/css/views/rooms/_BasicMessageComposer.pcss b/res/css/views/rooms/_BasicMessageComposer.pcss index 7b88a058153..32e7c5288f6 100644 --- a/res/css/views/rooms/_BasicMessageComposer.pcss +++ b/res/css/views/rooms/_BasicMessageComposer.pcss @@ -78,7 +78,7 @@ limitations under the License. min-width: $font-16px; /* ensure the avatar is not compressed */ height: $font-16px; margin-inline-end: 0.24rem; - background: var(--avatar-background), $background; + background: var(--avatar-background); color: $avatar-initial-color; background-repeat: no-repeat; background-size: $font-16px; diff --git a/src/Avatar.ts b/src/Avatar.ts index 32bc4f6544e..3e6b18dbc79 100644 --- a/src/Avatar.ts +++ b/src/Avatar.ts @@ -47,6 +47,17 @@ export function avatarUrlForMember( return url; } +export function getMemberAvatar( + member: RoomMember | null | undefined, + width: number, + height: number, + resizeMethod: ResizeMethod, +): string | undefined { + const mxcUrl = member?.getMxcAvatarUrl(); + if (!mxcUrl) return undefined; + return mediaFromMxc(mxcUrl).getThumbnailOfSourceHttp(width, height, resizeMethod); +} + export function avatarUrlForUser( user: Pick, width: number, diff --git a/src/editor/parts.ts b/src/editor/parts.ts index c9b756686f2..0157cd738a0 100644 --- a/src/editor/parts.ts +++ b/src/editor/parts.ts @@ -1,6 +1,5 @@ /* -Copyright 2019 New Vector Ltd -Copyright 2019 The Matrix.org Foundation C.I.C. +Copyright 2019, 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -295,8 +294,8 @@ export abstract class PillPart extends BasePart implements IPillPart { } // helper method for subclasses - protected setAvatarVars(node: HTMLElement, avatarUrl: string, initialLetter: string | undefined): void { - const avatarBackground = `url('${avatarUrl}')`; + protected setAvatarVars(node: HTMLElement, avatarBackground: string, initialLetter: string | undefined): void { + // const avatarBackground = `url('${avatarUrl}')`; const avatarLetter = `'${initialLetter || ""}'`; // check if the value is changing, // otherwise the avatars flicker on every keystroke while updating. @@ -413,13 +412,15 @@ class RoomPillPart extends PillPart { } protected setAvatar(node: HTMLElement): void { - let initialLetter: string | undefined = ""; - let avatarUrl = Avatar.avatarUrlForRoom(this.room, 16, 16, "crop"); - if (!avatarUrl) { - initialLetter = Avatar.getInitialLetter(this.room?.name || this.resourceId); - avatarUrl = Avatar.defaultAvatarUrlForString(this.room?.roomId ?? this.resourceId); + const avatarUrl = Avatar.avatarUrlForRoom(this.room, 16, 16, "crop"); + if (avatarUrl) { + this.setAvatarVars(node, `url('${avatarUrl}')`, ""); + return; } - this.setAvatarVars(node, avatarUrl, initialLetter); + + const initialLetter = Avatar.getInitialLetter(this.room?.name || this.resourceId); + const color = Avatar.getColorForString(this.room?.roomId ?? this.resourceId); + this.setAvatarVars(node, color, initialLetter); } public get type(): IPillPart["type"] { @@ -465,14 +466,17 @@ class UserPillPart extends PillPart { if (!this.member) { return; } - const name = this.member.name || this.member.userId; - const defaultAvatarUrl = Avatar.defaultAvatarUrlForString(this.member.userId); - const avatarUrl = Avatar.avatarUrlForMember(this.member, 16, 16, "crop"); - let initialLetter: string | undefined = ""; - if (avatarUrl === defaultAvatarUrl) { - initialLetter = Avatar.getInitialLetter(name); + + const avatar = Avatar.getMemberAvatar(this.member, 16, 16, "crop"); + if (avatar) { + this.setAvatarVars(node, `url('${avatar}')`, ""); + return; } - this.setAvatarVars(node, avatarUrl, initialLetter); + + const name = this.member.name || this.member.userId; + const initialLetter = Avatar.getInitialLetter(name); + const color = Avatar.getColorForString(this.member.userId); + this.setAvatarVars(node, color, initialLetter); } protected onClick = (): void => { diff --git a/test/editor/__snapshots__/parts-test.ts.snap b/test/editor/__snapshots__/parts-test.ts.snap new file mode 100644 index 00000000000..e8a6ef9d6d7 --- /dev/null +++ b/test/editor/__snapshots__/parts-test.ts.snap @@ -0,0 +1,45 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`RoomPillPart matches snapshot (avatar) 1`] = ` + + !room:example.com + +`; + +exports[`RoomPillPart matches snapshot (no avatar) 1`] = ` + + !room:example.com + +`; + +exports[`UserPillPart matches snapshot (avatar) 1`] = ` + + DisplayName + +`; + +exports[`UserPillPart matches snapshot (no avatar) 1`] = ` + + DisplayName + +`; diff --git a/test/editor/parts-test.ts b/test/editor/parts-test.ts index 534221ece3a..31c620c94ad 100644 --- a/test/editor/parts-test.ts +++ b/test/editor/parts-test.ts @@ -1,5 +1,5 @@ /* -Copyright 2022 The Matrix.org Foundation C.I.C. +Copyright 2022 - 2023 The Matrix.org Foundation C.I.C. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,7 +14,11 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { EmojiPart, PlainPart } from "../../src/editor/parts"; +import { MatrixClient, Room, RoomMember } from "matrix-js-sdk/src/matrix"; + +import { EmojiPart, PartCreator, PlainPart } from "../../src/editor/parts"; +import DMRoomMap from "../../src/utils/DMRoomMap"; +import { stubClient } from "../test-utils"; import { createPartCreator } from "./mock"; describe("editor/parts", () => { @@ -40,3 +44,67 @@ describe("editor/parts", () => { expect(() => part.toDOMNode()).not.toThrow(); }); }); + +describe("UserPillPart", () => { + const roomId = "!room:example.com"; + let client: MatrixClient; + let room: Room; + let creator: PartCreator; + + beforeEach(() => { + client = stubClient(); + room = new Room(roomId, client, "@me:example.com"); + creator = new PartCreator(room, client); + }); + + it("matches snapshot (no avatar)", () => { + jest.spyOn(room, "getMember").mockReturnValue(new RoomMember(room.roomId, "@user:example.com")); + const pill = creator.userPill("DisplayName", "@user:example.com"); + const el = pill.toDOMNode(); + + expect(el).toMatchSnapshot(); + }); + + it("matches snapshot (avatar)", () => { + const member = new RoomMember(room.roomId, "@user:example.com"); + jest.spyOn(room, "getMember").mockReturnValue(member); + jest.spyOn(member, "getMxcAvatarUrl").mockReturnValue("mxc://www.example.com/avatar.png"); + + const pill = creator.userPill("DisplayName", "@user:example.com"); + const el = pill.toDOMNode(); + + expect(el).toMatchSnapshot(); + }); +}); + +describe("RoomPillPart", () => { + const roomId = "!room:example.com"; + let client: jest.Mocked; + let room: Room; + let creator: PartCreator; + + beforeEach(() => { + client = stubClient() as jest.Mocked; + DMRoomMap.makeShared(); + + room = new Room(roomId, client, "@me:example.com"); + client.getRoom.mockReturnValue(room); + creator = new PartCreator(room, client); + }); + + it("matches snapshot (no avatar)", () => { + jest.spyOn(room, "getMxcAvatarUrl").mockReturnValue(null); + const pill = creator.roomPill("super-secret clubhouse"); + const el = pill.toDOMNode(); + + expect(el).toMatchSnapshot(); + }); + + it("matches snapshot (avatar)", () => { + jest.spyOn(room, "getMxcAvatarUrl").mockReturnValue("mxc://www.example.com/avatars/room1.jpeg"); + const pill = creator.roomPill("cool chat club"); + const el = pill.toDOMNode(); + + expect(el).toMatchSnapshot(); + }); +});