Fix read receipt animation

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<IProps, IState> {
 
     // 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:

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

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}

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<IProps, IStat
     public componentWillUnmount(): void {
         // before we remove the rr, store its location in the map, so that if
         // it reappears, it can be animated from the right place.
-        const rrInfo = this.props.readReceiptInfo;
+        const rrInfo = this.props.readReceiptPosition;
         if (!rrInfo) {
             return;
         }
@@ -121,63 +121,39 @@ export default class ReadReceiptMarker extends React.PureComponent<IProps, IStat
         }
     }
 
-    private buildReadReceiptInfo(target: IReadReceiptInfo = {}): IReadReceiptInfo {
+    private buildReadReceiptInfo(target: IReadReceiptPosition = {}): IReadReceiptPosition {
         const element = this.avatar.current;
         // this is the mx_ReadReceiptsGroup_container
         const horizontalContainer = element?.offsetParent;
-        if (!horizontalContainer || !(horizontalContainer instanceof HTMLElement)) {
+        if (!horizontalContainer || !horizontalContainer.getBoundingClientRect) {
             // this seems to happen sometimes for reasons I don't understand
             // the docs for `offsetParent` say it may be null if `display` is
             // `none`, but I can't see why that would happen.
             logger.warn(`ReadReceiptMarker for ${this.props.fallbackUserId} has no valid horizontalContainer`);
 
             target.top = 0;
             target.right = 0;
-            target.parent = undefined;
             return target;
         }
-        // this is the mx_ReadReceiptsGroup
-        const verticalContainer = horizontalContainer.offsetParent;
-        if (!verticalContainer || !(verticalContainer instanceof HTMLElement)) {
-            // this seems to happen sometimes for reasons I don't understand
-            // the docs for `offsetParent` say it may be null if `display` is
-            // `none`, but I can't see why that would happen.
-            logger.warn(`ReadReceiptMarker for ${this.props.fallbackUserId} has no valid verticalContainer`);
 
-            target.top = 0;
-            target.right = 0;
-            target.parent = undefined;
-            return target;
-        }
+        const elementRect = element.getBoundingClientRect();
 
-        target.top = element.offsetTop;
-        target.right = element.getBoundingClientRect().right - horizontalContainer.getBoundingClientRect().right;
-        target.parent = verticalContainer;
+        target.top = elementRect.top;
+        target.right = elementRect.right - horizontalContainer.getBoundingClientRect().right;
         return target;
     }
 
-    private readReceiptPosition(info: IReadReceiptInfo): number {
-        if (!info.parent) {
-            // this seems to happen sometimes for reasons I don't understand
-            // the docs for `offsetParent` say it may be null if `display` is
-            // `none`, but I can't see why that would happen.
-            logger.warn(`ReadReceiptMarker for ${this.props.fallbackUserId} has no offsetParent`);
-            return 0;
-        }
-
-        return (info.top ?? 0) + info.parent.getBoundingClientRect().top;
-    }
-
     private animateMarker(): void {
-        const oldInfo = this.props.readReceiptInfo;
+        const oldInfo = this.props.readReceiptPosition;
         const newInfo = this.buildReadReceiptInfo();
 
-        const newPosition = this.readReceiptPosition(newInfo);
-        const oldPosition = oldInfo
-            ? // start at the old height and in the old h pos
-              this.readReceiptPosition(oldInfo)
-            : // treat new RRs as though they were off the top of the screen
-              -READ_AVATAR_SIZE;
+        const newPosition = newInfo.top ?? 0;
+        const oldPosition =
+            oldInfo && oldInfo.top !== undefined
+                ? // start at the old height and in the old h pos
+                  oldInfo.top
+                : // treat new RRs as though they were off the top of the screen
+                  -READ_AVATAR_SIZE;
 
         const startStyles: IReadReceiptMarkerStyle[] = [];
         if (oldInfo?.right) {

test/components/views/rooms/ReadReceiptMarker-test.tsx

@@ -0,0 +1,79 @@
+/*
+Copyright 2024 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 React from "react";
+import { render, screen } from "@testing-library/react";
+
+import ReadReceiptMarker, { IReadReceiptPosition } from "../../../../src/components/views/rooms/ReadReceiptMarker";
+
+describe("ReadReceiptMarker", () => {
+    afterEach(() => {
+        jest.restoreAllMocks();
+        jest.useRealTimers();
+    });
+
+    it("should position at -16px if given no previous position", () => {
+        render(<ReadReceiptMarker fallbackUserId="bob" offset={0} />);
+
+        expect(screen.getByTestId("avatar-img").style.top).toBe("-16px");
+    });
+
+    it("should position at previous top if given", () => {
+        render(<ReadReceiptMarker fallbackUserId="bob" offset={0} readReceiptPosition={{ top: 100, right: 0 }} />);
+
+        expect(screen.getByTestId("avatar-img").style.top).toBe("100px");
+    });
+
+    it("should apply new styles after mounted to animate", () => {
+        jest.useFakeTimers();
+
+        render(<ReadReceiptMarker fallbackUserId="bob" offset={0} readReceiptPosition={{ top: 100, right: 0 }} />);
+        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(<ReadReceiptMarker fallbackUserId="bob" offset={0} readReceiptPosition={pos} />);
+
+        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(<ReadReceiptMarker fallbackUserId="bob" offset={0} readReceiptPosition={pos} />);
+
+        expect(pos.top).toBeUndefined();
+
+        unmount();
+
+        expect(pos.top).toBe(100);
+    });
+});