Fix new lines when replacing <blockquote> and by aldo-expensify · Pull Request #474 · Expensify/expensify-common

aldo-expensify · 2022-11-22T03:05:53Z

This is part of a group of changes to preserving new lines when converting from message[0].html using ExpensiMark.htmlToText. This should improve:

Copy message content to clipboard (Copy text using the clipboard button, and past in text editor: the line breaks are missing. Related issue: [HOLD for payment 2022-12-20] [$2000] mweb - Copy & Paste - Copy & Pasting text form code editor is not consistent App#10262)
It is going to be useful for showing only the first line of a message as in the LHN (currently <blockquote> cause buggy spacing)

Fixed Issues

$ Expensify/App#12894 (partially)
Expensify/App#11494 (partially)

Tests

Make sure unit test run (npm run test)

Go to a chat
Go offline
Compose a message by:
1. Add an emoji
2. Add two or more empty lines
3. Add an emoji
Send the message
Find report action in Onyx and make sure that the line breaks appear as \n in message[0].text
Compose another message with the content:

before quotes
> quoted text
after quotes

Find report action in Onyx and make sure that the message[0].text contains before quotes\nquoted text\nafter quotes. This is because <blockquote> are block elements and they should cause line breaks in the content.

QA

Go to a chat
Go offline
Compose a message by:
1. Add an emoji
2. Add two or more empty lines
3. Add an emoji
Send the message
Find report action in Onyx and make sure that the line breaks appear as \n in message[0].text
Compose another message with the content:

before quotes
> quoted text
after quotes

Find report action in Onyx and make sure that the message[0].text contains before quotes\nquoted text\nafter quotes. This is because <blockquote> are block elements and they should cause line breaks in the content.

aldo-expensify · 2022-11-22T03:08:33Z

cc @mollfpr since you added this code that transform html into text using regexes. Just tagging you in case you notice that what I'm doing here will cause a bug.

mollfpr · 2022-11-22T04:58:14Z

lib/ExpensiMark.js

            {
                name: 'breakline',
-                regex: /((<br[^>]*>)+)/gi,
+                regex: /<br[^>]*>/gi,


I just roll back to the issue and the PR. Back then the issue is about the last message text in the LHN when sending a blockquote. So the solution I propose is to replace the blockquote closing tag with a space. Other regex rules in the htmlToText rule from the E/App that I move to expensi-mark.

I believe this will affect the text in LHN where it should be replacing adjoin break lines with a space.

@mollfpr I tested blockquotes but couldn't spot any new bugs. Did you find any issues with these changes?

I did notice that entering two block quotes with empty new lines seems to remove the space in the LHN message, but that's also happening on staging so it's not introduced here.

web.mov

I believe this will affect the text in LHN where it should be replacing adjoin break lines with a space.

I had this thought after I stopped working hehe, but I just checked and it doesn't affect in web:

I'm going to check Android/IOS

Also, I noticed that the server:

for the report action, it replaces each   for a ' ' (whitespace), but

for the report, the lastMessageText has collapsed consecutive   into a single ' '

I guess we should separate the function to convert the message HTML to text between LHN and chat.

Yeah, something like that... I was looking at the backend, and convert the html to lastMessageText using a function that applies the same regexes that htmlToText apply, including /((<br[^>]*>)+)/.

I was thinking that it doesn't make much sense that 🥶. 🥶 gets converted to 🥶..🥶 (two spaces) just collapsing the   into a single space... why the result is not 🥶.🥶 (single space) if our intention is really to collapse multiple white spaces. I'm thinking that htmlToText should maybe be changed to:

Step 1: replace each   by single spaces

Step 2: collapse all consecutive spaces into a single space

I still have to look at what the back end is doing to produce the text of the message in the report action from the html. Is there a reason why we don't collapse the spaces when deriving text from html?

btw, I replaced the whitespaces in 🥶 🥶 by . because they are getting collapsed here.

The html of a report action's message gets converted to text here, and it replaces each   with a single whitespace (no collapsing).

The lastMessageText of the report gets derived from html here, collapsing multiple   into a single whitespace.

I guess we should separate the function to convert the message HTML to text between LHN and chat.

@mollfpr so.. I think the best way to go here is:

htmlToText won't collapse multiple   into a single whitespace because that is only needed for the lastMessageText which is used in the LHN (what this PR is doing)

The function ReportUtils.formatReportLastMessageText will have the responsibility of collapsing consecutive whitespaces into a single one. We already use this function to process what we put in lastMessageText

@aldo-expensify sounds good to me!

I found an issue when testing this PR, but it's not related to the PR changes. The last message on LHN flinched without whitespace when sending a text with a blockquote.

Screen.Recording.2022-11-27.at.22.16.53.mov

Logged issue here https://expensify.slack.com/archives/C049HHMV9SM/p1669564594408699

luacmartins

LGTM just waiting on @mollfpr confirmation on any new issues with blockquotes

mollfpr · 2022-11-22T18:28:48Z

Okay, I couldn't find any issue in the current LHN with this change but some things broke the current LHN.

As you can see the alternateText creates spaces between the text because we replaced every breakline with a space. The alternateText value itself is from the lastMessageText key from the report object where the text is using htmlToText. But apparently, the current LHN doesn't show that lastMessageText is in LHN.

I found another issue in LHN, but it's not related to these changes but related to the htmlToText function. The optimistic last message in LHN should be Confusion This.

Screen.Recording.2022-11-23.at.01.14.19.mov

With the current LHN behavior to show the last message, this PR will not affect the LHN but I think we should fix the above issue to show the last message correctly.

…o_replace-each-br-for-single-whitespace

aldo-expensify · 2022-11-29T20:27:12Z

@mollfpr @luacmartins can you review from 0 again 🙏 ? I changed this PR to:

<blockquote> are replaced by \n if there is content before
</blockquote> are replaced by \n if there is content after
  are replaced by \n

mollfpr · 2022-11-30T07:34:14Z

Will test in a couple of hours.

mollfpr · 2022-11-30T18:51:30Z

Sorry, I have a question I'm a little bit confused. Is this PR will fix Expensify/App#12894 and Expensify/App#11494?

Anyway, the changes look much better now 👍

aldo-expensify · 2022-11-30T18:53:05Z

Sorry, I have a question I'm a little bit confused. Is this PR will fix Expensify/App#12894 and Expensify/App#11494?

Partially, there is also a PR for the API and also for the App.

This PR:

Updates ExpensiMark.htmlToText to:
- replace each   by \n
- replace <blockquote> by \n when there is content before it
- replace </blockquote> by \n when there is content after it
Updated tests.

Other PRs:

https://github.com/Expensify/Web-Expensify/pull/35658
- Updates how we convert from message[0].html to message[0].text using a centralized function
- ReportUtils::convertCommentsHtmlToText, and makes it consistent with ExpensiMark.htmlToText
- Added tests
- Updated code that produces lastMessageText to be consistent with the App code. Now we only keep the first line (slack convo: https://expensify.slack.com/archives/C01GTK53T8Q/p1669671957761929).
Message html to text and last message (LHN) fixes App#12968:
- ReportUtils.formatReportLastMessageText truncates to the first line.
- Simplified the code deciding between rendering as html or text
- Update expensify-common version (HOLD)

luacmartins

LGTM!

aldo-expensify added 2 commits November 21, 2022 18:58

Replace each BR for a single whitespace

c5b0ff5

Add test case / fix borked regex

2279133

aldo-expensify self-assigned this Nov 22, 2022

aldo-expensify requested a review from a team as a code owner November 22, 2022 03:05

melvin-bot bot requested review from luacmartins and removed request for a team November 22, 2022 03:06

mollfpr reviewed Nov 22, 2022

View reviewed changes

luacmartins previously approved these changes Nov 22, 2022

View reviewed changes

aldo-expensify changed the title ~~Replace each for single whitespace~~ [HOLD - this may not be ready] Replace each for single whitespace Nov 22, 2022

This was referenced Nov 23, 2022

Message html to text and last message (LHN) fixes Expensify/App#12968

Merged

[HOLD for payment 2022-12-20] [$2000] mweb - Copy & Paste - Copy & Pasting text form code editor is not consistent Expensify/App#10262

Closed

aldo-expensify added 2 commits November 24, 2022 14:32

Merge branch 'main' of github.com:Expensify/expensify-common into ald…

58891a2

…o_replace-each-br-for-single-whitespace

Use newline instead of whitespace

4dde1fa

aldo-expensify dismissed luacmartins’s stale review via 4dde1fa November 26, 2022 03:25

aldo-expensify added 3 commits November 28, 2022 16:01

Update tests

1a7223f

Replace end quote by new line

c89e6a1

Add newlines around blockquotes

f7466fe

aldo-expensify changed the title ~~[HOLD - this may not be ready] Replace each for single whitespace~~ Fix new lines when replacing <blockquote> and Nov 29, 2022

aldo-expensify requested review from luacmartins and mollfpr and removed request for mollfpr November 29, 2022 20:25

Add test case

2ea21fa

aldo-expensify requested a review from mollfpr November 29, 2022 20:31

aldo-expensify added 2 commits November 29, 2022 13:20

Add strip tags test

ff84c71

Fix comment

acdc2f6

Update tests

5aa660e

mollfpr approved these changes Nov 30, 2022

View reviewed changes

luacmartins approved these changes Nov 30, 2022

View reviewed changes

luacmartins merged commit 6bd112f into main Nov 30, 2022

luacmartins deleted the aldo_replace-each-br-for-single-whitespace branch November 30, 2022 19:45

luacmartins mentioned this pull request Dec 5, 2022

[$250] Chat - In LHN preview there is no space between quoted text and normal text @thesahindia Expensify/App#11494

Closed

aldo-expensify mentioned this pull request Dec 5, 2022

Chat - Emojis are switching from big size to small Expensify/App#13292

Closed

b1tjoy mentioned this pull request Dec 7, 2022

Fix copy and paste text inconsistent on mWeb Expensify/App#13396

Merged

49 tasks

Conversation

aldo-expensify commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues

Tests

QA

Uh oh!

aldo-expensify commented Nov 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

mollfpr commented Nov 22, 2022

Uh oh!

aldo-expensify commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mollfpr commented Nov 30, 2022

Uh oh!

mollfpr commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aldo-expensify commented Nov 22, 2022 •

edited

Loading

aldo-expensify Nov 22, 2022 •

edited

Loading

aldo-expensify Nov 22, 2022 •

edited

Loading

aldo-expensify commented Nov 29, 2022 •

edited

Loading

mollfpr commented Nov 30, 2022 •

edited

Loading

aldo-expensify commented Nov 30, 2022 •

edited

Loading