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

Allow sending free-form reactions #6628

Closed
wants to merge 9 commits into from

Conversation

tadzik
Copy link

@tadzik tadzik commented Aug 17, 2021

Fixes element-hq/element-web#19409

This unleashes user creativity and allows them to react in whatever shape or form they desire. Free-form reactions don't get stored in the "Recently Used" list since they aren't emojis and I wasn't sure what that'll break – though they come as strings so it should be fine, but on the note of encouraging creativity perhaps it shouldn't be stored for later anyway :)

This is basically My First React[tm], so let me know if something can be made less awful.
reactions


Here's what your changelog entry will look like:

✨ Features

Preview: https://616996feafffde0805ad1da9--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@tadzik tadzik requested a review from a team as a code owner August 17, 2021 09:35
@SimonBrandner
Copy link
Contributor

Thank you for your contribution! I think this is going to need a product review to decide if this is something we want

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

LGTM codewise. Thanks!

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

EmojiPicker is actually used in two different places: the emoji picker in the composer, and the reaction picker in the timeline. Currently this change applies to both of them, though presumably it should only apply to the reaction picker.

@tadzik
Copy link
Author

tadzik commented Aug 17, 2021

EmojiPicker is actually used in two different places: the emoji picker in the composer, and the reaction picker in the timeline. Currently this change applies to both of them, though presumably it should only apply to the reaction picker.

Fixed in 7d335a5. I struggled with the naming here a little, and settled for allowUnlisted (as in: unlisted emojis), since it's an EmojiPicker after all, and allowing plaintext in emoji picker sounded a bit iffy.

@turt2live turt2live requested review from a team and removed request for a team August 17, 2021 14:47
@@ -27,6 +27,7 @@ import Preview from "./Preview";
import QuickReactions from "./QuickReactions";
Copy link
Contributor

Choose a reason for hiding this comment

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

This encourages creating more detailed, personalized reactions, so maybe there should be a reminder in the menu that reactions are never encrypted.

@t3chguy t3chguy added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Sep 1, 2021
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM

@@ -35,6 +36,7 @@ export const EMOJIS_PER_ROW = 8;
const ZERO_WIDTH_JOINER = "\u200D";

interface IProps {
allowUnlisted?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

allowFreeform maybe?

Copy link
Author

Choose a reason for hiding this comment

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

I was on the fence here – "freeform emojis" are not a thing (and this is an EmojiPicker), but at the same time "unlisted emojis" are not the point here – it allows for just not using emojis at all (though technically that also allows for not-yet-supported, thus "unlisted" emojis). I'm indifferent, we can go either way.

@novocaine
Copy link
Contributor

Hey @tadzik - nice PR.

Can you create a T-Enhancement issue in element-web linked to this where it can be discussed with the design and product teams please?

@tadzik
Copy link
Author

tadzik commented Oct 15, 2021

Hey @tadzik - nice PR.

Can you create a T-Enhancement issue in element-web linked to this where it can be discussed with the design and product teams please?

Yep: done in element-hq/element-web#19409. Thanks!

@nadonomy nadonomy self-assigned this Oct 19, 2021
@turt2live turt2live requested a review from a team February 3, 2022 04:18
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

echoing approval to fix internal stats

Copy link
Contributor

@kittykat kittykat left a comment

Choose a reason for hiding this comment

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

From a product POV: while we don't want to add maintenance burden, this is a feature already supported by other apps and the MSC2677 currently implies that emojis are just one type of reaction that could be sent.

We should add the feature because:

  • MSC2677 supports reactions with long keys, i.e. text reactions
  • This feature is already available through other apps and our users can see those reactions
  • It is already possible to send freeform reactions through /devtools and bots from Element, so at the moment the feature exists but is obscure
  • Element users can already "+1" a freeform reaction on any platform by clicking it

Product considerations:

  • If we're adding this feature on web, we should add it on mobile as well to maintain feature parity

Design considerations:

  • How might we allow users to send freeform reactions
  • How might we display long reactions in a user friendly way? (I can't see any upper limit set by the MSC)
  • How might reporting of reactions look like?
  • How might removal of reactions look like for room moderators?

Moderation considerations to address:

  • Should reactions be reportable or can they be reported using the existing mechanism for reporting messages? @jimmackenzie maybe you can help with input on this one?

Please reach out to @frakic before merging the PR so that he can test the build and add test cases for RC testing.

@jimmackenzie
Copy link

I've used freeform reactions elsewhere, and loved them. From a safety perspective:

  • Should reactions be reportable?
    Yes. Emphatically yes.

  • Can they be reported using the existing mechanism for reporting messages?
    Kinda. Any event can be reported, but clients don't typically expose the report option for reactions, only for messages. Admins would need to use the Event Context API to fetch surrounding events for the reported message to try and grab the surrounding reactions. That is hit-and-miss, so I wouldn't rely on that alone.

Recommendation: We should make it easy for clients to report reaction events as part of this change.

  • Do existing moderation tools work for redacting abusive reactions?
    Yes, but not smoothly. Safety teams can use the redactions API to redact the reaction event, but as above, that is currently fiddly. Mjolnir and other moderation tools issue redactions by using an event's permalink which clients don't expose for reactions currently. We could update those tools to work with the event id instead. I'll get someone to investigate.

Recommendation: Update clients to allow room moderators to remove reactions. T&S to update safety tooling to handle those redactions as well (either via freshly-exposed event permalink for reactions, or via the reaction event-id).

  • Should there be an upper limit to key length?
    Yes. While clients can limit what they display (and likely should!) people will try to post the entire Bee Movie script — or less benign content — into free-form text reactions. Prior art I've seen for freeform reactions used a 16 character limit.

Recommendation: Establish an upper limit for reaction length in the spec + implement in the clients.

@Mikaela
Copy link

Mikaela commented May 11, 2022

How might we allow users to send freeform reactions

Other clients, at least FluffyChat and Nheko allow you to reply to a message with /react whatever to add reaction with text whatever

How might we display long reactions in a user friendly way? (I can't see any upper limit set by the MSC)

FluffyChat cuts the reaction at some point (I am not sure what logic it uses) after which the full reaction text becomes visible by long touching (mobile) which also shows who have added that reaction. I think Element Web could make it visible on hover.

sumnerevans added a commit to sumnerevans/matrix-react-sdk that referenced this pull request May 13, 2022
* Render Beeper-style image emoji
* Allow sending free-form reactions
  (matrix-org#6628)
* Notify even when not focused
sumnerevans added a commit to sumnerevans/matrix-react-sdk that referenced this pull request May 13, 2022
* Render Beeper-style image emoji
* Allow sending free-form reactions
  (matrix-org#6628)
* Notify even when not focused
@Johennes
Copy link
Contributor

This has been blocked for a very long time now. Given it's obvious user value, I propose us to be more pragmatic in getting it landed and not let perfect be the enemy of good.

Summarizing the outstanding concerns that I've found:

  1. @timokoesters mentioned that reactions are not encrypted and we might want to add cautionary copy to explain that to the free-form reaction UI. This seems reasonable and likely only a small change so I propose that we include it before landing.
  2. @jimmackenzie raised concerns around reporting reactions. These are valid but I don't think they should block this change from landing. The limitations affect all reactions, not only free-form ones, and it is possible to report individual reaction events after enabling hidden events in the timeline.
  3. @jimmackenzie also recommends to limit the length of free-form reactions. Judging from the screenshots in Allow free-form reactions element-hq/element-web#19409 (comment), we already truncate reactions in the UI. Beyond that, this strikes me as no more an abuse vector than sending any event – all of which are capped by the maximum event size. Consequently, I don't think we should block on this either.
  4. @kittykat mentions that the font style of free-form reactions doesn't match other reactions. This seems like an obvious inconsistency and, again, is likely an easy fix. So I suggest we include it in this change.

@daniellekirkwood are you in agreement with the above? If so, could you help figure out the language for 1.?

@tadzik could you look inot fixing 4.?

@andybalaam
Copy link
Contributor

I agree with all @Johennes said above, and I would further comment that the problems with length and reporting of reactions already exist in Element because we already display free-form reactions.

@daniellekirkwood
Copy link

Hi there!

I really appreciate the hard-work and thought that's gone into this issue, along with the obvious passion that y'all feel for making this product more flexible and delightful.

On behalf of the product team, I'm going to decline this PR.

Here's a brief overview of why:

  • Reactions are not encrypted: Allowing our users to type in any text doesn't feel safe (warning or no warning).
    • There are users that wouldn't want this feature at all because of this, and managing another level of complexity here just isn't something we're able to support right now.
  • The design of adding free form reactions does not feel polished. Unfortunately this lands 'on my desk' during a period of time where we're really focussing on removing features and complexity from our product in order to raise the quality and polish throughout.
    • Another design consideration here is the need for this feature. While it's cool and something that the spec supports, there are other ways to achieve the same (or similar) thing in the product (replies for example) and therefore this particular addition is not critical for us to be successful
  • Being able to write anything here opens a spam vector that we have no way to handle (either technically or resource-wise at the minute)

Apologies that I'm here with bad news, friends.

Hope you have a great day 💛

@t3chguy
Copy link
Member

t3chguy commented Jun 20, 2023

Being able to write anything here opens a spam vector that we have no way to handle (either technically or resource-wise at the minute)

It really doesn't. It is far more effort to do this by hand than using the Element Web console or using curl, both of these latter options can be automated in a while loop to go infinitely if you wish to spam. Element web still shows these, so the vector has been open for years.

@daniellekirkwood
Copy link

We'd be making it more accessible for those users that are not technically able to exploit it in the way you've described.

@bkil
Copy link
Contributor

bkil commented Jun 20, 2023

@t3chguy I think the spam vector is "pending internal context" and refers to some rooms being configured so that participants can not post comments, but are still allowed to add reactji.

@t3chguy
Copy link
Member

t3chguy commented Jun 20, 2023

Security via obscurity is bogus. Someone will write a basic guide:

  1. Press Ctrl + Shift + I
  2. Find the room ID and event ID you wish to send a reaction to
roomId = "...";
eventId = "...";
reaction = "foobar";
while (true) mxMatrixClientPeg.matrixClient.sendEvent(roomId, null, "m.reaction", { "m.relates_to": {
  "event_id": eventId,
  "key": reaction,
  "rel_type": "m.annotation"
} })

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow free-form reactions