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

MSC4027: Propose method of specifying custom images in reactions #4027

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
138 changes: 138 additions & 0 deletions proposals/4027-custom-images-in-reactions.md
sumnerevans marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

@mscbot concern Needs to be reviewed against other custom emoji/sticker MSCs

Copy link
Member

Choose a reason for hiding this comment

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

I think in particular, this MSC sets up grammar for emoji shortcodes in Matrix, which we'll want to double-check aligns with whatever MSC defines sharable image packs.

@turt2live did you have anything else in mind? The rest is fairly reaction-specific IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Shortcodes and feature as a whole are conflicting, imo. Specific impact to be determined once the remainder of the stack is looked at :)

(I plan to do this over the next 2ish weeks, barring other commitments - will hand over ~next week if things go poorly on my side)

Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
# MSC4027 Custom Images in Reactions

One of the most desired features within the Matrix ecosystem is the ability to
react to messages with custom images. This feature is especially requested by
users who come from Slack and Discord where this functionality is one of the
main ways that the culture of a chat rooms develops.

There is an existing proposal to
[render image data in reactions (MSC3746)](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/),
but it has had little attention recently and has the flaw of not being conducive
to deduplication (either on the client or server). Sorunome proposed a
modification to that MSC to
[use the MXC URI as the key](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/files#r866285147)
which this proposal adopts.

This proposal is meant to replace
[MSC3746](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/) and is
additionally intended to document the behaviour of existing clients and bridges.

Like
[MSC3746](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/), this
MSC does not propose a mechanism for providing a list of available images.

## Proposal

This proposal suggests two changes to events with the `m.annotation` relation.

1. If the `key` of an `m.annotation` relation is an MXC URI of an image, clients
should render the referenced image instead of the key text.

2. When the annotation's key is an MXC URI, a new (optional) `shortcode` key can
be added to the content of the event with a textual name for the image. This
sumnerevans marked this conversation as resolved.
Show resolved Hide resolved
field must be a string and should start and end with the `:` (colon)
Copy link
Member

Choose a reason for hiding this comment

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

@mscbot concern Requiring two : characters in every shortcode string.

Could you explain why we should encourage clients to put :s in every shortcode instead of restricting : from being put in the string?

I know we can't enforce it over federation, but the spec could at least enforce it in the Client-Server API. I think the occasional extra : appearing in clients is worth the bandwidth saving.

For bridges, if they connect to a network that includes : on the ends of their custom emote shortcodes (or another character), then the bridge can just strip it, no?

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

character.

This shortcode should be shown by clients in situations such as hovering over
the annotation, as alt-text, or if the client does not support rendering
images.

The `shortcode` must have a length of less than or equal to 100 bytes
(including the colons at the beginning and end of the string). This
restriction must be enforced by servers when sending reactions, but servers
should not reject events coming across federation due to having too many
bytes in the `shortcode` field. Servers may still opt to locally redact
events having too many bytes in the `shortcode` field.

Example custom image reaction event content

```json
"content": {
"m.relates_to": {
"rel_type": "m.annotation",
"event_id": "$abcdefg",
"key": "mxc://matrix.org/VOczFYqjdGaUKNwkKsTjDwUa"
},
"shortcode": ":partyparrot:"
Copy link
Member

Choose a reason for hiding this comment

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

Should we enforce that shortcode's must begin and end with :? I think it'd be confusing to users if some clients started picking some other character.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a "should" for that. It's not enforceable other than by convention.

Copy link
Member

Choose a reason for hiding this comment

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

A "must" snuck back in on line 33.

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 type of the field is fairly easily enforceable by servers. We are already saying that they should verify the length. (Obviously if a rouge server doesn't validate these fields, we could have "bad" events come across federation.)

I think the question here is what part of the verification is "should" vs "must"? Right now, the servers "must" verify type and length of the shortcode, but the leading and trailing : is "should".

Maybe everything should be "must" and force servers to verify all three things, but leave it up to the server implementation if they want to locally redact invalid events that come across federation?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would eliminate confusion in implementations for all three to be a MUST on the Client-Server API.

leave it up to the server implementation if they want to locally redact invalid events that come across federation?

I would say yes, though I imagine most implementations would not redact such events - instead just allowing any UI bugs in the client that would surface.

}
```

## Potential issues
sumnerevans marked this conversation as resolved.
Show resolved Hide resolved

### Clients rendering the MXC URI as text

The biggest disadvantage is that clients that do not support rendering custom
reactions will render the MXC URI as text. However, this is already problematic
because many bridges and clients already support this MSC, and users likely
already encounter this.

### Un-renderable image referenced in the `key`

The MXC URI could specify an asset that either does not exist, or is not a
renderable image. Clients can opt to render the `shortcode` in these situations,
or some placeholder/error image, or just opt to render the full key.

### Multiple MXC URIs can refer to identical files

Consider two distinct MXC URIs which both refer to a file with identical
contents. If the URIs are used as reaction keys on a given message,
deduplication will not happen as the keys are different. This could cause
confusion as the two reactions would be visually identical despite the keys
being different.

A similar problem exists already in some clients with Unicode emojis because
there is significant inconsistency as to whether skin tone indicators and other
un-renderable characters should be included in the reaction keys generated by
clients. As such, this proposal does not materially worsen the current
situation.

Clients can opt to compare the file contents of the images, but this should not
be required as it would complicate rendering significantly.

Future MSCs may provide a way to share a set of custom reaction images available
in a room/space. This will make it so that users can share custom reaction
images reducing the likelihood of two users uploading the same image under two
different MXC URIs and reacting with them.

## Alternatives

### Use the shortcode as the `key`

This is what was proposed by
[MSC3746](https://github.com/matrix-org/matrix-spec-proposals/pull/3746/). The
problem with this is that there could possibly be multiple distinct images with
the same shortcode. Reactions are only deduplicated based on `key`, so clients
and servers would group these distinct reactions together which is undesirable.

### Put the `shortcode` as a key within `m.relates_to`

Instead of being at the root of the `content` dictionary, the `shortcode` value
could be included within `m.relates_to`. This is the wrong place to put this
value because `m.relates_to` is meant to only contain information pertaining to
the relationship between events, not information about the reaction event
itself.

## Security considerations

### Image is unencrypted

Reaction events are not encrypted, and so the MXC URI referenced by the key
would have to be an unencrypted image. However, this is probably not a problem
for the following reasons:

- Custom reactions are most likely not sensitive information.

- Users are already able to upload unencrypted content into encrypted rooms, so
this does not introduce any leakage that was not previously possible.

- Clients can add UX to indicate to users that the reaction images are not
encrypted.

## Unstable prefix

Until this proposal is merged into the spec, the `shortcode` unstable field name
should be `com.beeper.reaction.shortcode`.

An unstable prefix for the `key` in `m.relates_to` is not necessary as the spec
already allows arbitrary data to be used as the `key`. This MSC merely adds
extra meaning to a specific class of key.
Loading