element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
GNU Affero General Public License v3.0
11.04k stars 1.96k forks source link

Edit message composer has superfluous escaping #22456

Open germain-gg opened 2 years ago

germain-gg commented 2 years ago

Steps to reproduce

  1. Send matrix-react-sdk/res/themes/light/css/_light.scss as message in Element
  2. Hit the up arrow to edit the message

Outcome

What did you expect?

To see the edit message composer appear and prefilled with matrix-react-sdk/res/themes/light/css/_light.scss

What happened instead?

Screen Shot 2022-06-06 at 09 05 50 Screen Shot 2022-06-06 at 13 56 28

Notice the incorrect escaping before the underscore

Operating system

No response

Browser information

No response

URL for webapp

No response

Application version

No response

Homeserver

No response

Will you send logs?

No

robintown commented 2 years ago

It's not incorrect, just unnecessary, so will mark this as tolerable

germain-gg commented 2 years ago

I disagree with the triaging here. The escaping is incorrect. My message does not contain those backslash and to a non technical user this will look out of place

It could even make them move the cursor to delete those characters which is a big ask

t3chguy commented 2 years ago

@gsouquet that's a whole class of bugs

https://github.com/vector-im/element-web/issues/22342 https://github.com/vector-im/element-meta/issues/1493 https://github.com/vector-im/element-web/issues/10808

anoadragon453 commented 2 years ago

Also chiming in with my own instance of this while editing a message. The editing composer showed an extra backslash, whereas the original message only contained one:

image

Event source ``` { "type": "m.room.message", "content": { "org.matrix.msc1767.message": [ { "body": "\\", "mimetype": "text/plain" }, { "body": "\\", "mimetype": "text/html" } ], "body": "\\", "msgtype": "m.text", "format": "org.matrix.custom.html", "formatted_body": "\\" } } ```

Looking at the event source, I'm confused why body has two backslashes...

t3chguy commented 2 years ago

Looking at the event source, I'm confused why body has two backslashes...

Otherwise it'd be escaping the " and breaking JSON

https://www.freeformatter.com/json-escape.html

Backslash is replaced with \

anoadragon453 commented 2 years ago

Otherwise it'd be escaping the " and breaking JSON

Ahhh, duh, right.

I suppose the blame does still lie with the edit composer field displaying \\ to the user then.

t3chguy commented 2 years ago

Yeah so that is just the re-escaping code being naive and not checking if the escaping is necessary

babolivier commented 2 years ago

Worth noting it affects more than just what the sender sees in the composer, as it can also affect code blocks, as seen in one of the supporter rooms earlier this week:

image

(since escaping backlashes aren't automagically hidden in code blocks)

robintown commented 2 years ago

@babolivier Please open a separate issue for that, so we can keep this one focused on the benign cases of unnecessary escaping. Incidentally, I can't reproduce that issue, so please provide repro steps if you can.

StyXman commented 2 years ago

So this was closed but I still see it in https://app.element.io/ is it just a matter of the code not being deployed yet?

t3chguy commented 2 years ago

@StyXman this issue hasn't been closed though?

image

babolivier commented 2 years ago

Please open a separate issue for that, so we can keep this one focused on the benign cases of unnecessary escaping. Incidentally, I can't reproduce that issue, so please provide repro steps if you can.

Sorry I completely missed this comment. iirc I was reporting an issue with a message that @anoadragon453 sent. I cannot seem to be able to repro it either. So I'll let Andrew open a new issue if it happens again.

anoadragon453 commented 2 years ago

I'm not able to reproduce the issue in your comment at this point @babolivier.

I can still reproduce the one in the message editing composer as shown here however.

Element Desktop Nightly 2022090801, Arch Linux.

StyXman commented 1 year ago

@StyXman this issue hasn't been closed though?

yeah, sorry, I got confused by this:

image