Repair Editing a comment containing both RTL and LTR characters breaks the (edited) text #9834

Karim-30 · 2022-07-11T21:40:30Z

Details

Fixed Issues

$ #9709

Tests

Go to any chat
Type something containing RTL characters
Send the comment
Edit the comment and add LTR characters
Press Save changes
Verify (edited) text appears at the end of the line not between words.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Go to any chat
Type something containing RTL characters
Send the comment
Edit the comment and add LTR characters
Press Save changes
Verify (edited) text appears at the end of the line not between words.

Verify that no errors appear in the JS console

Screenshots

Web

web2.mov

Mobile Web

mweb2.mov

Desktop

Desktop2.mov

iOS

Ios2.mov

Android

Desktop2.mov

github-actions · 2022-07-11T21:40:50Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Karim-30 · 2022-07-11T21:42:22Z

I have read the CLA Document and I hereby sign the CLA

rushatgabhane · 2022-07-12T21:10:03Z

src/pages/home/report/ReportActionItemFragment.js

-                        style={EmojiUtils.containsOnlyEmojis(props.fragment.text) ? styles.onlyEmojisText : undefined}
-                    >
-                        {Str.htmlDecode(props.fragment.text)}
+                    <View style={[styles.flexRow, styles.alignItemsBaseline]}>


@Karim-30 unfortunately, justifyContent: baseline doesn't work well for a multiline message

Paste this message and edit it - "pen app on webPost(if not) some code block message with long lines so that it overflows.Try to move two finger (scroll) not precisely horizontally but at some other angle eg. 45Expected Result:
We have to precisely scroll to left or right or else the page will also scroll vertically."

updated, here how it looks like :

web :

web2.mov

mweb :

mweb2.mov

android :

android2.mov

ios :

Ios2.mov

desktop :

Desktop2.mov

@Karim-30 code keeps changing, so it isn't necessary to add updated screenshots :)

sketchydroide · 2022-07-13T14:21:21Z

@Karim-30 after you address @rushatgabhane concern I would be happy to review the code

rushatgabhane · 2022-07-14T02:51:39Z

src/pages/home/report/ReportActionItemFragment.js

@@ -106,9 +106,9 @@ const ReportActionItemFragment = (props) => {
                ) : (
                    <Text
                        selectable={!canUseTouchScreen() || !props.isSmallScreenWidth}
-                        style={EmojiUtils.containsOnlyEmojis(props.fragment.text) ? styles.onlyEmojisText : undefined}
+                        style={[EmojiUtils.containsOnlyEmojis(props.fragment.text) ? styles.onlyEmojisText : undefined, {writingDirection: 'ltr'}]}


@Karim-30 any context on where this came from? Link to some docs/stackoverflow would be great!

P.S. inline styles are forbidden https://github.com/Expensify/App/blob/main/contributingGuides/STYLING.md#inline-styles

writingDirection: 'ltr' fixes the issue on web, desktop, and IOS but not on android. docs

solved inline styles issue.

rushatgabhane · 2022-07-14T02:51:47Z

src/pages/home/report/ReportActionItemFragment.js

                    >
-                        {Str.htmlDecode(props.fragment.text)}
+                        {`\u2066${Str.htmlDecode(props.fragment.text)}`}


This one too

\u2066${Str.htmlDecode(props.fragment.text)} fixes the issue on Android. docs

I know these changes are different from what I proposed earlier, So if you find these not fit enough or prefer looking for other proposals no problem at all I can decline the Upwork offer from my side.

@Karim-30 Thank you!

I know these changes are different from what I proposed earlier, So if you find these not fit enough or prefer looking for other proposals no problem at all I can decline the Upwork offer from my side

no no, that's alright. We just want to get to the best possible solution. It doesn't matter if it's different from original proposal

rushatgabhane · 2022-07-19T03:27:59Z

src/pages/home/report/ReportActionItemFragment.js

+                        selectable={!canUseTouchScreen() || !props.isSmallScreenWidth}
+                        style={[EmojiUtils.containsOnlyEmojis(props.fragment.text) ? styles.onlyEmojisText : undefined, styles.ltr]}
+                    >
+                        {`\u2066${Str.htmlDecode(props.fragment.text)}`}


@Karim-30 can we also move this to a function? It should prepend \u2066 to a string
Make sure to write jsdoc for it too

sketchydroide · 2022-07-22T13:22:34Z

src/styles/utilities/writingDirection.js

+ */
+export default {
+    rtl: {
+        writingDirection: 'ltr',


This are both the same, not sure what the value of that is, of if this is a typo

sketchydroide · 2022-07-22T15:24:17Z

@rushatgabhane it seems you concerns have been met, are you happy with the PR?

rushatgabhane

@sketchydroide yep, looks great!

rushatgabhane

@Karim-30 noticed a few comment issues

rushatgabhane · 2022-07-23T11:58:01Z

src/styles/utilities/writingDirection.js

@@ -0,0 +1,14 @@
+/**
+ * Writing direction utility styles.
+ * note: writingDirection not available for Android, for Android you can use the Unicode.


Suggested change

* note: writingDirection not available for Android, for Android you can use the Unicode.

* Note: writingDirection isn't supported on Android. Unicode controls are being used for Android

rushatgabhane · 2022-07-23T11:58:04Z

src/styles/utilities/writingDirection.js

+/**
+ * Writing direction utility styles.
+ * note: writingDirection not available for Android, for Android you can use the Unicode.
+ *


remove extra line

Suggested change

*

rushatgabhane · 2022-07-23T12:00:03Z

src/styles/StyleUtils.js

@@ -448,6 +448,15 @@ function getPaddingLeft(paddingLeft) {
    };
 }

+/**
+ * Convert RTL text to a LTR text on Android devices useing the Unicode controls, see : https://www.w3.org/International/questions/qa-bidi-unicode-controls


Suggested change

* Convert RTL text to a LTR text on Android devices useing the Unicode controls, see : https://www.w3.org/International/questions/qa-bidi-unicode-controls

* Android only - convert RTL text to LTR text using Unicode controls.

* https://www.w3.org/International/questions/qa-bidi-unicode-controls

OSBotify · 2022-07-25T15:56:47Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-07-25T18:03:24Z

🚀 Deployed to staging by @sketchydroide in version: 1.1.86-0 🚀

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

ctkochan22 · 2022-07-26T22:09:20Z

Causing regression here: #10086

Why did we replaces text with props.fragment.text?

And on another note, is the new text for the edited comment supposed to go on the right side?

Karim-30 · 2022-07-27T16:29:33Z

@ctkochan22 I'm so sorry for this mistake, the reason is when I started this PR 15 days ago there was only props.fragment.txt in ReportActionItemFragment.js it looked like this:

App/src/pages/home/report/ReportActionItemFragment.js

Lines 90 to 121 in 0fcc492

    
           // If the only difference between fragment.text and fragment.html is <br /> tags 
        
           // we replace them with line breaks and render it as text, not as html. 
        
           // This is done to render emojis with line breaks between them as text. 
        
           const differByLineBreaksOnly = Str.replaceAll(props.fragment.html, '<br />', ' ') === props.fragment.text; 
        
           if (differByLineBreaksOnly) { 
        
               const textWithLineBreaks = Str.replaceAll(props.fragment.html, '<br />', '\n'); 
        
               // eslint-disable-next-line no-param-reassign 
        
               props.fragment = {...props.fragment, text: textWithLineBreaks, html: textWithLineBreaks}; 
        
           } 
        
           // Only render HTML if we have html in the fragment 
        
           return props.fragment.html !== props.fragment.text 
        
               ? ( 
        
                   <RenderHTML 
        
                       html={`<comment>${props.fragment.html + (props.fragment.isEdited ? '<edited></edited>' : '')}</comment>`} 
        
                   /> 
        
               ) : ( 
        
                   <Text 
        
                       selectable={!canUseTouchScreen() || !props.isSmallScreenWidth} 
        
                       style={EmojiUtils.containsOnlyEmojis(props.fragment.text) ? styles.onlyEmojisText : undefined} 
        
                   > 
        
                       {Str.htmlDecode(props.fragment.text)} 
        
                       {props.fragment.isEdited && ( 
        
                           <Text 
        
                               fontSize={variables.fontSizeSmall} 
        
                               color={themeColors.textSupporting} 
        
                           > 
        
                               {` ${props.translate('reportActionCompose.edited')}`} 
        
                           </Text> 
        
                       )} 
        
                   </Text> 
        
               );

one week later the PR fixes the empty lines bug https://github.com/Expensify/App/pull/9195/files which replace this.props.text with text was merged into Expensify/App, when I tried to merge the upstream here using fetch upstream button, GIt merged it without showing any conflict and this caused the trouble.

OSBotify · 2022-08-01T22:11:12Z

🚀 Deployed to production by @yuwenmemon in version: 1.1.86-5 🚀

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

mountiny · 2022-08-02T08:44:15Z

@Karim-30 The linked Pr has been incorrectly linked, please make sure there is the dollar sign always included before the link like so:

$ https://github.com/Expensify/App/issues/9709

Thank you!

fix RTL (edited) text

3bf8321

Karim-30 requested a review from a team as a code owner July 11, 2022 21:40

melvin-bot bot requested review from rushatgabhane and sketchydroide and removed request for a team July 11, 2022 21:40

Karim-30 added 2 commits July 12, 2022 22:06

fix RTL (edited) text

c30e992

Merge branch 'main' of https://github.com/Karim-30/App into main

556506d

rushatgabhane reviewed Jul 12, 2022

View reviewed changes

Karim-30 added 2 commits July 13, 2022 19:46

Merge branch 'main' of https://github.com/Karim-30/App into main

1e9514e

Merge branch 'main' of https://github.com/Karim-30/App into main

c794059

rushatgabhane suggested changes Jul 14, 2022

View reviewed changes

Karim-30 added 2 commits July 15, 2022 11:08

create writing Direction utility

71bb61a

Merge branch 'main' into main

84b3394

rushatgabhane reviewed Jul 19, 2022

View reviewed changes

Karim-30 added 4 commits July 19, 2022 22:17

Merge branch 'Expensify:main' into main

e7af093

Merge branch 'Expensify:main' into main

5dfd58c

Add LTR unicode function

d1fda9f

Add LTR unicode function

80ef0c2

sketchydroide reviewed Jul 22, 2022

View reviewed changes

fix rtl typo

3a3a820

rushatgabhane approved these changes Jul 23, 2022

View reviewed changes

rushatgabhane suggested changes Jul 23, 2022

View reviewed changes

fix comments typo

7800d79

rushatgabhane approved these changes Jul 24, 2022

View reviewed changes

sketchydroide approved these changes Jul 25, 2022

View reviewed changes

sketchydroide merged commit 0a3a425 into Expensify:main Jul 25, 2022

OSBotify mentioned this pull request Jul 25, 2022

Deploy Checklist: New Expensify 2022-07-25 #10077

Closed

92 tasks

ctkochan22 mentioned this pull request Jul 26, 2022

Web - Line breaks & Text from code editor do not appear correctly within chat #10086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repair Editing a comment containing both RTL and LTR characters breaks the (edited) text #9834

Repair Editing a comment containing both RTL and LTR characters breaks the (edited) text #9834

Karim-30 commented Jul 11, 2022 •

edited by mountiny

Loading

github-actions bot commented Jul 11, 2022 •

edited

Loading

Karim-30 commented Jul 11, 2022

rushatgabhane Jul 12, 2022 •

edited

Loading

Karim-30 Jul 13, 2022 •

edited

Loading

rushatgabhane Jul 14, 2022

sketchydroide commented Jul 13, 2022

rushatgabhane Jul 14, 2022

rushatgabhane Jul 14, 2022

Karim-30 Jul 14, 2022

Karim-30 Jul 15, 2022

rushatgabhane Jul 14, 2022

Karim-30 Jul 14, 2022

rushatgabhane Jul 14, 2022

rushatgabhane Jul 19, 2022 •

edited

Loading

Karim-30 Jul 20, 2022

sketchydroide Jul 22, 2022

Karim-30 Jul 22, 2022

sketchydroide commented Jul 22, 2022

rushatgabhane left a comment •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane Jul 23, 2022 •

edited

Loading

Karim-30 Jul 23, 2022

rushatgabhane Jul 23, 2022 •

edited

Loading

Karim-30 Jul 23, 2022

rushatgabhane Jul 23, 2022

Karim-30 Jul 23, 2022

OSBotify commented Jul 25, 2022

OSBotify commented Jul 25, 2022

ctkochan22 commented Jul 26, 2022

Karim-30 commented Jul 27, 2022

OSBotify commented Aug 1, 2022

mountiny commented Aug 2, 2022

	* note: writingDirection not available for Android, for Android you can use the Unicode.
	* Note: writingDirection isn't supported on Android. Unicode controls are being used for Android

	* Convert RTL text to a LTR text on Android devices useing the Unicode controls, see : https://www.w3.org/International/questions/qa-bidi-unicode-controls
	* Android only - convert RTL text to LTR text using Unicode controls.
	* https://www.w3.org/International/questions/qa-bidi-unicode-controls

Repair Editing a comment containing both RTL and LTR characters breaks the (edited) text #9834

Repair Editing a comment containing both RTL and LTR characters breaks the (edited) text #9834

Conversation

Karim-30 commented Jul 11, 2022 • edited by mountiny Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Jul 11, 2022 • edited Loading

Karim-30 commented Jul 11, 2022

rushatgabhane Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Karim-30 Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sketchydroide commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sketchydroide commented Jul 22, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jul 25, 2022

OSBotify commented Jul 25, 2022

ctkochan22 commented Jul 26, 2022

Karim-30 commented Jul 27, 2022

OSBotify commented Aug 1, 2022

mountiny commented Aug 2, 2022

Karim-30 commented Jul 11, 2022 •

edited by mountiny

Loading

github-actions bot commented Jul 11, 2022 •

edited

Loading

rushatgabhane Jul 12, 2022 •

edited

Loading

Karim-30 Jul 13, 2022 •

edited

Loading

rushatgabhane Jul 19, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane Jul 23, 2022 •

edited

Loading

rushatgabhane Jul 23, 2022 •

edited

Loading