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

Emoji rendering destroys paragraphs #6523

Closed
spantaleev opened this issue Apr 14, 2018 · 8 comments
Closed

Emoji rendering destroys paragraphs #6523

spantaleev opened this issue Apr 14, 2018 · 8 comments
Labels
A-Timeline P2 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect

Comments

@spantaleev
Copy link
Contributor

This seems to be a regression since 0.14.0.

The paragraphs in the following message get destroyed while rendering.

Original (as it appears in the message input box):

test 🙂

test

Resulting event:

  "content": {
    "body": "test 🙂\n\ntest",
    "msgtype": "m.text"
  },

Incorrectly renders as:

riot-web-emoji-plain


Workaround 1

Forcing Markdown-processing to trigger works around this problem.

The following message:

**test** 🙂

test

produces the following event, which renders correctly (paragraphs are preserved):

  "content": {
    "body": "**test** 🙂\n\ntest",
    "msgtype": "m.text",
    "formatted_body": "<p><strong>test</strong> 🙂</p>\n<p>test</p>\n",
    "format": "org.matrix.custom.html"
  },

Workaround 2

If there's no emoji in the text, everything works well.

The following message:

test

test

produces the following event, which renders correctly (paragraphs are preserved):

  "content": {
    "body": "test\n\ntest",
    "msgtype": "m.text"
  },
@lukebarnard1
Copy link
Contributor

We apply the CSS class markdown-body in messages with emoji (because we insert emoji with HTML). This class includes the rule white-space: normal;.

With white-space: normal, "Sequences of whitespace are collapsed..." including newline characters.

Using white-space: pre-wrap (preserves \n as <br>) might be a solution (which is what .mx_MTextBody defines) , but it would be worth:

  • testing this in a whole world of different cases
  • finding out why we're using white-space: normal (perhaps something to do with wrapping? or overflowing?)
  • writing a bunch of unit tests for EventTile ♥️

@zeratax
Copy link

zeratax commented Apr 30, 2018

Same seems to happen for Braille Patterns (U+2800-U+28FF) possibly more.

@spantaleev
Copy link
Contributor Author

Will this see some fix/workaround in 0.14.x?

Seems like a pretty annoying regression to drag along.

@MazeChaZer
Copy link

I found the problem that caused this bug, will submit a pull request this evening.

MazeChaZer pushed a commit to MazeChaZer/matrix-react-sdk that referenced this issue May 17, 2018
This regression was probably introduced in
4f4441f and is caused by the fact that
the variable `isHtml` conflates two different meanings:

- The event contains an HTML message
- The event message is displayed using HTML

This is an important difference. Plain text messages that contain
emojies are rendered with an HTML string and thus have to be sanitized
etc. But they must not use the MarkDown CSS styles for HTML messages.

The MarkDown CSS styles include `whitespace: normal` because HTML events
use `<br/>`-tags for line breaks. Plain text messages with emojies
obviously don't use `<br/>`-tags, so these styles must not be applied.
MazeChaZer pushed a commit to MazeChaZer/matrix-react-sdk that referenced this issue May 17, 2018
This regression was probably introduced in
4f4441f and is caused by the fact that
the variable `isHtml` conflates two different meanings:

- The event contains an HTML message
- The event message is displayed using HTML

This is an important difference. Plain text messages that contain
emojies are rendered with an HTML string and thus have to be sanitized
etc. But they must not use the MarkDown CSS styles for HTML messages.

The MarkDown CSS styles include `whitespace: normal` because HTML events
use `<br/>`-tags for line breaks. Plain text messages with emojies
obviously don't use `<br/>`-tags, so these styles must not be applied.

Signed-off-by: Jonas Schürmann <[email protected]>
MazeChaZer pushed a commit to MazeChaZer/matrix-react-sdk that referenced this issue May 17, 2018
This regression was probably introduced in
4f4441f and is caused by the fact that
the variable `isHtml` conflates two different meanings:

- The event contains an HTML message
- The event message is displayed using HTML

This is an important difference. Plain text messages that contain
emojies are rendered with an HTML string and thus have to be sanitized
etc. But they must not use the MarkDown CSS styles for HTML messages.

The MarkDown CSS styles include `whitespace: normal` because HTML events
use `<br/>`-tags for line breaks. Plain text messages with emojies
obviously don't use `<br/>`-tags, so these styles must not be applied.

Signed-off-by: Jonas Schürmann <[email protected]>
MazeChaZer pushed a commit to MazeChaZer/matrix-react-sdk that referenced this issue May 17, 2018
This regression was probably introduced in
4f4441f and is caused by the fact that
the variable `isHtml` conflates two different meanings:

- The event contains an HTML message
- The event message is displayed using HTML

This is an important difference. Plain text messages that contain
emojies are rendered with an HTML string and thus have to be sanitized
etc. But they must not use the MarkDown CSS styles for HTML messages.

The MarkDown CSS styles include `whitespace: normal` because HTML events
use `<br/>`-tags for line breaks. Plain text messages with emojies
obviously don't use `<br/>`-tags, so these styles must not be applied.

Signed-off-by: Jonas Schürmann <[email protected]>
@MazeChaZer
Copy link

@MazeChaZer
Copy link

@zeratax The pull request also fixes the problem for braille characters

@MazeChaZer
Copy link

@lukebarnard1 I removed the class markdown-body from emoji messages because it looked like it was accidentally added in matrix-org/matrix-react-sdk@4f4441f, so it shouldn't be a problem. Does that look good to you?

lukebarnard1 added a commit to matrix-org/matrix-react-sdk that referenced this issue May 18, 2018
@lukebarnard1
Copy link
Contributor

It does indeed, merged 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Timeline P2 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Development

No branches or pull requests

5 participants