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

Fix-up message decoration when mentioned in an edited messages with MSC3952 #24923

Closed
clokep opened this issue Mar 24, 2023 · 8 comments · Fixed by matrix-org/matrix-react-sdk#10502

Comments

@clokep
Copy link

clokep commented Mar 24, 2023

As part of #24377 initial support for MSC3952 was implemented in #9983. That implementation has a couple of bugs/deficiencies to fix-up around editing messages. (The below all assume you have MSC3952 support enabled.)


Currently if you're mentioned in a message it is displayed as red, if that message is then edited (e.g. fix a typo) it is no longer red. The implementation runs push rules over the event and if any event is a highlight than it gets displayed in red. See UnwrappedEventTile.shouldHighlight. This logic needs to be updated somehow for intentional mentions:

  • @daniellekirkwood suggested that if the message or any version of it every notified the user (via a highlight) than the event should be red. (See conversation around here.)
  • Another option (which the MSC suggests) is that the latest edit-only should be checked for mentions. This would be done by checking the mentions property of m.new_content instead of the root-level.
    • Implementation-wise, I'm not really sure how to do this, one option could be to "clone" the event, but replace the content with content["m.new_content"] and then run push rules over that to see if there are any highlights).

While doing this we should be sure to consider both user mentions & room mentions and that the push rules can be disabled by the user.

Similarly to the red text, we sure ensure the state of your own pill (if one exists) is consistent with whether the message itself is treated as a highlight or not.


Expected behaviour:

  1. Someone mentioned me in a message.
  2. They then edit the content (not the mentions)
  3. My mention is still red

This matches the behavior pre-MSC3952, which is broken by MSC3952's implementation.
This should apply to both pre-MSC3952 and MSC3952 mentions

@daniellekirkwood
Copy link
Contributor

daniellekirkwood commented Mar 27, 2023

It's disorienting for users to have a red notification on a room and then to see nothing in the timeline calling their attention... This would happen if the user was mentioned, and then the mention was removed from the timeline message via Edit. In this case, I think the message should stay Red (as it does today)

This is how the message looks when you are mentioned.
Screenshot 2023-03-27 at 14 58 20

This is the suggestion for what the message looks like when the mention is removed but you were originally tagged in it.
Screenshot 2023-03-27 at 14 59 31

We discussed that the all-red messages might be going away soon?
If this is the case, then the "(edit)" should be red, similar to how a keyword would be highlighted in this case.

Created new issue: #24970

@daniellekirkwood
Copy link
Contributor

@t3chguy did you mention that we were starting to incorp different designs for messages with keywords/replies/mentions?

@t3chguy
Copy link
Member

t3chguy commented Mar 27, 2023

@daniellekirkwood element-hq/element-meta#744

@clokep
Copy link
Author

clokep commented Mar 27, 2023

In this case, I think the message should stay Red (as it does today)

It does not stay red today. If you remove a (pre-MSC3952) mention then it reverts back to normal text.


This bug (to be consistent with today's behavior) is that if you edit a message but leave the mention in, then it loses the red text when using MSC3952.

@clokep
Copy link
Author

clokep commented Mar 27, 2023

@daniellekirkwood element-hq/element-meta#744

Although we might want to update how this is displayed, I think this is a separate task from updating the logic to ensure it is displayed at the correct time. If it makes sense to do them at the same time, that's great, but I would hate to see the fixes here get stuck behind a design review when they don't really need it.

@daniellekirkwood
Copy link
Contributor

Ahhh... Ok, then yes, let's move my comment to an Enhancement issue and leave this one as the bug...

So the definition of done here is:

  • Someone mentioned me in a message
  • They then edit the content (not the mentions)
  • My mention is no longer red

@clokep
Copy link
Author

clokep commented Mar 27, 2023

So the definition of done here is:

  • Someone mentioned me in a message
  • They then edit the content (not the mentions)
  • My mention is no longer red

Not quite, it should be:

  • Someone mentioned me in a message.
  • They then edit the content (not the mentions)
  • My mention is still red

This matches the behavior pre-MSC3952, which is broken by MSC3952's implementation.

@kerryarchibald
Copy link
Contributor

Updated the ticket description to include the expected behaviour resolved in Patrick's comment.

su-ex added a commit to SchildiChat/element-desktop that referenced this issue Apr 25, 2023
* Fixes for [CVE-2023-30609](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=CVE-2023-30609) / GHSA-xv83-x443-7rmw
* Pick sensible default option for phone country dropdown ([\#10627](matrix-org/matrix-react-sdk#10627)). Fixes element-hq/element-web#3528.
* Relate field validation tooltip via aria-describedby ([\#10522](matrix-org/matrix-react-sdk#10522)). Fixes element-hq/element-web#24963.
* Handle more completion types in rte autocomplete ([\#10560](matrix-org/matrix-react-sdk#10560)). Contributed by @alunturner.
* Show a tile for an unloaded predecessor room if it has via_servers ([\#10483](matrix-org/matrix-react-sdk#10483)). Contributed by @andybalaam.
* Exclude message timestamps from aria live region ([\#10584](matrix-org/matrix-react-sdk#10584)). Fixes element-hq/element-web#5696.
* Make composer format bar an aria toolbar ([\#10583](matrix-org/matrix-react-sdk#10583)). Fixes element-hq/element-web#11283.
* Improve accessibility of font slider ([\#10473](matrix-org/matrix-react-sdk#10473)). Fixes element-hq/element-web#20168 and element-hq/element-web#24962.
* fix file size display from kB to KB ([\#10561](matrix-org/matrix-react-sdk#10561)). Fixes element-hq/element-web#24866. Contributed by @NSV1991.
* Handle /me in rte ([\#10558](matrix-org/matrix-react-sdk#10558)). Contributed by @alunturner.
* bind html with switch for manage extension setting option ([\#10553](matrix-org/matrix-react-sdk#10553)). Contributed by @NSV1991.
* Handle command completions in RTE ([\#10521](matrix-org/matrix-react-sdk#10521)). Contributed by @alunturner.
* Add room and user avatars to rte ([\#10497](matrix-org/matrix-react-sdk#10497)). Contributed by @alunturner.
* Support for MSC3882 revision 1 ([\#10443](matrix-org/matrix-react-sdk#10443)). Contributed by @hughns.
* Check profiles before starting a DM ([\#10472](matrix-org/matrix-react-sdk#10472)). Fixes element-hq/element-web#24830.
* Quick settings: Change the copy / labels on the options ([\#10427](matrix-org/matrix-react-sdk#10427)). Fixes element-hq/element-web#24522. Contributed by @justjanne.
* Update rte autocomplete styling ([\#10503](matrix-org/matrix-react-sdk#10503)). Contributed by @alunturner.
* Workaround Squirrel.Mac wedging app restart after failed update check ([\element-hq#629](element-hq#629)).
* Fix error about webContents on log out ([\element-hq#627](element-hq#627)).
* Fix error when breadcrumb image fails to load ([\element-hq#609](element-hq#609)).
* Fix create subspace dialog not working ([\#10652](matrix-org/matrix-react-sdk#10652)). Fixes element-hq/element-web#24882
* Fix multiple accessibility defects identified by AXE ([\#10606](matrix-org/matrix-react-sdk#10606)).
* Fix view source from edit history dialog always showing latest event ([\#10626](matrix-org/matrix-react-sdk#10626)). Fixes element-hq/element-web#21859.
* #21451 Fix WebGL disabled error message ([\#10589](matrix-org/matrix-react-sdk#10589)). Contributed by @rashmitpankhania.
* Properly translate errors in `AddThreepid.ts` so they show up translated to the user but not in our logs ([\#10432](matrix-org/matrix-react-sdk#10432)). Contributed by @MadLittleMods.
* Fix overflow on auth pages ([\#10605](matrix-org/matrix-react-sdk#10605)). Fixes element-hq/element-web#19548.
* Fix incorrect avatar background colour when using a custom theme ([\#10598](matrix-org/matrix-react-sdk#10598)). Contributed by @jdauphant.
* Remove dependency on `org.matrix.e2e_cross_signing` unstable feature ([\#10593](matrix-org/matrix-react-sdk#10593)).
* Update setting description to match reality ([\#10600](matrix-org/matrix-react-sdk#10600)). Fixes element-hq/element-web#25106.
* Fix no identity server in help & about settings ([\#10563](matrix-org/matrix-react-sdk#10563)). Fixes element-hq/element-web#25077.
* Fix: Images no longer reserve their space in the timeline correctly ([\#10571](matrix-org/matrix-react-sdk#10571)). Fixes element-hq/element-web#25082. Contributed by @kerryarchibald.
* Fix issues with inhibited accessible focus outlines ([\#10579](matrix-org/matrix-react-sdk#10579)). Fixes element-hq/element-web#19742.
* Fix read receipts falling from sky ([\#10576](matrix-org/matrix-react-sdk#10576)). Fixes element-hq/element-web#25081.
* Fix avatar text issue in rte ([\#10559](matrix-org/matrix-react-sdk#10559)). Contributed by @alunturner.
* fix resizer only work with left mouse click ([\#10546](matrix-org/matrix-react-sdk#10546)). Contributed by @NSV1991.
* Fix send two join requests when joining a room from spotlight search ([\#10534](matrix-org/matrix-react-sdk#10534)). Fixes element-hq/element-web#25054.
* Highlight event when any version triggered a highlight ([\#10502](matrix-org/matrix-react-sdk#10502)). Fixes element-hq/element-web#24923 and element-hq/element-web#24970. Contributed by @kerryarchibald.
* Fix spacing of headings of integration manager on General settings tab ([\#10232](matrix-org/matrix-react-sdk#10232)). Fixes element-hq/element-web#24085. Contributed by @luixxiul.
su-ex added a commit to SchildiChat/element-web that referenced this issue Apr 25, 2023
* Fixes for [CVE-2023-30609](https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=CVE-2023-30609) / GHSA-xv83-x443-7rmw
* Pick sensible default option for phone country dropdown ([\element-hq#10627](matrix-org/matrix-react-sdk#10627)). Fixes element-hq#3528.
* Relate field validation tooltip via aria-describedby ([\element-hq#10522](matrix-org/matrix-react-sdk#10522)). Fixes element-hq#24963.
* Handle more completion types in rte autocomplete ([\element-hq#10560](matrix-org/matrix-react-sdk#10560)). Contributed by @alunturner.
* Show a tile for an unloaded predecessor room if it has via_servers ([\element-hq#10483](matrix-org/matrix-react-sdk#10483)). Contributed by @andybalaam.
* Exclude message timestamps from aria live region ([\element-hq#10584](matrix-org/matrix-react-sdk#10584)). Fixes element-hq#5696.
* Make composer format bar an aria toolbar ([\element-hq#10583](matrix-org/matrix-react-sdk#10583)). Fixes element-hq#11283.
* Improve accessibility of font slider ([\element-hq#10473](matrix-org/matrix-react-sdk#10473)). Fixes element-hq#20168 and element-hq#24962.
* fix file size display from kB to KB ([\element-hq#10561](matrix-org/matrix-react-sdk#10561)). Fixes element-hq#24866. Contributed by @NSV1991.
* Handle /me in rte ([\element-hq#10558](matrix-org/matrix-react-sdk#10558)). Contributed by @alunturner.
* bind html with switch for manage extension setting option ([\element-hq#10553](matrix-org/matrix-react-sdk#10553)). Contributed by @NSV1991.
* Handle command completions in RTE ([\element-hq#10521](matrix-org/matrix-react-sdk#10521)). Contributed by @alunturner.
* Add room and user avatars to rte ([\element-hq#10497](matrix-org/matrix-react-sdk#10497)). Contributed by @alunturner.
* Support for MSC3882 revision 1 ([\element-hq#10443](matrix-org/matrix-react-sdk#10443)). Contributed by @hughns.
* Check profiles before starting a DM ([\element-hq#10472](matrix-org/matrix-react-sdk#10472)). Fixes element-hq#24830.
* Quick settings: Change the copy / labels on the options ([\element-hq#10427](matrix-org/matrix-react-sdk#10427)). Fixes element-hq#24522. Contributed by @justjanne.
* Update rte autocomplete styling ([\element-hq#10503](matrix-org/matrix-react-sdk#10503)). Contributed by @alunturner.
* Fix create subspace dialog not working ([\element-hq#10652](matrix-org/matrix-react-sdk#10652)). Fixes element-hq#24882
* Fix multiple accessibility defects identified by AXE ([\element-hq#10606](matrix-org/matrix-react-sdk#10606)).
* Fix view source from edit history dialog always showing latest event ([\element-hq#10626](matrix-org/matrix-react-sdk#10626)). Fixes element-hq#21859.
* element-hq#21451 Fix WebGL disabled error message ([\element-hq#10589](matrix-org/matrix-react-sdk#10589)). Contributed by @rashmitpankhania.
* Properly translate errors in `AddThreepid.ts` so they show up translated to the user but not in our logs ([\element-hq#10432](matrix-org/matrix-react-sdk#10432)). Contributed by @MadLittleMods.
* Fix overflow on auth pages ([\element-hq#10605](matrix-org/matrix-react-sdk#10605)). Fixes element-hq#19548.
* Fix incorrect avatar background colour when using a custom theme ([\#10598](matrix-org/matrix-react-sdk#10598)). Contributed by @jdauphant.
* Remove dependency on `org.matrix.e2e_cross_signing` unstable feature ([\element-hq#10593](matrix-org/matrix-react-sdk#10593)).
* Update setting description to match reality ([\element-hq#10600](matrix-org/matrix-react-sdk#10600)). Fixes element-hq#25106.
* Fix no identity server in help & about settings ([\element-hq#10563](matrix-org/matrix-react-sdk#10563)). Fixes element-hq#25077.
* Fix: Images no longer reserve their space in the timeline correctly ([\element-hq#10571](matrix-org/matrix-react-sdk#10571)). Fixes element-hq#25082. Contributed by @kerryarchibald.
* Fix issues with inhibited accessible focus outlines ([\element-hq#10579](matrix-org/matrix-react-sdk#10579)). Fixes element-hq#19742.
* Fix read receipts falling from sky ([\element-hq#10576](matrix-org/matrix-react-sdk#10576)). Fixes element-hq#25081.
* Fix avatar text issue in rte ([\element-hq#10559](matrix-org/matrix-react-sdk#10559)). Contributed by @alunturner.
* fix resizer only work with left mouse click ([\element-hq#10546](matrix-org/matrix-react-sdk#10546)). Contributed by @NSV1991.
* Fix send two join requests when joining a room from spotlight search ([\#10534](matrix-org/matrix-react-sdk#10534)). Fixes element-hq#25054.
* Highlight event when any version triggered a highlight ([\element-hq#10502](matrix-org/matrix-react-sdk#10502)). Fixes element-hq#24923 and element-hq#24970. Contributed by @kerryarchibald.
* Fix spacing of headings of integration manager on General settings tab ([\element-hq#10232](matrix-org/matrix-react-sdk#10232)). Fixes element-hq#24085. Contributed by @luixxiul.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants