fix: email new line by skipping extra div #13503

ahmdshrif · 2022-12-10T11:28:00Z

Details

Fixed Issues

$ #9250
PROPOSAL: #9250 (comment)

Tests

Login with any account and go to any chat room
Typed a message a@mail.com is my email. b@mail.com is someone else's email. in composer and send it.
Select that message, copy and paste it into composer.
Verify there is no extra line on pasted text.

Verify that no errors appear in the JS console

Offline tests

Login with any account and go to any chat room
set your device to Airplane mode.
Typed a message a@mail.com is my email. b@mail.com is someone else's email. in composer and send it.
Select that message, copy and paste it into composer.
Verify there is no extra line on pasted text.

QA Steps

steps :

Login with any account and go to any chat room

Typed the below message in composer and send it.

a@mail.com is my email. 
https://google.com  is my website.
this is **bold** and this is not bold 
this is *italic*  and this is not italic 
this is ~~Strikethrough~~  an this not  Strikethrough
> this is a line quote

this is an inline `quote` and this is not quote 

this is emoji 😃  and this another one 😄

Select that message, copy and paste it into composer.
Verify there is no extra line on pasted text.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2022-12-10.at.1.52.22.PM.mov

Mobile Web - Chrome

WhatsApp.Video.2022-12-14.at.11.59.39.PM.mp4

Mobile Web - Safari

Simulator.Screen.Recording.-.iPhone.14.-.2022-12-14.at.23.23.19.mp4

Desktop

Screen.Recording.2022-12-10.at.7.33.18.PM.mov

iOS

Simulator.Screen.Recording.-.iPhone.13.-.2022-12-15.at.17.11.22.mp4

Android

WhatsApp.Video.2022-12-15.at.12.05.47.AM.mp4

ahmdshrif · 2022-12-11T00:01:33Z

there is an issue with login in development env as mentioned here so I can't provide all video in MWeb since I am not login there. @aimane-chnaif if you want you can do an initial code review until this issue is fixed .

aimane-chnaif · 2022-12-15T10:12:06Z

@ahmdshrif PR is still in draft. when is it marked as review? I see you already added web/mWeb screenshots but iOS screenshot is missing

ahmdshrif · 2022-12-15T13:46:47Z

@aimane-chnaif I have an issue with running on ios. looks like it's a cache dependency issue with.
once I solve it I will update ios.

but I think we do not need ios or android since this issue does not affect native and the code we change is only working on the web.

![Screenshot 2022-12-15 at 3 44 21 PM](https://user-images.githubusercontent.com/21364901/207874415-1e727a8f-f910-4acd-b465-7710898eb6fb.png)

aimane-chnaif · 2022-12-15T14:15:55Z

@aimane-chnaif I have an issue with running on ios. looks like it's a cache dependency issue with.
once I solve it I will update ios.

ok, please fix soon and complete PR so I can review immediately.
Let's target to get PR merged asap to be eligible for timeline bonus.

melvin-bot · 2022-12-15T15:13:35Z

@Julesssss @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

ahmdshrif · 2022-12-15T15:13:48Z

@aimane-chnaif it's ready now

aimane-chnaif · 2022-12-15T17:54:18Z

@ahmdshrif please update testing step like this: (Emails are hyperlinked automatically, no need markdown)
Typed a message [a@mail.com](mailto:a@mail.com) is my email. [b@mail.com](mailto:b@mail.com) is someone else's email. ->
Type a message a@mail.com is my email. b@mail.com is someone else's email. in composer and send it.

Also Copied and pasted in the text box below -> Select that message, copy and paste it into composer.

And Login with any account and go to any chat room step is missing at the beginning.

Please thoroughly check and fix any spelling or space mistakes in PR description.
i.e.
1-xxx, 2- xxx should be consistent. Add space between number and -
3- verify there is no extra line on pasted text . Here, v should be capital letter.

ahmdshrif · 2022-12-15T18:13:35Z

@aimane-chnaif update it ... I copied the steps from the issue. but now correct it.

aimane-chnaif · 2022-12-15T18:15:57Z

src/libs/SelectionScraper/index.js

+        // skip useless div because it's doing some issues on reconverting the HTML to markdown
+        // and we skip it only if:-
+        // 1- not contain specific attributes
+        // 2- have only one child and not a text


also fix here including any spelling or grammatical mistakes in comment

I was thinking about updating comment to this comment:-

// We are excluding divs that have only one child and no text nodes and don't have a tagAttribute. // This is due to the fact that these divs are meaningless to us and are interfering with the HTML to Markdown conversion process. // For example, the following HTML : // <div><div> hi </div></div> // we will return as : // <div> hi </div>

@aimane-chnaif I update the comment but delete the example above I think we not need it.

aimane-chnaif · 2022-12-16T10:57:43Z

@ahmdshrif please add step in Offline tests. Even though offline is not related to this GH, you should follow.

ahmdshrif · 2022-12-16T11:18:47Z

please add step in Offline tests.

done . @aimane-chnaif

aimane-chnaif · 2022-12-16T11:35:17Z

~~@ahmdshrif not done yet. can you please be careful whenever you comment?~~
~~There should be step to disable network.~~
~~Please add Disable network in 2nd step~~

Edit: ah sorry, I see you added
set your device to Airplane mode.
But it would be better to add it in Step 2. so send message when offline

ahmdshrif · 2022-12-16T11:44:09Z

@aimane-chnaif np, and I agree it's better to be Step 2 I update it.

aimane-chnaif · 2022-12-16T11:51:59Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

Mobile Web - Safari

msafari.mp4

Desktop

desktop.mov

iOS

ios.mp4

Android

aimane-chnaif

Tests well for me

All yours @Julesssss

Julesssss · 2022-12-16T12:31:18Z

src/libs/SelectionScraper/index.js

+        // We are excluding divs that have only one child and no text nodes and don't have a tagAttribute.
+        // This is due to the fact that these divs are meaningless to us and are interfering with the HTML to Markdown conversion process.


and don't have a tagAttribute

Is that true? I can't see which condition this is referring to

else condition

Slight re-wording:

We are excluding divs that have only one child and no text nodes and don't have a tagAttribute to prevent additional newlines from being added in the HTML to Markdown conversion process.

I just think this is the case we catch but can be another case also not only a new line.

@Julesssss but yes we just explain what we have now so i update the comment

Julesssss · 2022-12-16T12:49:54Z

Tests well, I used a bunch of random Markdown text in addition to the tests. Can't think of any additional things to regression test.

github-actions · 2022-12-16T13:12:20Z

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries

Name	Duration
TTI	787.127 ms → 809.221 ms (+22.094 ms, +2.8%)
runJsBundle	184.839 ms → 195.219 ms (+10.380 ms, +5.6%)
nativeLaunch	10.000 ms → 10.125 ms (+0.125 ms, +1.3%)
regularAppStart	0.016 ms → 0.014 ms (-0.003 ms, -15.5%) 🟢

Show details

Name	Duration
TTI	Baseline Mean: 787.127 ms Stdev: 46.798 ms (5.9%) Runs: 725.7061200002208 732.0400830004364 732.7325419997796 743.2205969998613 744.9422690002248 748.1352679999545 749.1205740002915 752.6143450001255 755.5639140000567 755.6689160000533 757.1970340004191 760.3870400004089 762.7891130000353 767.4618790000677 769.3897050004452 776.5059690000489 780.4777440000325 781.8002049997449 785.3940059999004 789.9809299996123 791.3861670000479 798.4778490001336 800.9541239999235 801.563060999848 816.7500480003655 837.1585280001163 851.1253920001909 869.2828599996865 883.2368769999593 886.3121539996937 893.5636980002746 Current Mean: 809.221 ms Stdev: 38.530 ms (4.8%) Runs: 744.8740189997479 753.8735910002142 756.4746289998293 758.7352449996397 770.5933900000528 777.9357500001788 778.785713000223 784.3080070000142 786.3315129997209 787.009058999829 788.3345109997317 791.0786969996989 792.8301780000329 795.6480799997225 797.2363170003518 798.5937989996746 801.6138679999858 801.8200209997594 812.1786059997976 818.7531369999051 829.5469610001892 832.4172489997 834.9183050002903 836.0856590000913 837.8569470001385 852.4311840003356 853.5683549996465 860.1283390000463 867.1283170003444 892.2555339997634 892.5180240003392
runJsBundle	Baseline Mean: 184.839 ms Stdev: 19.552 ms (10.6%) Runs: 151 154 161 161 166 167 167 168 172 174 174 175 176 177 181 183 186 188 188 190 194 195 198 199 203 203 206 207 209 222 235 Current Mean: 195.219 ms Stdev: 24.750 ms (12.7%) Runs: 156 161 164 166 167 168 170 171 174 180 185 186 187 187 188 190 193 194 196 199 199 208 213 215 216 216 220 221 222 242 245 248
nativeLaunch	Baseline Mean: 10.000 ms Stdev: 1.483 ms (14.8%) Runs: 8 8 8 8 9 9 9 9 9 9 9 9 9 9 10 10 10 10 10 11 11 11 11 11 11 11 12 12 13 14 Current Mean: 10.125 ms Stdev: 2.484 ms (24.5%) Runs: 7 8 8 8 8 8 8 8 8 8 9 9 9 9 9 9 9 9 10 10 10 11 11 11 12 12 12 12 14 15 16 17
regularAppStart	Baseline Mean: 0.016 ms Stdev: 0.002 ms (10.5%) Runs: 0.014078999869525433 0.014159999787807465 0.014485999941825867 0.01501499954611063 0.015176999382674694 0.015178000554442406 0.015217999927699566 0.01521800085902214 0.015259000472724438 0.01537999976426363 0.015461999922990799 0.015665999613702297 0.015705999918282032 0.015705999918282032 0.015705999918282032 0.015786999836564064 0.01582799945026636 0.015868999995291233 0.015868999995291233 0.015951000154018402 0.016479999758303165 0.0170889999717474 0.017292999662458897 0.017414999194443226 0.017862999811768532 0.017862999811768532 0.018920999951660633 0.019084000028669834 0.019123999401926994 0.01932699978351593 0.02164699975401163 Current Mean: 0.014 ms Stdev: 0.001 ms (4.2%) Runs: 0.012492000125348568 0.012939000502228737 0.013143000192940235 0.013183999806642532 0.013387000188231468 0.01342800073325634 0.0134680001065135 0.013469000346958637 0.013712000101804733 0.01375299971550703 0.01375299971550703 0.013753999955952168 0.013753999955952168 0.013875000178813934 0.013957000337541103 0.013957000337541103 0.01403799932450056 0.014038000255823135 0.014038000255823135 0.014119000174105167 0.014322999864816666 0.014403999783098698 0.014404000714421272 0.014566999860107899 0.01460800040513277 0.014770000241696835 0.015178000554442406

OSBotify · 2022-12-16T14:00:40Z

🚀 Deployed to staging by @Julesssss in version: 1.2.41-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-12-20T06:05:20Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.41-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

tienifr · 2023-04-24T07:50:06Z

Hello @ahmdshrif. Coming from #16921 (comment). I have a question regarding this PR: Why do we skip text node here? What if I remove that condition (i.e. only skip div which has only one child)? Can you elaborate some cases? Feel so grateful if you take a look at my question.

Cc @aimane-chnaif

ahmdshrif · 2023-04-24T18:54:11Z

Hi @tienifr, sorry for the late response. I was sick leave.
Anyway, we do not skip the text node. we skip the extra div around the text node and map the text node here because the extra div between text means a new line .

If you delete the text condition, some div response to new line will be skipped

I write this comment from mobile, and i will comment on the issue soon with more examples and details.

tienifr · 2023-04-25T16:51:21Z

@ahmdshrif Take a look at this when you have some time.

fix: email new line by skipping extra div

16c67a7

ahmdshrif marked this pull request as ready for review December 15, 2022 15:13

ahmdshrif requested a review from a team as a code owner December 15, 2022 15:13

melvin-bot bot requested review from aimane-chnaif and Julesssss and removed request for a team December 15, 2022 15:13

aimane-chnaif suggested changes Dec 15, 2022

View reviewed changes

update the comment

6256506

aimane-chnaif previously approved these changes Dec 16, 2022

View reviewed changes

Julesssss reviewed Dec 16, 2022

View reviewed changes

re-wording comment

725f4ae

ahmdshrif dismissed aimane-chnaif’s stale review via 725f4ae December 16, 2022 12:40

Julesssss approved these changes Dec 16, 2022

View reviewed changes

Julesssss assigned ahmdshrif Dec 16, 2022

Julesssss merged commit fde8d11 into Expensify:main Dec 16, 2022

OSBotify mentioned this pull request Dec 16, 2022

Deploy Checklist: New Expensify 2022-12-16 #13654

Closed

25 tasks

melvin-bot bot mentioned this pull request Dec 20, 2022

[Needs regression test steps] [$1000] When copy pasting message containing email/links, email address/link occupies new line. #9250

Closed

0xmiros mentioned this pull request Apr 25, 2023

[HOLD for payment 2023-06-01] [$4000] Paste H1(# test) message in chat causes incorrect line breaks #16921

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: email new line by skipping extra div #13503

fix: email new line by skipping extra div #13503

ahmdshrif commented Dec 10, 2022 •

edited

Loading

ahmdshrif commented Dec 11, 2022

aimane-chnaif commented Dec 15, 2022

ahmdshrif commented Dec 15, 2022 •

edited

Loading

aimane-chnaif commented Dec 15, 2022

melvin-bot bot commented Dec 15, 2022

ahmdshrif commented Dec 15, 2022

aimane-chnaif commented Dec 15, 2022 •

edited

Loading

ahmdshrif commented Dec 15, 2022

aimane-chnaif Dec 15, 2022

ahmdshrif Dec 15, 2022 •

edited

Loading

ahmdshrif Dec 15, 2022

aimane-chnaif commented Dec 16, 2022

ahmdshrif commented Dec 16, 2022

aimane-chnaif commented Dec 16, 2022 •

edited

Loading

ahmdshrif commented Dec 16, 2022 •

edited

Loading

aimane-chnaif commented Dec 16, 2022

aimane-chnaif left a comment

Julesssss Dec 16, 2022

ahmdshrif Dec 16, 2022

Julesssss Dec 16, 2022

ahmdshrif Dec 16, 2022

ahmdshrif Dec 16, 2022

Julesssss commented Dec 16, 2022

github-actions bot commented Dec 16, 2022

OSBotify commented Dec 16, 2022

OSBotify commented Dec 20, 2022

tienifr commented Apr 24, 2023 •

edited

Loading

ahmdshrif commented Apr 24, 2023 •

edited

Loading

tienifr commented Apr 25, 2023

		// We are excluding divs that have only one child and no text nodes and don't have a tagAttribute.
		// This is due to the fact that these divs are meaningless to us and are interfering with the HTML to Markdown conversion process.

fix: email new line by skipping extra div #13503

fix: email new line by skipping extra div #13503

Conversation

ahmdshrif commented Dec 10, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

ahmdshrif commented Dec 11, 2022

aimane-chnaif commented Dec 15, 2022

ahmdshrif commented Dec 15, 2022 • edited Loading

aimane-chnaif commented Dec 15, 2022

melvin-bot bot commented Dec 15, 2022

ahmdshrif commented Dec 15, 2022

aimane-chnaif commented Dec 15, 2022 • edited Loading

ahmdshrif commented Dec 15, 2022

aimane-chnaif Dec 15, 2022

Choose a reason for hiding this comment

ahmdshrif Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

ahmdshrif Dec 15, 2022

Choose a reason for hiding this comment

aimane-chnaif commented Dec 16, 2022

ahmdshrif commented Dec 16, 2022

aimane-chnaif commented Dec 16, 2022 • edited Loading

ahmdshrif commented Dec 16, 2022 • edited Loading

aimane-chnaif commented Dec 16, 2022

Reviewer Checklist

Screenshots/Videos

aimane-chnaif left a comment

Choose a reason for hiding this comment

Julesssss Dec 16, 2022

Choose a reason for hiding this comment

ahmdshrif Dec 16, 2022

Choose a reason for hiding this comment

Julesssss Dec 16, 2022

Choose a reason for hiding this comment

ahmdshrif Dec 16, 2022

Choose a reason for hiding this comment

ahmdshrif Dec 16, 2022

Choose a reason for hiding this comment

Julesssss commented Dec 16, 2022

github-actions bot commented Dec 16, 2022

Performance Comparison Report 📊

Significant Changes To Duration

Meaningless Changes To Duration

OSBotify commented Dec 16, 2022

OSBotify commented Dec 20, 2022

tienifr commented Apr 24, 2023 • edited Loading

ahmdshrif commented Apr 24, 2023 • edited Loading

tienifr commented Apr 25, 2023

ahmdshrif commented Dec 10, 2022 •

edited

Loading

ahmdshrif commented Dec 15, 2022 •

edited

Loading

aimane-chnaif commented Dec 15, 2022 •

edited

Loading

ahmdshrif Dec 15, 2022 •

edited

Loading

aimane-chnaif commented Dec 16, 2022 •

edited

Loading

ahmdshrif commented Dec 16, 2022 •

edited

Loading

tienifr commented Apr 24, 2023 •

edited

Loading

ahmdshrif commented Apr 24, 2023 •

edited

Loading