Skip to content

Commit

Permalink
Use standard HTML for non-image avatars
Browse files Browse the repository at this point in the history
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 element-hq/element-web#23936

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
  • Loading branch information
clarkf committed Jan 26, 2023
1 parent 26dd36f commit 527da5a
Show file tree
Hide file tree
Showing 13 changed files with 115 additions and 219 deletions.
24 changes: 14 additions & 10 deletions src/Avatar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import DMRoomMap from "./utils/DMRoomMap";
import { mediaFromMxc } from "./customisations/Media";
import { isLocalRoom } from "./utils/localRoom/isLocalRoom";

const DEFAULT_COLORS: Readonly<string[]> = ["#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,
Expand Down Expand Up @@ -89,16 +91,8 @@ const colorToDataURLCache = new Map<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;
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
Expand All @@ -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
Expand Down
36 changes: 12 additions & 24 deletions src/components/views/avatars/BaseAvatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ const BaseAvatar: React.FC<IProps> = (props) => {
className={classNames("mx_BaseAvatar", className)}
ref={inputRef}
{...otherProps}
style={{
width: toPx(width),
height: toPx(height),
}}
role="presentation"
>
{avatar}
Expand Down Expand Up @@ -219,38 +223,22 @@ const TextAvatar: React.FC<{
title?: string;
}> = ({ name, idName, width, height, title }) => {
const initialLetter = AvatarLogic.getInitialLetter(name);
const textNode = (

return (
<span
className="mx_BaseAvatar_initial"
className="mx_BaseAvatar_image mx_BaseAvatar_initial"
aria-hidden="true"
style={{
fontSize: toPx(width * 0.65),
backgroundColor: AvatarLogic.getColorForString(idName || name),
width: toPx(width),
height: toPx(height),
fontSize: toPx(width * 0.65),
lineHeight: toPx(height),
}}
title={title}
data-testid="avatar-img"
>
{initialLetter}
</span>
);
const imgNode = (
<img
className="mx_BaseAvatar_image"
src={AvatarLogic.defaultAvatarUrlForString(idName || name)}
alt=""
title={title}
style={{
width: toPx(width),
height: toPx(height),
}}
aria-hidden="true"
data-testid="avatar-img"
/>
);

return (
<>
{textNode}
{imgNode}
</>
);
};
11 changes: 11 additions & 0 deletions test/Avatar-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
avatarUrlForRoom,
avatarUrlForUser,
defaultAvatarUrlForString,
getColorForString,
getInitialLetter,
} from "../src/Avatar";
import { mediaFromMxc } from "../src/customisations/Media";
Expand Down Expand Up @@ -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");

Expand Down
95 changes: 25 additions & 70 deletions test/components/structures/__snapshots__/RoomView-test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,16 @@ exports[`RoomView for a local room in state CREATING should match the snapshot 1
<span
class="mx_BaseAvatar"
role="presentation"
style="width: 24px; height: 24px;"
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 15.600000000000001px; width: 24px; line-height: 24px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 24px; height: 24px; font-size: 15.600000000000001px; line-height: 24px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 24px; height: 24px;"
/>
</span>
</div>
</div>
Expand Down Expand Up @@ -119,22 +113,16 @@ exports[`RoomView for a local room in state ERROR should match the snapshot 1`]
<span
class="mx_BaseAvatar"
role="presentation"
style="width: 24px; height: 24px;"
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 15.600000000000001px; width: 24px; line-height: 24px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 24px; height: 24px; font-size: 15.600000000000001px; line-height: 24px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 24px; height: 24px;"
/>
</span>
</div>
</div>
Expand Down Expand Up @@ -219,19 +207,12 @@ exports[`RoomView for a local room in state ERROR should match the snapshot 1`]
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 33.800000000000004px; width: 52px; line-height: 52px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 52px; height: 52px; font-size: 33.800000000000004px; line-height: 52px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 52px; height: 52px;"
/>
</span>
<h2>
@user:example.com
Expand Down Expand Up @@ -314,22 +295,16 @@ exports[`RoomView for a local room in state NEW should match the snapshot 1`] =
<span
class="mx_BaseAvatar"
role="presentation"
style="width: 24px; height: 24px;"
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 15.600000000000001px; width: 24px; line-height: 24px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 24px; height: 24px; font-size: 15.600000000000001px; line-height: 24px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 24px; height: 24px;"
/>
</span>
</div>
</div>
Expand Down Expand Up @@ -414,19 +389,12 @@ exports[`RoomView for a local room in state NEW should match the snapshot 1`] =
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 33.800000000000004px; width: 52px; line-height: 52px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 52px; height: 52px; font-size: 33.800000000000004px; line-height: 52px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 52px; height: 52px;"
/>
</span>
<h2>
@user:example.com
Expand Down Expand Up @@ -581,22 +549,16 @@ exports[`RoomView for a local room in state NEW that is encrypted should match t
<span
class="mx_BaseAvatar"
role="presentation"
style="width: 24px; height: 24px;"
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 15.600000000000001px; width: 24px; line-height: 24px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 24px; height: 24px; font-size: 15.600000000000001px; line-height: 24px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 24px; height: 24px;"
/>
</span>
</div>
</div>
Expand Down Expand Up @@ -676,19 +638,12 @@ exports[`RoomView for a local room in state NEW that is encrypted should match t
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 33.800000000000004px; width: 52px; line-height: 52px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(172, 59, 168); width: 52px; height: 52px; font-size: 33.800000000000004px; line-height: 52px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 52px; height: 52px;"
/>
</span>
<h2>
@user:example.com
Expand Down
14 changes: 4 additions & 10 deletions test/components/structures/__snapshots__/UserMenu-test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,16 @@ exports[`<UserMenu> when rendered should render as expected 1`] = `
<span
class="mx_BaseAvatar mx_UserMenu_userAvatar_BaseAvatar"
role="presentation"
style="width: 32px; height: 32px;"
>
<span
aria-hidden="true"
class="mx_BaseAvatar_initial"
style="font-size: 20.8px; width: 32px; line-height: 32px;"
class="mx_BaseAvatar_image mx_BaseAvatar_initial"
data-testid="avatar-img"
style="background-color: rgb(54, 139, 214); width: 32px; height: 32px; font-size: 20.8px; line-height: 32px;"
>
U
</span>
<img
alt=""
aria-hidden="true"
class="mx_BaseAvatar_image"
data-testid="avatar-img"
src="data:image/png;base64,00"
style="width: 32px; height: 32px;"
/>
</span>
</div>
</div>
Expand Down
3 changes: 1 addition & 2 deletions test/components/views/avatars/MemberAvatar-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
10 changes: 5 additions & 5 deletions test/components/views/avatars/RoomAvatar-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand All @@ -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(<RoomAvatar room={room} />).container).toMatchSnapshot();
expect(AvatarModule.defaultAvatarUrlForString).toHaveBeenCalledWith(room.roomId);
expect(AvatarModule.getColorForString).toHaveBeenCalledWith(room.roomId);
});

it("should render as expected for a DM room", () => {
Expand All @@ -64,7 +64,7 @@ describe("RoomAvatar", () => {
room.name = "DM room";
mocked(DMRoomMap.shared().getUserIdForRoomId).mockReturnValue(userId);
expect(render(<RoomAvatar room={room} />).container).toMatchSnapshot();
expect(AvatarModule.defaultAvatarUrlForString).toHaveBeenCalledWith(userId);
expect(AvatarModule.getColorForString).toHaveBeenCalledWith(userId);
});

it("should render as expected for a LocalRoom", () => {
Expand All @@ -73,6 +73,6 @@ describe("RoomAvatar", () => {
localRoom.name = "local test room";
localRoom.targets.push(new DirectoryMember({ user_id: userId }));
expect(render(<RoomAvatar room={localRoom} />).container).toMatchSnapshot();
expect(AvatarModule.defaultAvatarUrlForString).toHaveBeenCalledWith(userId);
expect(AvatarModule.getColorForString).toHaveBeenCalledWith(userId);
});
});
Loading

0 comments on commit 527da5a

Please sign in to comment.