Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Handle newlines in user pills #11166

Merged
merged 9 commits into from
Jul 5, 2023
6 changes: 3 additions & 3 deletions src/Markdown.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { logger } from "matrix-js-sdk/src/logger";

import { linkify } from "./linkify-matrix";

const ALLOWED_HTML_TAGS = ["sub", "sup", "del", "u"];
const ALLOWED_HTML_TAGS = ["sub", "sup", "del", "u", "br", "br/"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

presumably br/ is now redundant here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd say not. This is actually a bit broader than what we need. The tags listed here are allowed anywhere in a message, not just inside user pills. So if we remove br/, user pills would still work but you could not use <br/> in a message even though you can us <br> which feels a bit non-intuitive to me.


// These types of node are definitely text
const TEXT_NODES = ["text", "softbreak", "linebreak", "paragraph", "document"];
Expand All @@ -36,8 +36,8 @@ function isAllowedHtmlTag(node: commonmark.Node): boolean {
return true;
}

// Regex won't work for tags with attrs, but we only
// allow <del> anyway.
// Regex won't work for tags with attrs, but the tags we allow
// shouldn't really have any anyway.
const matches = /^<\/?(.*)>$/.exec(node.literal);
if (matches && matches.length == 2) {
const tag = matches[1];
Expand Down
24 changes: 12 additions & 12 deletions src/editor/serialize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,20 @@ export function mdSerialize(model: EditorModel): string {
case Type.PillCandidate:
case Type.AtRoomPill:
return html + part.text;
case Type.RoomPill:
case Type.RoomPill: {
const url = makeGenericPermalink(part.resourceId);
// Escape square brackets and backslashes
// Here we use the resourceId for compatibility with non-rich text clients
// See https://github.com/vector-im/element-web/issues/16660
return (
html +
`[${part.resourceId.replace(/[[\\\]]/g, (c) => "\\" + c)}](${makeGenericPermalink(
part.resourceId,
)})`
);
case Type.UserPill:
return (
html +
`[${part.text.replace(/[[\\\]]/g, (c) => "\\" + c)}](${makeGenericPermalink(part.resourceId)})`
);
const title = part.resourceId.replace(/[[\\\]]/g, (c) => "\\" + c);
return html + `[${title}](${url})`;
}
case Type.UserPill: {
const url = makeGenericPermalink(part.resourceId);
// Escape square brackets and backslashes; convert newlines to HTML
const title = part.text.replace(/[[\\\]]/g, (c) => "\\" + c).replace(/\n/g, "<br>");
return html + `[${title}](${url})`;
}
}
}, "");
}
Expand Down
8 changes: 8 additions & 0 deletions test/editor/deserialize-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,14 @@ describe("editor/deserialize", function () {
expect(parts[1]).toStrictEqual({ type: "user-pill", text: "Alice]", resourceId: "@alice:hs.tld" });
expect(parts[2]).toStrictEqual({ type: "plain", text: "!" });
});
it("user pill with displayname containing linebreak square bracket", function () {
const html = 'Hi <a href="https://matrix.to/#/@alice:hs.tld">Alice<br/>123</a>!';
Johennes marked this conversation as resolved.
Show resolved Hide resolved
const parts = normalize(parseEvent(htmlMessage(html), createPartCreator()));
expect(parts.length).toBe(3);
expect(parts[0]).toStrictEqual({ type: "plain", text: "Hi " });
expect(parts[1]).toStrictEqual({ type: "user-pill", text: "Alice123", resourceId: "@alice:hs.tld" });
expect(parts[2]).toStrictEqual({ type: "plain", text: "!" });
});
it("room pill", function () {
const html = 'Try <a href="https://matrix.to/#/#room:hs.tld">#room:hs.tld</a>?';
const parts = normalize(parseEvent(htmlMessage(html), createPartCreator()));
Expand Down
7 changes: 6 additions & 1 deletion test/editor/serialize-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ describe("editor/serialize", function () {
const html = htmlSerializeIfNeeded(model, {});
expect(html).toBe('<a href="https://matrix.to/#/@user:server">Displayname]</a>');
});
it("displaynames containing a newline work", function () {
const pc = createPartCreator();
const model = new EditorModel([pc.userPill("Display\nname", "@user:server")], pc);
const html = htmlSerializeIfNeeded(model, {});
expect(html).toBe('<a href="https://matrix.to/#/@user:server">Display<br>name</a>');
Johennes marked this conversation as resolved.
Show resolved Hide resolved
});
it("escaped markdown should not retain backslashes", function () {
const pc = createPartCreator();
const model = new EditorModel([pc.plain("\\*hello\\* world")], pc);
Expand Down Expand Up @@ -96,7 +102,6 @@ describe("editor/serialize", function () {
const html = htmlSerializeIfNeeded(model, { useMarkdown: false });
expect(html).toBe("\\*hello\\* world &lt; hey world!");
});

it("plaintext remains plaintext even when forcing html", function () {
const pc = createPartCreator();
const model = new EditorModel([pc.plain("hello world")], pc);
Expand Down