From 3e2b727f6beb42fe9eb595ba8d19c62b34b7dfe5 Mon Sep 17 00:00:00 2001 From: Michael Weimann <michaelw@matrix.org> Date: Tue, 21 Mar 2023 13:16:22 +0100 Subject: [PATCH 1/2] Set pill avatar always on left side --- res/css/views/elements/_Pill.pcss | 6 ----- src/components/views/elements/Pill.tsx | 32 +++++++++++++------------- src/i18n/strings/en_EN.json | 4 ++-- 3 files changed, 18 insertions(+), 24 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..41161ca3e14 100644 --- a/src/components/views/elements/Pill.tsx +++ b/src/components/views/elements/Pill.tsx @@ -116,35 +116,33 @@ 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]; + avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />; + pillText = _t("Message from %(user)s", { + user: text, + }); } 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 +159,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/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", From f3a82b5f8dbccfa2cd99b72205b23e359e833873 Mon Sep 17 00:00:00 2001 From: Michael Weimann <michaelw@matrix.org> Date: Tue, 21 Mar 2023 14:21:19 +0100 Subject: [PATCH 2/2] Implement pill fallback for unknown events in same room --- src/components/views/elements/Pill.tsx | 19 ++-- src/hooks/usePermalink.ts | 8 +- .../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 +- 6 files changed, 163 insertions(+), 34 deletions(-) diff --git a/src/components/views/elements/Pill.tsx b/src/components/views/elements/Pill.tsx index 41161ca3e14..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, @@ -130,10 +132,15 @@ export const Pill: React.FC<PillProps> = ({ type: propType, url, inMessage, room break; case PillType.EventInSameRoom: { - avatar = <PillMemberAvatar shouldShowPillAvatar={shouldShowPillAvatar} member={member} />; - pillText = _t("Message from %(user)s", { - user: text, - }); + 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: 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/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 05e86a3b8f4..865ab0836db 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -535,7 +535,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([]),