element-hq / element-ios

A glossy Matrix collaboration client for iOS
https://element.io
GNU Affero General Public License v3.0
1.73k stars 498 forks source link

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

Open aaronraimist opened 4 years ago

aaronraimist commented 4 years ago

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"
}
SBiOSoftWhare commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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

ShadowJonathan commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

ShadowJonathan commented 3 years ago

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 commented 2 years ago

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.