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

Edit message composer has superfluous escaping #22456

Open
germain-gg opened this issue Jun 6, 2022 · 14 comments
Open

Edit message composer has superfluous escaping #22456

germain-gg opened this issue Jun 6, 2022 · 14 comments
Labels
A-Composer A-Message-Editing O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression

Comments

@germain-gg
Copy link
Contributor

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

@germain-gg germain-gg added T-Defect X-Regression S-Minor Impairs non-critical functionality or suitable workarounds exist A-Composer O-Occasional Affects or can be seen by some users regularly or most users rarely labels Jun 6, 2022
@robintown
Copy link
Member

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

@robintown robintown added S-Tolerable Low/no impact on users A-Message-Editing and removed S-Minor Impairs non-critical functionality or suitable workarounds exist labels Jun 7, 2022
@t3chguy t3chguy changed the title Edit message composer incorrect escaping Edit message composer has superfluous escaping Jun 7, 2022
@germain-gg
Copy link
Contributor Author

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

@robintown robintown added S-Minor Impairs non-critical functionality or suitable workarounds exist and removed S-Tolerable Low/no impact on users labels Jun 7, 2022
@t3chguy
Copy link
Member

t3chguy commented Jun 7, 2022

@gsouquet that's a whole class of bugs

#22342
element-hq/element-meta#1493
#10808

@anoadragon453
Copy link
Member

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
Copy link
Member

t3chguy commented Aug 10, 2022

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
Copy link
Member

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
Copy link
Member

t3chguy commented Aug 10, 2022

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

@babolivier
Copy link
Contributor

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
Copy link
Member

@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.

@t3chguy t3chguy self-assigned this Sep 16, 2022
@RiotRobot RiotRobot added this to In Progress in Web App Team Sep 16, 2022
@t3chguy t3chguy moved this from In Progress to In Review in Web App Team Sep 16, 2022
@t3chguy t3chguy removed their assignment Sep 16, 2022
@t3chguy t3chguy removed this from In Review in Web App Team Sep 16, 2022
@StyXman

This comment was marked as off-topic.

@t3chguy

This comment was marked as resolved.

@babolivier
Copy link
Contributor

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
Copy link
Member

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

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Composer A-Message-Editing O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect X-Regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants