Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC3765: Rich text in room topics #3765

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Changes from 17 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
128 changes: 128 additions & 0 deletions proposals/3765-rich-room-topics.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
# MSC3765: Rich text in room topics
Copy link
Member

Choose a reason for hiding this comment

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

@alphapapa says:

On one hand, I can see some elegance in repurposing room topics for general-purpose, long-term room reference information. OTOH, it seems like overloading the purpose of topics with what, in other systems, would go in "pinned" topics or messages, or a wiki, etc.

So IMHO I would consider implementing support for pinned messages/events before overloading topics like this. It would seem relatively straightforward for a room's state to have a list of pinned events, which could be sent in initial sync by the server or be retrieved manually by clients. Clients could then display these pinned events in a room's timeline view, optionally hiding them, compressing them, etc. And the pinned events could be edited by room moderators using existing event-editing tools. (Forgive me if there's already a proposal for something like that.)

Copy link
Member

Choose a reason for hiding this comment

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

@Johennes replies:

Interesting idea. Pinned events seem to already exist. However, in their current form, these are not fit to be used for what you describe because, depending on room settings, users joining the room after the events were sent could be unable to see them.


## Problem
Johennes marked this conversation as resolved.
Show resolved Hide resolved

Topics are a central piece of room meta data and usually made easily
accessible to room members in clients. As a result, room administrators
often extend the use of topics to collect helpful peripheral information
that is related to the room’s purpose. Most commonly these are links to
external resources. At the moment, topics are limited to [plain text]
which, depending on the number and length of URLs and other content,
easily gets inconvenient to consume and calls for richer text formatting
options.

## Proposal

Drawing from extensible events as described in [MSC1767], a new content
block `m.topic` is defined, which wraps an `m.text` content block that
allows representing the room topic in different mime types. In current
room versions, this content block is added to the content of [`m.room.topic`]
events as shown below[^1].

```json5
{
"type": "m.room.topic",
"state_key": "",
"content": {
"m.topic": {
"m.text": [{
"body": "All about **pizza** | [Recipes](https://recipes.pizza.net)"
}, {
"mimetype": "text/html",
"body": "All about <b>pizza</b> | <a href=\"https://recipes.pizza.net\">Recipes</a>"
}]
Comment on lines +28 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Aren't these the wrong way round? According to MSC1767, the first supported representation should be used, so nobody will ever use the html representation.

Copy link
Member

Choose a reason for hiding this comment

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

(clients can also optimize and pick a preferred represenation, but indeed the preference should be to order them. I remain unconvinced that MSC1767 picked the right order here though, because everyone (including me) keeps putting plaintext first)

},
"topic": "All about **pizza** | [Recipes](https://recipes.pizza.net)"
},
...
}
```

Details of how `m.text` works may be found in [MSC1767] and are not
repeated here.
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

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

I hadn't really expected m.room.topic events to be the way that the m.text stuff makes it into the spec, but I guess that's ok.


The wrapping `m.topic` content block is similar to `m.caption` for file
uploads as defined in [MSC3551]. It avoids clients accidentally rendering
the topic state event as a room message.
Comment on lines +45 to +46
Copy link
Member

Choose a reason for hiding this comment

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

It avoids clients accidentally rendering the topic state event as a room message.

Is this actually necessary?

It seems to me that the reason for m.caption in MSC3551 is quite different to the reason for it here: in that case, m.text at the top level has a very different meaning to m.text within m.caption, whereas here there is no ambiguity.

This might be a good thing to do anyway, for consistency for example, but I'm unconvinced this paragraph captures a good reason for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extra wrapping block was based off of @turt2live's comment from an earlier review. I don't feel strongly either way but am curious what Travis thinks.

Copy link
Member

Choose a reason for hiding this comment

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

I tripped over this too when reviewing it just now. My initial reaction was the same as richvdh's: state events don't get rendered as fallbacks, plus clients know to special-case topics already - plus the contents of the topic is semantically a top-level 'm.text' (unlike m.caption, which is effectively describing a nested event). If topics actually required some more custom topic-specific datatype (e.g. show_at_all_times: true or something) then perhaps one might wrap it... but in practice, I can't think of any plausible customisation which couldn't/shouldn't be pulled in as another mix-in.

The only argument I can see for it is consistency with m.caption and other places where you might want to explicitly say "don't fall back to displaying m.text if you don't recognise the event type. even if it's a state event". Or possibly futureproofing for additional topic-specific keys we just haven't thought of yet.

If it were me, I'd probably go ahead without the m.topic wrapper and just have it as m.text. But my strength of feeling here is about 6/10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the absence of dispute so far, I have opened #4251 to remove the wrapping m.topic content block. I'll just need somebody to merge it for me, assuming this looks alright.


It is recommended that clients always include a plain text variant when
richvdh marked this conversation as resolved.
Show resolved Hide resolved
sending `m.topic` events. This prevents bad UX in situations where a plain
richvdh marked this conversation as resolved.
Show resolved Hide resolved
text topic is sufficient such as the public rooms directory.
Johennes marked this conversation as resolved.
Show resolved Hide resolved

Additionally, clients should duplicate the plain text topic into the existing
`topic` field for backwards compatibility with clients that don't support
`m.topic` yet. This also helps prevent inconsistencies since such clients
are likely to delete the `m.topic` content block when updating `m.room.topic`
themselves.

In order to prevent formatting abuse in room topics, clients are
encouraged to limit the length of topics during both entry and display,
for instance, by capping the number of displayed lines. Additionally,
clients should ignore things like headings and enumerations (or format them
as regular text). A future MSC may introduce a mechanism to capture extended
multiline details that are not suitable for room topics in a separate field
or event type.

On the server side, any logic that currently operates on the `topic` field is
updated to use the `m.topic` content block instead.
Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

Am I right in understanding that the next four paragraphs are examples of this change? If so, it would be good to make that a little clearer:

Suggested change
On the server side, any logic that currently operates on the `topic` field is
updated to use the `m.topic` content block instead.
On the server side, any logic that currently operates on the `topic` field is
updated to use the `m.topic` content block instead. For example:

... and make the following paragraphs bullet points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually meant to be a conclusive list. I hope I didn't miss anything. Maybe ending with a colon (without "For example") and making the next paragraphs bullets would be clearer?

Copy link
Member

Choose a reason for hiding this comment

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

yup, sgtm

Copy link
Contributor Author

@Johennes Johennes Dec 18, 2024

Choose a reason for hiding this comment

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

Have captured it into https://github.com/matrix-org/matrix-spec-proposals/pull/3765/files#r1890019768 which I'll need you to push the button on once more. Sorry for the hassle.


In [`/_matrix/client/v3/createRoom`], the `topic` parameter causes `m.room.topic`
to be written with a `text/plain` mimetype in `m.topic`. If at the same time an
richvdh marked this conversation as resolved.
Show resolved Hide resolved
`m.room.topic` event is supplied in `initial_state`, it is overwritten entirely.
A future MSC may generalize the `topic` parameter to allow specifying other mime
types without `initial_state`.

In [`GET /_matrix/client/v3/publicRooms`], [`GET /_matrix/federation/v1/publicRooms`]
and their `POST` siblings, the `topic` response field is read from the
richvdh marked this conversation as resolved.
Show resolved Hide resolved
`text/plain` mimetype of `m.topic` if it exists or omitted otherwise.
A plain text topic is sufficient here because this data is commonly
only displayed to users that are *not* a member of the room yet. These
users don't commonly have the same need for rich room topics as users
who already reside in the room. A future MSC may update these endpoints
to support rich text topics.

The same logic is applied to [`/_matrix/client/v1/rooms/{roomId}/hierarchy`]
and [`/_matrix/federation/v1/hierarchy/{roomId}`].

In [server side search], the `room_events` category is expanded to search
over the `m.text` content block of `m.room.topic` events.

## Potential issues

None.

## Alternatives
Johennes marked this conversation as resolved.
Show resolved Hide resolved

The combination of `format` and `formatted_body` currently utilised to
enable HTML in `m.room.message` events could be generalised to
`m.room.topic` events. However, this would only allow for a single
format in addition to plain text and is a weaker form of reuse than
described in the introductory section of [MSC1767].

## Security considerations

Allowing HTML in room topics is subject to the same security
considerations that apply to HTML in room messages. In particular,
topics are already included in the content that clients should [sanitise]
for unsafe HTML.

## Unstable prefix

While this MSC is not considered stable, `m.topic` should be referred to
as `org.matrix.msc3765.topic`. Note that extensible events and content
blocks might have their own prefixing requirements.
Comment on lines +112 to +113
Copy link
Member

Choose a reason for hiding this comment

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

Note that extensible events and content blocks might have their own prefixing requirements.

I don't really know what this means, as it pertains to this MSC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is stolen from MSC3381. It was originally intended to make m.text into org.matrix.msc1767.text. However, I suppose we don't need this anymore because MSC1767 has since been accepted?


[^1]: A future MSC may discuss how to adopt the `m.topic` content block in
new room versions which support extensible events.

[plain text]: https://spec.matrix.org/v1.12/client-server-api/#mroomtopic
[MSC1767]: https://github.com/matrix-org/matrix-spec-proposals/pull/1767
[MSC3551]: https://github.com/matrix-org/matrix-spec-proposals/pull/3551
[sanitise]: https://spec.matrix.org/v1.12/client-server-api/#security-considerations
[server side search]: https://spec.matrix.org/v1.12/client-server-api/#server-side-search
[`m.room.topic`]: https://spec.matrix.org/v1.12/client-server-api/#mroomtopic
[`/_matrix/client/v1/rooms/{roomId}/hierarchy`]: https://spec.matrix.org/v1.12/client-server-api/#get_matrixclientv1roomsroomidhierarchy
[`/_matrix/client/v3/createRoom`]: https://spec.matrix.org/v1.12/client-server-api/#post_matrixclientv3createroom
[`/_matrix/federation/v1/hierarchy/{roomId}`]: https://spec.matrix.org/v1.12/server-server-api/#get_matrixfederationv1hierarchyroomid
[`GET /_matrix/client/v3/publicRooms`]: https://spec.matrix.org/v1.12/client-server-api/#get_matrixclientv3publicrooms
[`GET /_matrix/federation/v1/publicRooms`]: https://spec.matrix.org/v1.12/server-server-api/#get_matrixfederationv1publicrooms
Loading