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

Replies edited on web are not shown as a reply #3517

Open
aaronraimist opened this issue Aug 7, 2020 · 8 comments
Open

Replies edited on web are not shown as a reply #3517

aaronraimist opened this issue Aug 7, 2020 · 8 comments
Assignees
Labels
T-Defect Something isn't working: bugs, crashes, hangs and other reported problems X-Blocked

Comments

@aaronraimist
Copy link
Contributor

aaronraimist commented Aug 7, 2020

993ED146-D17E-4471-93A0-46B37D57549C

This message from Michael just shows up as a normal message but it is actually a reply to another message. https://matrix.to/#/!mjbDjyNsRXndKLkHIe%3Amatrix.org/%24n2RTKvjGWKL5QPqp8npnmTZaNQ1FZjrWdLuVDHU1PiQ

{
  "sender" : "@delph:matrix.org",
  "content" : {
    "body" : "uh. sorry, that looks like you're mounting .\/etc on top of \/etc\/ in the container, aren't you? So the container won't have all its existing \/etc bits in it",
    "format" : "org.matrix.custom.html",
    "formatted_body" : "uh. sorry, that looks like you're mounting .\/etc on top of \/etc\/ in the container, aren't you? So the container won't have all its existing \/etc bits in it",
    "m.relates_to" : {
      "m.in_reply_to" : {
        "event_id" : "$A3Sk_oyvNNr_Di31gUH2V_MyXLtKM3hTcrgNzxFbJ50"
      }
    },
    "msgtype" : "m.text"
  },
  "origin_server_ts" : 1596806688835,
  "room_id" : "!mjbDjyNsRXndKLkHIe:matrix.org",
  "event_id" : "$n2RTKvjGWKL5QPqp8npnmTZaNQ1FZjrWdLuVDHU1PiQ",
  "unsigned" : {
    "age" : 1031670,
    "m.relations" : {
      "m.replace" : {
        "event_id" : "$EOsp0Xfmd7EAo6ZhR-DqgyWQZrrhSqoGCzc33GI0aAg"
      }
    }
  },
  "type" : "m.room.message"
}
@manuroe manuroe added the T-Defect Something isn't working: bugs, crashes, hangs and other reported problems label Aug 7, 2020
@SBiOSoftWhare SBiOSoftWhare self-assigned this Aug 24, 2020
@SBiOSoftWhare
Copy link
Contributor

The edited reply event has an issue with formatted_body. It does not contain mx-reply tags. It seems that message edition went wrong on the client side. We should determine from which client the edition has been done.

@SBiOSoftWhare
Copy link
Contributor

The behavior for editing a reply message will change. Wait for clarification of section What happens when we edit a reply? of MSC 1849.

@ara4n
Copy link
Member

ara4n commented Jun 20, 2021

Edited replies emitted by EI get this right; EW get it wrong.

@ara4n ara4n changed the title Message not shown as a reply Edited messages are not shown as a reply Jun 20, 2021
@ara4n ara4n changed the title Edited messages are not shown as a reply Replies edited on web are not shown as a reply Jun 20, 2021
@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Jun 20, 2021

Edited replies emitted by EI get this right; EW get it wrong.

EI-edited replies show up as a duplicate of the reply's text on web, i'd argue that EI is the one that gets this wrong, as it mangles the reply fallback;
image

@ara4n
Copy link
Member

ara4n commented Jun 20, 2021

Editing a reply in EI sends <mx-reply><blockquote><a href="permalink">In reply to</a> <a href="mxid">sender</a><br/>Message being replied to</blockquote></mx-reply>Edited reply, for both E2EE and plaintext rooms. Which is a valid fallback. I think the bug there is on EW for not stripping out the fallback on seeing that it's an (edited) reply.

@ara4n
Copy link
Member

ara4n commented Jun 20, 2021

Yeah. comparing the two - EW is sending the edit of the reply without a reply fallback in the new content (hence the original bug here). EI sends including the reply fallback, which then gets double-displayed by EW (which fails to strip the reply). So, there are two things that need to be done:

  • EW needs to strip the fallback when displaying
  • Either EI needs to walk the relations to find the reply, or EW needs to be a better citizen and include a fallback. The latter is easier and will also help out other clients.

@ShadowJonathan
Copy link
Contributor

Interesting, I think I've seen an issue related to the first one on -web, but i'm not sure. (Though maybe it was related to reply fallbacks overall)

@aringenbach
Copy link
Contributor

Yeah. comparing the two - EW is sending the edit of the reply without a reply fallback in the new content (hence the original bug here). EI sends including the reply fallback, which then gets double-displayed by EW (which fails to strip the reply). So, there are two things that need to be done:

  • EW needs to strip the fallback when displaying
  • Either EI needs to walk the relations to find the reply, or EW needs to be a better citizen and include a fallback. The latter is easier and will also help out other clients.

I'm currently looking at a bunch of things regarding replies and rich replies, and from what I see in MSC1849, there is definitely an issue in EI: we are currently providing the fallback formatted_body to both new_content and the "compatibility" body when we should only provide in the latter. EW rightly don't strip the fallback when using new_content as it's not supposed to be there.

For the former issue described here, this is indeed something I can reproduce systematically on EW alone, it happens any time someone edits a reply multiple times. At some point the fallback is simply not provided anymore in the "compatibility" body. While still an issue, this will be less noticeable the further we go on rich replies implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Something isn't working: bugs, crashes, hangs and other reported problems X-Blocked
Projects
None yet
Development

No branches or pull requests

6 participants