From 6bfdb3e16b738a0353da8f344a3a1716ab730f97 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 3 Sep 2024 12:59:30 +0100 Subject: [PATCH] Fix read receipt animation (#12923) * Fix read receipt animation The way it was done involved remembering dom nodes and then getting their position later when animating the receipt to its next position, but I'm not sure how this worked since the DOM node may not neccessarily be in the DOM anymore. Instead, just remember the bounding box coordinates. At worst it might go weird if the window is resized but seems fine in practice. Also, keeping references to dom nodes feels like a fast road to memory leaks. Fixes https://github.com/element-hq/element-web/issues/27916 * Attempt to write a test for read receipts and fix naming * Another test also change a condition to make it testable --- src/components/structures/MessagePanel.tsx | 4 +- src/components/views/rooms/EventTile.tsx | 4 +- .../views/rooms/ReadReceiptGroup.tsx | 16 ++-- .../views/rooms/ReadReceiptMarker.tsx | 58 ++++---------- .../views/rooms/ReadReceiptMarker-test.tsx | 79 +++++++++++++++++++ 5 files changed, 108 insertions(+), 53 deletions(-) create mode 100644 test/components/views/rooms/ReadReceiptMarker-test.tsx diff --git a/src/components/structures/MessagePanel.tsx b/src/components/structures/MessagePanel.tsx index 07b600484ac..c28992f33e0 100644 --- a/src/components/structures/MessagePanel.tsx +++ b/src/components/structures/MessagePanel.tsx @@ -47,7 +47,7 @@ import { RoomPermalinkCreator } from "../../utils/permalinks/Permalinks"; import EditorStateTransfer from "../../utils/EditorStateTransfer"; import { Action } from "../../dispatcher/actions"; import { getEventDisplayInfo } from "../../utils/EventRenderingUtils"; -import { IReadReceiptInfo } from "../views/rooms/ReadReceiptMarker"; +import { IReadReceiptPosition } from "../views/rooms/ReadReceiptMarker"; import { haveRendererForEvent } from "../../events/EventTileFactory"; import { editorRoomKey } from "../../Editing"; import { hasThreadSummary } from "../../utils/EventUtils"; @@ -213,7 +213,7 @@ export default class MessagePanel extends React.Component { // opaque readreceipt info for each userId; used by ReadReceiptMarker // to manage its animations - private readReceiptMap: { [userId: string]: IReadReceiptInfo } = {}; + private readReceiptMap: { [userId: string]: IReadReceiptPosition } = {}; // Track read receipts by event ID. For each _shown_ event ID, we store // the list of read receipts to display: diff --git a/src/components/views/rooms/EventTile.tsx b/src/components/views/rooms/EventTile.tsx index e0f8ab1161c..616ed1442cf 100644 --- a/src/components/views/rooms/EventTile.tsx +++ b/src/components/views/rooms/EventTile.tsx @@ -60,7 +60,7 @@ import PlatformPeg from "../../../PlatformPeg"; import MemberAvatar from "../avatars/MemberAvatar"; import SenderProfile from "../messages/SenderProfile"; import MessageTimestamp from "../messages/MessageTimestamp"; -import { IReadReceiptInfo } from "./ReadReceiptMarker"; +import { IReadReceiptPosition } from "./ReadReceiptMarker"; import MessageActionBar from "../messages/MessageActionBar"; import ReactionsRow from "../messages/ReactionsRow"; import { getEventDisplayInfo } from "../../../utils/EventRenderingUtils"; @@ -167,7 +167,7 @@ export interface EventTileProps { // opaque readreceipt info for each userId; used by ReadReceiptMarker // to manage its animations. Should be an empty object when the room // first loads - readReceiptMap?: { [userId: string]: IReadReceiptInfo }; + readReceiptMap?: { [userId: string]: IReadReceiptPosition }; // A function which is used to check if the parent panel is being // unmounted, to avoid unnecessary work. Should return true if we diff --git a/src/components/views/rooms/ReadReceiptGroup.tsx b/src/components/views/rooms/ReadReceiptGroup.tsx index c9d00a4e699..551658a54b8 100644 --- a/src/components/views/rooms/ReadReceiptGroup.tsx +++ b/src/components/views/rooms/ReadReceiptGroup.tsx @@ -18,7 +18,7 @@ import React, { PropsWithChildren } from "react"; import { User } from "matrix-js-sdk/src/matrix"; import { Tooltip } from "@vector-im/compound-web"; -import ReadReceiptMarker, { IReadReceiptInfo } from "./ReadReceiptMarker"; +import ReadReceiptMarker, { IReadReceiptPosition } from "./ReadReceiptMarker"; import { IReadReceiptProps } from "./EventTile"; import AccessibleButton from "../elements/AccessibleButton"; import MemberAvatar from "../avatars/MemberAvatar"; @@ -41,7 +41,7 @@ export const READ_AVATAR_SIZE = 16; interface Props { readReceipts: IReadReceiptProps[]; - readReceiptMap: { [userId: string]: IReadReceiptInfo }; + readReceiptMap: { [userId: string]: IReadReceiptPosition }; checkUnmounting?: () => boolean; suppressAnimation: boolean; isTwelveHour?: boolean; @@ -111,13 +111,13 @@ export function ReadReceiptGroup({ const { hidden, position } = determineAvatarPosition(index, maxAvatars); const userId = receipt.userId; - let readReceiptInfo: IReadReceiptInfo | undefined; + let readReceiptPosition: IReadReceiptPosition | undefined; if (readReceiptMap) { - readReceiptInfo = readReceiptMap[userId]; - if (!readReceiptInfo) { - readReceiptInfo = {}; - readReceiptMap[userId] = readReceiptInfo; + readReceiptPosition = readReceiptMap[userId]; + if (!readReceiptPosition) { + readReceiptPosition = {}; + readReceiptMap[userId] = readReceiptPosition; } } @@ -128,7 +128,7 @@ export function ReadReceiptGroup({ fallbackUserId={userId} offset={position * READ_AVATAR_OFFSET} hidden={hidden} - readReceiptInfo={readReceiptInfo} + readReceiptPosition={readReceiptPosition} checkUnmounting={checkUnmounting} suppressAnimation={suppressAnimation} timestamp={receipt.ts} diff --git a/src/components/views/rooms/ReadReceiptMarker.tsx b/src/components/views/rooms/ReadReceiptMarker.tsx index 7b907e6f23e..c1f2e3ccaaa 100644 --- a/src/components/views/rooms/ReadReceiptMarker.tsx +++ b/src/components/views/rooms/ReadReceiptMarker.tsx @@ -24,10 +24,10 @@ import { toPx } from "../../../utils/units"; import MemberAvatar from "../avatars/MemberAvatar"; import { READ_AVATAR_SIZE } from "./ReadReceiptGroup"; -export interface IReadReceiptInfo { +// The top & right from the bounding client rect of each read receipt +export interface IReadReceiptPosition { top?: number; right?: number; - parent?: Element; } interface IProps { @@ -48,7 +48,7 @@ interface IProps { suppressAnimation?: boolean; // an opaque object for storing information about this user's RR in this room - readReceiptInfo?: IReadReceiptInfo; + readReceiptPosition?: IReadReceiptPosition; // A function which is used to check if the parent panel is being // unmounted, to avoid unnecessary work. Should return true if we @@ -90,7 +90,7 @@ export default class ReadReceiptMarker extends React.PureComponent { + afterEach(() => { + jest.restoreAllMocks(); + jest.useRealTimers(); + }); + + it("should position at -16px if given no previous position", () => { + render(); + + expect(screen.getByTestId("avatar-img").style.top).toBe("-16px"); + }); + + it("should position at previous top if given", () => { + render(); + + expect(screen.getByTestId("avatar-img").style.top).toBe("100px"); + }); + + it("should apply new styles after mounted to animate", () => { + jest.useFakeTimers(); + + render(); + expect(screen.getByTestId("avatar-img").style.top).toBe("100px"); + + jest.runAllTimers(); + + expect(screen.getByTestId("avatar-img").style.top).toBe("0px"); + }); + + it("should update readReceiptPosition when unmounted", () => { + const pos: IReadReceiptPosition = {}; + const { unmount } = render(); + + expect(pos.top).toBeUndefined(); + + unmount(); + + expect(pos.top).toBe(0); + }); + + it("should update readReceiptPosition to current position", () => { + const pos: IReadReceiptPosition = {}; + jest.spyOn(HTMLElement.prototype, "offsetParent", "get").mockImplementation(function (): Element | null { + return { + getBoundingClientRect: jest.fn().mockReturnValue({ top: 0, right: 0 } as DOMRect), + } as unknown as Element; + }); + jest.spyOn(HTMLElement.prototype, "getBoundingClientRect").mockReturnValue({ top: 100, right: 0 } as DOMRect); + + const { unmount } = render(); + + expect(pos.top).toBeUndefined(); + + unmount(); + + expect(pos.top).toBe(100); + }); +});