Skip to content
This repository was 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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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", function () {
const html = 'Hi <a href="https://matrix.to/#/@alice:hs.tld">Alice<br>123</a>!';
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>');
});
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