-
Notifications
You must be signed in to change notification settings - Fork 399
MSC4027: Propose method of specifying custom images in reactions #4027
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
d4ff1c7
cdfe089
c86338a
75473cc
5424ea8
2ac5328
badc1ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mscbot concern Needs to be reviewed against other custom emoji/sticker MSCs There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,111 @@ | ||
# 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. | ||
|
||
Detecting if the `key` is an MXC URI can be as sophisticated as the client | ||
wants, but this proposal recommends checking if the string starts with | ||
`mxc://`. | ||
sumnerevans marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. | ||
|
||
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:" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we enforce that shortcode's must begin and end with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A "must" snuck back in on line 33. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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. | ||
|
||
## 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` key should be | ||
prefixed with `com.beeper.msc4027.`. | ||
|
||
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. |
Uh oh!
There was an error while loading. Please reload this page.