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.

Sign up for GitHub

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

Jump to bottom

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"
  },
@t3chguy t3chguy mentioned this issue Apr 14, 2018
Closed
@ara4n ara4n mentioned this issue Apr 14, 2018
Closed
@lampholder lampholder added T-Defect ui/ux P2 S-Major Severely degrades major functionality or product features, with no satisfactory workaround A-Timeline labels Apr 16, 2018
@t3chguy t3chguy mentioned this issue Apr 19, 2018
Closed
@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 •
edited
Loading

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

@t3chguy t3chguy mentioned this issue May 1, 2018
Closed
@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
Fix element-hq/element-web#6523 Emoji rendering destroys paragraphs
14d8c14 
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
Fix element-hq/element-web#6523 Emoji rendering destroys paragraphs
e58853a 
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 <schuermann@creios.net>
MazeChaZer pushed a commit to MazeChaZer/matrix-react-sdk that referenced this issue May 17, 2018
Fix element-hq/element-web#6523 Emoji rendering destroys paragraphs
b55e0f4 
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 <schuermann@creios.net>
MazeChaZer pushed a commit to MazeChaZer/matrix-react-sdk that referenced this issue May 17, 2018
Fix element-hq/element-web#6523 Emoji rendering destroys paragraphs
2e19c7d 
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 <schuermann@creios.net>
@MazeChaZer
Copy link

See matrix-org/matrix-react-sdk#1910

@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
Merge pull request #1910 from MazeChaZer/fix-emoji-line-break-bug
3e79c3e 
Fix element-hq/element-web#6523 Emoji rendering destroys paragraphs
@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
Assignees
No one assigned
Labels
A-Timeline P2 S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@spantaleev @lampholder @MazeChaZer @zeratax @lukebarnard1