From 3eb6a55b93ca84547075d387672f123f340d3d54 Mon Sep 17 00:00:00 2001
From: Michael Weimann <michaelw@matrix.org>
Date: Wed, 22 Mar 2023 13:27:24 +0100
Subject: [PATCH] Tweak pill UI (#10417)

---
 res/css/views/elements/_Pill.pcss             |  6 --
 src/components/views/elements/Pill.tsx        | 43 +++++----
 src/hooks/usePermalink.ts                     |  8 +-
 src/i18n/strings/en_EN.json                   |  4 +-
 .../elements/__snapshots__/Pill-test.tsx.snap |  6 +-
 .../views/messages/TextualBody-test.tsx       | 91 +++++++++++++++----
 .../__snapshots__/TextualBody-test.tsx.snap   | 71 ++++++++++++++-
 test/test-utils/test-utils.ts                 |  2 +-
 8 files changed, 177 insertions(+), 54 deletions(-)

diff --git a/res/css/views/elements/_Pill.pcss b/res/css/views/elements/_Pill.pcss
index 6cced8b8b5d..aa9c236f8b3 100644
--- a/res/css/views/elements/_Pill.pcss
+++ b/res/css/views/elements/_Pill.pcss
@@ -59,12 +59,6 @@ limitations under the License.
         min-width: $font-16px; /* ensure the avatar is not compressed */
     }
 
-    &.mx_EventPill .mx_BaseAvatar {
-        /* Event pill avatars are inside the text. */
-        margin-inline-start: 0.2em;
-        margin-inline-end: 0.2em;
-    }
-
     .mx_Pill_text {
         min-width: 0;
         overflow: hidden;
diff --git a/src/components/views/elements/Pill.tsx b/src/components/views/elements/Pill.tsx
index ab0c2b7e2f8..67d0f6d6d65 100644
--- a/src/components/views/elements/Pill.tsx
+++ b/src/components/views/elements/Pill.tsx
@@ -45,6 +45,8 @@ export const pillRoomNotifLen = (): number => {
     return "@room".length;
 };
 
+const linkIcon = <LinkIcon className="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image" />;
+
 const PillRoomAvatar: React.FC<{
     shouldShowPillAvatar: boolean;
     room: Room | null;
@@ -56,7 +58,7 @@ const PillRoomAvatar: React.FC<{
     if (room) {
         return <RoomAvatar room={room} width={16} height={16} aria-hidden="true" />;
     }
-    return <LinkIcon className="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image" />;
+    return linkIcon;
 };
 
 const PillMemberAvatar: React.FC<{
@@ -88,7 +90,7 @@ export interface PillProps {
 
 export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room, shouldShowPillAvatar = true }) => {
     const [hover, setHover] = useState(false);
-    const { member, onClick, resourceId, targetRoom, text, type } = usePermalink({
+    const { event, member, onClick, resourceId, targetRoom, text, type } = usePermalink({
         room,
         type: propType,
         url,
@@ -116,35 +118,38 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
     };
 
     const tip = hover && resourceId ? <Tooltip label={resourceId} alignment={Alignment.Right} /> : null;
-    let content: (ReactElement | string)[] = [];
-    const textElement = <span className="mx_Pill_text">{text}</span>;
+    let avatar: ReactElement | null = null;
+    let pillText: string | null = text;
 
     switch (type) {
         case PillType.EventInOtherRoom:
             {
-                const avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
-                content = [_t("Message in"), avatar || " ", textElement];
+                avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
+                pillText = _t("Message in %(room)s", {
+                    room: text,
+                });
             }
             break;
         case PillType.EventInSameRoom:
             {
-                const avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
-                content = [_t("Message from"), avatar || " ", textElement];
+                if (event) {
+                    avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
+                    pillText = _t("Message from %(user)s", {
+                        user: text,
+                    });
+                } else {
+                    avatar = linkIcon;
+                    pillText = _t("Message");
+                }
             }
             break;
         case PillType.AtRoomMention:
         case PillType.RoomMention:
         case "space":
-            {
-                const avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
-                content = [avatar, textElement];
-            }
+            avatar = <PillRoomAvatar shouldShowPillAvatar={shouldShowPillAvatar} room={targetRoom} />;
             break;
         case PillType.UserMention:
-            {
-                const avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
-                content = [avatar, textElement];
-            }
+            avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />;
             break;
         default:
             return null;
@@ -161,12 +166,14 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room
                         onMouseOver={onMouseOver}
                         onMouseLeave={onMouseLeave}
                     >
-                        {content}
+                        {avatar}
+                        <span className="mx_Pill_text">{pillText}</span>
                         {tip}
                     </a>
                 ) : (
                     <span className={classes} onMouseOver={onMouseOver} onMouseLeave={onMouseLeave}>
-                        {content}
+                        {avatar}
+                        <span className="mx_Pill_text">{pillText}</span>
                         {tip}
                     </span>
                 )}
diff --git a/src/hooks/usePermalink.ts b/src/hooks/usePermalink.ts
index 6bf912786e6..362ec4bc30f 100644
--- a/src/hooks/usePermalink.ts
+++ b/src/hooks/usePermalink.ts
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import { Room, RoomMember } from "matrix-js-sdk/src/matrix";
+import { MatrixEvent, Room, RoomMember } from "matrix-js-sdk/src/matrix";
 
 import { ButtonEvent } from "../components/views/elements/AccessibleButton";
 import { PillType } from "../components/views/elements/Pill";
@@ -70,6 +70,11 @@ interface HookResult {
      * null here means that the type cannot be detected. Most likely if the URL was not a permalink.
      */
     type: PillType | "space" | null;
+    /**
+     * Target event of the permalink.
+     * Null if unable to load the event.
+     */
+    event: MatrixEvent | null;
 }
 
 /**
@@ -166,6 +171,7 @@ export const usePermalink: (args: Args) => HookResult = ({
     }
 
     return {
+        event,
         member,
         onClick,
         resourceId,
diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
index a72a13d87ea..862f33eec34 100644
--- a/src/i18n/strings/en_EN.json
+++ b/src/i18n/strings/en_EN.json
@@ -2592,8 +2592,8 @@
     "Rotate Right": "Rotate Right",
     "Information": "Information",
     "Language Dropdown": "Language Dropdown",
-    "Message in": "Message in",
-    "Message from": "Message from",
+    "Message in %(room)s": "Message in %(room)s",
+    "Message from %(user)s": "Message from %(user)s",
     "Create poll": "Create poll",
     "Create Poll": "Create Poll",
     "Edit poll": "Edit poll",
diff --git a/test/components/views/elements/__snapshots__/Pill-test.tsx.snap b/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
index 5cdc5abc1ff..0e589effd95 100644
--- a/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
+++ b/test/components/views/elements/__snapshots__/Pill-test.tsx.snap
@@ -71,7 +71,6 @@ exports[`<Pill> should render the expected pill for a message in another room 1`
         class="mx_Pill mx_EventPill"
         href="https://matrix.to/#/!room1:example.com/$123-456"
       >
-        Message in
         <span
           aria-hidden="true"
           class="mx_BaseAvatar"
@@ -96,7 +95,7 @@ exports[`<Pill> should render the expected pill for a message in another room 1`
         <span
           class="mx_Pill_text"
         >
-          Room 1
+          Message in Room 1
         </span>
       </a>
     </bdi>
@@ -112,7 +111,6 @@ exports[`<Pill> should render the expected pill for a message in the same room 1
         class="mx_Pill mx_EventPill"
         href="https://matrix.to/#/!room1:example.com/$123-456"
       >
-        Message from
         <span
           aria-hidden="true"
           class="mx_BaseAvatar"
@@ -137,7 +135,7 @@ exports[`<Pill> should render the expected pill for a message in the same room 1
         <span
           class="mx_Pill_text"
         >
-          User 1
+          Message from User 1
         </span>
       </a>
     </bdi>
diff --git a/test/components/views/messages/TextualBody-test.tsx b/test/components/views/messages/TextualBody-test.tsx
index d597a0f0d0f..d8f2e288a07 100644
--- a/test/components/views/messages/TextualBody-test.tsx
+++ b/test/components/views/messages/TextualBody-test.tsx
@@ -16,8 +16,9 @@ limitations under the License.
 
 import React from "react";
 import { MatrixClient, MatrixEvent } from "matrix-js-sdk/src/matrix";
-import { MockedObject } from "jest-mock";
+import { mocked, MockedObject } from "jest-mock";
 import { render } from "@testing-library/react";
+import * as prettier from "prettier";
 
 import { getMockClientWithEventEmitter, mkEvent, mkMessage, mkStubRoom } from "../../../test-utils";
 import { MatrixClientPeg } from "../../../../src/MatrixClientPeg";
@@ -28,10 +29,18 @@ import MatrixClientContext from "../../../../src/contexts/MatrixClientContext";
 import { RoomPermalinkCreator } from "../../../../src/utils/permalinks/Permalinks";
 import { MediaEventHelper } from "../../../../src/utils/MediaEventHelper";
 
-const mkRoomTextMessage = (body: string): MatrixEvent => {
+const room1Id = "!room1:example.com";
+const room2Id = "!room2:example.com";
+const room2Name = "Room 2";
+
+interface MkRoomTextMessageOpts {
+    roomId?: string;
+}
+
+const mkRoomTextMessage = (body: string, mkRoomTextMessageOpts?: MkRoomTextMessageOpts): MatrixEvent => {
     return mkMessage({
         msg: body,
-        room: "room_id",
+        room: mkRoomTextMessageOpts?.roomId ?? room1Id,
         user: "sender",
         event: true,
     });
@@ -42,7 +51,7 @@ const mkFormattedMessage = (body: string, formattedBody: string): MatrixEvent =>
         msg: body,
         formattedMsg: formattedBody,
         format: "org.matrix.custom.html",
-        room: "room_id",
+        room: room1Id,
         user: "sender",
         event: true,
     });
@@ -53,12 +62,29 @@ describe("<TextualBody />", () => {
         jest.spyOn(MatrixClientPeg, "get").mockRestore();
     });
 
-    const defaultRoom = mkStubRoom("room_id", "test room", undefined);
+    const defaultRoom = mkStubRoom(room1Id, "test room", undefined);
+    const otherRoom = mkStubRoom(room2Id, room2Name, undefined);
     let defaultMatrixClient: MockedObject<MatrixClient>;
+
+    const defaultEvent = mkEvent({
+        type: "m.room.message",
+        room: room1Id,
+        user: "sender",
+        content: {
+            body: "winks",
+            msgtype: "m.emote",
+        },
+        event: true,
+    });
+
     beforeEach(() => {
         defaultMatrixClient = getMockClientWithEventEmitter({
-            getRoom: () => defaultRoom,
-            getRooms: () => [defaultRoom],
+            getRoom: (roomId: string | undefined) => {
+                if (roomId === room1Id) return defaultRoom;
+                if (roomId === room2Id) return otherRoom;
+                return null;
+            },
+            getRooms: () => [defaultRoom, otherRoom],
             getAccountData: (): MatrixEvent | undefined => undefined,
             isGuest: () => false,
             mxcUrlToHttp: (s: string) => s,
@@ -67,18 +93,13 @@ describe("<TextualBody />", () => {
                 throw new Error("MockClient event not found");
             },
         });
-    });
 
-    const defaultEvent = mkEvent({
-        type: "m.room.message",
-        room: "room_id",
-        user: "sender",
-        content: {
-            body: "winks",
-            msgtype: "m.emote",
-        },
-        event: true,
+        mocked(defaultRoom).findEventById.mockImplementation((eventId: string) => {
+            if (eventId === defaultEvent.getId()) return defaultEvent;
+            return undefined;
+        });
     });
+
     const defaultProps = {
         mxEvent: defaultEvent,
         highlights: [] as string[],
@@ -88,6 +109,7 @@ describe("<TextualBody />", () => {
         permalinkCreator: new RoomPermalinkCreator(defaultRoom),
         mediaEventHelper: {} as MediaEventHelper,
     };
+
     const getComponent = (props = {}, matrixClient: MatrixClient = defaultMatrixClient, renderingFn?: any) =>
         (renderingFn ?? render)(
             <MatrixClientContext.Provider value={matrixClient}>
@@ -100,7 +122,7 @@ describe("<TextualBody />", () => {
 
         const ev = mkEvent({
             type: "m.room.message",
-            room: "room_id",
+            room: room1Id,
             user: "sender",
             content: {
                 body: "winks",
@@ -120,7 +142,7 @@ describe("<TextualBody />", () => {
 
         const ev = mkEvent({
             type: "m.room.message",
-            room: "room_id",
+            room: room1Id,
             user: "bot_sender",
             content: {
                 body: "this is a notice, probably from a bot",
@@ -196,13 +218,42 @@ describe("<TextualBody />", () => {
                 `"Visit <span><bdi><a class="mx_Pill mx_RoomPill" href="https://matrix.to/#/#room:example.com"><div class="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image"></div><span class="mx_Pill_text">#room:example.com</span></a></bdi></span>"`,
             );
         });
+
+        it("should pillify a permalink to a message in the same room with the label »Message from Member«", () => {
+            const ev = mkRoomTextMessage(`Visit https://matrix.to/#/${room1Id}/${defaultEvent.getId()}`);
+            const { container } = getComponent({ mxEvent: ev });
+            const content = container.querySelector(".mx_EventTile_body");
+            expect(
+                prettier.format(content.innerHTML.replace(defaultEvent.getId(), "%event_id%"), {
+                    parser: "html",
+                }),
+            ).toMatchSnapshot();
+        });
+
+        it("should pillify a permalink to an unknown message in the same room with the label »Message«", () => {
+            const ev = mkRoomTextMessage(`Visit https://matrix.to/#/${room1Id}/!abc123`);
+            const { container } = getComponent({ mxEvent: ev });
+            const content = container.querySelector(".mx_EventTile_body");
+            expect(content).toMatchSnapshot();
+        });
+
+        it("should pillify a permalink to an event in another room with the label »Message in Room 2«", () => {
+            const ev = mkRoomTextMessage(`Visit https://matrix.to/#/${room2Id}/${defaultEvent.getId()}`);
+            const { container } = getComponent({ mxEvent: ev });
+            const content = container.querySelector(".mx_EventTile_body");
+            expect(
+                prettier.format(content.innerHTML.replace(defaultEvent.getId(), "%event_id%"), {
+                    parser: "html",
+                }),
+            ).toMatchSnapshot();
+        });
     });
 
     describe("renders formatted m.text correctly", () => {
         let matrixClient: MatrixClient;
         beforeEach(() => {
             matrixClient = getMockClientWithEventEmitter({
-                getRoom: () => mkStubRoom("room_id", "room name", undefined),
+                getRoom: () => mkStubRoom(room1Id, "room name", undefined),
                 getAccountData: (): MatrixEvent | undefined => undefined,
                 getUserId: () => "@me:my_server",
                 getHomeserverUrl: () => "https://my_server/",
diff --git a/test/components/views/messages/__snapshots__/TextualBody-test.tsx.snap b/test/components/views/messages/__snapshots__/TextualBody-test.tsx.snap
index 28fe92c48b4..238826978e8 100644
--- a/test/components/views/messages/__snapshots__/TextualBody-test.tsx.snap
+++ b/test/components/views/messages/__snapshots__/TextualBody-test.tsx.snap
@@ -88,7 +88,6 @@ exports[`<TextualBody /> renders formatted m.text correctly pills appear for eve
             class="mx_Pill mx_EventPill"
             href="https://matrix.to/#/!ZxbRYPQXDXKGmDnJNg:example.com/$16085560162aNpaH:example.com?via=example.com"
           >
-            Message in
             <img
               alt=""
               aria-hidden="true"
@@ -100,7 +99,7 @@ exports[`<TextualBody /> renders formatted m.text correctly pills appear for eve
             <span
               class="mx_Pill_text"
             >
-              room name
+              Message in room name
             </span>
           </a>
         </bdi>
@@ -240,3 +239,71 @@ exports[`<TextualBody /> renders formatted m.text correctly pills get injected c
   </span>
 </span>
 `;
+
+exports[`<TextualBody /> renders plain-text m.text correctly should pillify a permalink to a message in the same room with the label »Message from Member« 1`] = `
+"Visit
+<span
+  ><bdi
+    ><a
+      class="mx_Pill mx_EventPill"
+      href="https://matrix.to/#/!room1:example.com/%event_id%"
+      ><img
+        class="mx_BaseAvatar mx_BaseAvatar_image"
+        src="mxc://avatar.url/image.png"
+        style="width: 16px; height: 16px"
+        alt=""
+        data-testid="avatar-img"
+        aria-hidden="true"
+      /><span class="mx_Pill_text">Message from Member</span></a
+    ></bdi
+  ></span
+>
+"
+`;
+
+exports[`<TextualBody /> renders plain-text m.text correctly should pillify a permalink to an event in another room with the label »Message in Room 2« 1`] = `
+"Visit
+<span
+  ><bdi
+    ><a
+      class="mx_Pill mx_EventPill"
+      href="https://matrix.to/#/!room2:example.com/%event_id%"
+      ><img
+        class="mx_BaseAvatar mx_BaseAvatar_image"
+        src="mxc://avatar.url/room.png"
+        style="width: 16px; height: 16px"
+        alt=""
+        data-testid="avatar-img"
+        aria-hidden="true"
+      /><span class="mx_Pill_text">Message in Room 2</span></a
+    ></bdi
+  ></span
+>
+"
+`;
+
+exports[`<TextualBody /> renders plain-text m.text correctly should pillify a permalink to an unknown message in the same room with the label »Message« 1`] = `
+<span
+  class="mx_EventTile_body"
+  dir="auto"
+>
+  Visit 
+  <span>
+    <bdi>
+      <a
+        class="mx_Pill mx_EventPill"
+        href="https://matrix.to/#/!room1:example.com/!abc123"
+      >
+        <div
+          class="mx_Pill_LinkIcon mx_BaseAvatar mx_BaseAvatar_image"
+        />
+        <span
+          class="mx_Pill_text"
+        >
+          Message
+        </span>
+      </a>
+    </bdi>
+  </span>
+</span>
+`;
diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts
index add949b12dc..ad00f79f056 100644
--- a/test/test-utils/test-utils.ts
+++ b/test/test-utils/test-utils.ts
@@ -537,7 +537,7 @@ export function mkStubRoom(
         } as unknown as RoomState,
         eventShouldLiveIn: jest.fn().mockReturnValue({}),
         fetchRoomThreads: jest.fn().mockReturnValue(Promise.resolve()),
-        findEventById: (_: string) => undefined as MatrixEvent | undefined,
+        findEventById: jest.fn().mockReturnValue(undefined),
         findPredecessor: jest.fn().mockReturnValue({ roomId: "", eventId: null }),
         getAccountData: (_: EventType | string) => undefined as MatrixEvent | undefined,
         getAltAliases: jest.fn().mockReturnValue([]),