Fixed Android - Chat - Message gets displayed from right to left #32297

samilabud · 2023-11-30T19:33:08Z

Details

The root cause is that Android only converts RTL text to LTR text using Unicode controls and by default, when you start writing in a language that is written in RTL mode it converts the input text to RTL.

Fixed Issues

$ #30584
PROPOSAL: #30584 (comment)

Tests

Launch App
Open any chat and paste an RTL text in the compose box, for example " مثال "
Continue typing some content

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Launch App
Open any chat and paste an RTL text in the compose box, for example " مثال "
Continue typing some content

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android.Native.mov

Android: mWeb Chrome

Android.Chrome.mov

iOS: Native

Iphone.Native.mp4

iOS: mWeb Safari

Iphone.Safari.mov

MacOS: Chrome / Safari

Web.Chrome.mp4

MacOS: Desktop

MAC.Desktop.mov

melvin-bot · 2023-11-30T19:33:13Z

@allroundexperts Please 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]

allroundexperts · 2023-12-03T19:44:50Z

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:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
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.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Screen.Recording.2023-12-04.at.12.35.43.AM.mov

Android: mWeb Chrome

Screen.Recording.2023-12-04.at.12.34.17.AM.mov

iOS: Native

Screen.Recording.2023-12-04.at.12.32.33.AM.mov

iOS: mWeb Safari

Screen.Recording.2023-12-04.at.12.32.00.AM.mov

MacOS: Chrome / Safari

MacOS: Desktop

Screen.Recording.2023-12-04.at.12.30.32.AM.mov

allroundexperts

Looks good!

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

HardikChoudhary24 · 2023-12-05T11:38:41Z

@samilabud @allroundexperts As I had worked on this for other platforms previously, I was getting some issues, and similar issues are visible in this change also:

Mention Suggestions
Draft comments
Sending an empty message
No placeholder for the composer

23-12-05-16-38-32.mp4

allroundexperts · 2023-12-05T12:07:21Z

Thanks for testing this out @HardikChoudhary24. In the screenshots I attached, I can see that the placeholder was appearing at-least 🤔. @samilabud Can you please check the issues that @HardikChoudhary24 pointed out?

samilabud · 2023-12-05T16:11:52Z

Good catch @HardikChoudhary24 thank you!.

@allroundexperts currently this works better, please check it out:

Testing.messages.gets.displayed.from.right.to.left.mp4

tgolen

I feel like the diff changed by an order of magnitude since the last time I looked at it and now I don't really follow the scope of the solution.

Are the problems mentioned by @HardikChoudhary24 caused by your PR, or are they existing bugs that are being fixed in this PR?

I think if they are regressions caused by your PR, they should be fixed in this PR. If they are existing bugs, then we should open up a new GH to track and fix them independently.

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

samilabud · 2023-12-06T13:51:32Z

I feel like the diff changed by an order of magnitude since the last time I looked at it and now I don't really follow the scope of the solution.

Are the problems mentioned by @HardikChoudhary24 caused by your PR, or are they existing bugs that are being fixed in this PR?

I think if they are regressions caused by your PR, they should be fixed in this PR. If they are existing bugs, then we should open up a new GH to track and fix them independently.

Hi, issues mentioned by @HardikChoudhary24 are caused by my PR but it looks like they tried this before and didn't works or something.

I have fixed every scenario inside the corresponding files to make this change affect only android mobiles, also I have added comments that explain the why of this changes, please check them out 🙏🏼

tgolen

I know this is a lot of comments, but the changes in this PR really went down an interesting path!

src/libs/convertToLTR/index.android.ts

src/libs/convertToLTR/index.ts

src/libs/convertToLTRForComposer/index.android.ts

src/libs/convertToLTR/index.android.ts

src/libs/convertToLTR/index.ts

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js

samilabud · 2023-12-06T21:07:43Z

I know this is a lot of comments, but the changes in this PR really went down an interesting path!

Thanks for the suggestions, I think I've addressed everything, please check back when you get a chance 🙏🏼

…naming variable from newCommentConverted to newCommentConvertedToLTR

samilabud · 2023-12-08T13:13:51Z

Hi @tgolen, I think I finished with the changes please double-check because I marked the conversations as resolved and currently I see just 2 comments from you, could you please mark them as unsolved when you add a new comment? or maybe it is better to never mark comments as resolved 🤔.

BTW when I was testing I found a new bug (this is not related to this PR, the test was in https://staging.new.expensify.com):

Staging.Bug.in.Web.Chrome.mp4

Do you know where we can report this? I think we can fix it too.

tgolen · 2023-12-08T15:23:44Z

For bugs not related to your PR, you can post them in the slack channel #expensify-bugs.

For the comments, I would usually never manually mark things as resolved. I know there were some issues with my previous review and having multiple comments, so I will go through it this morning and clean it up and give another review.

samilabud · 2023-12-08T15:29:13Z

For bugs not related to your PR, you can post them in the slack channel #expensify-bugs.

For the comments, I would usually never manually mark things as resolved. I know there were some issues with my previous review and having multiple comments, so I will go through it this morning and clean it up and give another review.

Got it, thank you!

tgolen · 2023-12-08T17:40:24Z

@allroundexperts do you want to give another testing pass on this to ensure it's still in working order before it's merged? There were quite a few changes since your initial testing I think.

allroundexperts · 2023-12-08T17:41:37Z

Sure thing @tgolen. I'll retest this tonight/tomorrow.

samilabud · 2023-12-13T18:56:51Z

I did some tests after the last change and everything seems to be ok:

Testing.regex.variable.names.mp4

tgolen

It's looking better! I think we just need to clean up some of the grammar and comment formatting and then this is good.

tgolen · 2023-12-13T23:08:09Z

src/libs/convertToLTRForComposer/index.android.ts

+ *  Sending an empty message;
+ *  Mention suggestions not works if @ or \s (at or space) is the first character;
+ *  Placeholder is not displayed if the unicode character is the only character remaining;
+ * force: always remove the LTR unicode, going to be used when composer is consider as empty */


The */ needs to go on the next line by itself.

tgolen · 2023-12-13T23:08:54Z

src/libs/convertToLTRForComposer/index.android.ts

+ *  Sending an empty message;
+ *  Mention suggestions not works if @ or \s (at or space) is the first character;
+ *  Placeholder is not displayed if the unicode character is the only character remaining;
+ * force: always remove the LTR unicode, going to be used when composer is consider as empty */


If the force: comment is for the force param, please format it as a legit @param doc.

tgolen · 2023-12-13T23:10:49Z

src/libs/convertToLTRForComposer/index.android.ts

 import ConvertToLTRForComposer from './types';

+/**
+ * Android only - The composer can be converted to LTR if its content is the LTR character followed by an @ or space


This comment should also explain why it's OK to convert it in these cases and why it shouldn't be converted. Consider adding "because..." and then fill out the rest.

tgolen · 2023-12-13T23:11:06Z

src/libs/convertToLTRForComposer/index.android.ts

+ * Android only - The composer can be converted to LTR if its content is the LTR character followed by an @ or space
+ */
+function canComposerBeConvertedToLTR(text: string): boolean {
+    // this handle cases when user type only spaces


Please start comments with a capital letter

"this handle cases" is not good grammar and should be more like "This regex handles the case when a user only types spaces into the composer.

tgolen · 2023-12-13T23:11:19Z

src/libs/convertToLTRForComposer/index.android.ts

+function canComposerBeConvertedToLTR(text: string): boolean {
+    // this handle cases when user type only spaces
+    const containOnlySpaces = /^\s*$/;
+    // this handle the case where someone has RTL enabled and they began typing an @mention for someone.


Do comments end with a period or not? Let's be consistent

tgolen · 2023-12-13T23:12:33Z

src/libs/convertToLTRForComposer/index.android.ts

+    const containOnlySpaces = /^\s*$/;
+    // this handle the case where someone has RTL enabled and they began typing an @mention for someone.
+    const startsWithLTRAndAt = new RegExp(`^${CONST.UNICODE.LTR}@$`);
+    // this handle cases could send empty messages when composer is multiline


I'm not quite sure what this means and the grammar is a bit confusing. Are you trying to say this?

This regex handles the case where the composer can contain multiple lines of whitespace.

That's right, sorry 😣!

samilabud · 2023-12-14T14:40:51Z

Hi @tgolen, please verify again when you get a chance.

tgolen · 2023-12-15T14:47:58Z

src/libs/convertToLTRForComposer/index.android.ts

+ * @newComment: the comment written by the user
+ * @force: always remove the LTR unicode, going to be used when composer is consider as empty */


These need to follow the JSDocs format: https://jsdoc.app/tags-param.

The */ is still on the wrong line.

Sorry, thanks for the link, I appreciate it (TIL).
Done.

tgolen · 2023-12-17T16:05:29Z

All yours @allroundexperts

allroundexperts · 2023-12-19T14:15:58Z

@samilabud Now I am getting the following issue:

Screen.Recording.2023-12-19.at.7.13.21.PM.mov

samilabud · 2023-12-19T14:23:11Z

@samilabud Now I am getting the following issue:

Screen.Recording.2023-12-19.at.7.13.21.PM.mov

Hi, do you mean the delay where text takes to be from left to right? If so, this seems to be the expect behavior due android has not transitions when we convert the input to LTR from RTL

allroundexperts · 2023-12-19T14:26:00Z

@samilabud I'm talking about the text suddenly moving from right to left. This did not happen in your previous versions. You can also confirm this by watching your / mine screen recordings for Android.

samilabud · 2023-12-19T14:34:34Z

@samilabud I'm talking about the text suddenly moving from right to left. This did not happen in your previous versions. You can also confirm this by watching your / mine screen recordings for Android.

Got it, I have noticed this behavior from beginning but it seems related to emulator/device available resources, this happen even in web version with a bug I reported previously in this thread.

samilabud · 2023-12-19T14:45:57Z

@allroundexperts I'm thinking maybe you mean the way it was it the beginning that we never allow the input to be converted to RTL but this causes mentions to not work, or blank messages to be sent, and so on. If this is a big issue please let me know so I can try to find another way to convert the input.

tgolen · 2023-12-21T15:24:29Z

What's the status with this?

@allroundexperts I think you are suggesting that earlier commits in this PR didn't have the delay. Is that right?

samilabud · 2023-12-21T16:55:05Z

Yes, this has been happening since the first commits (see the video at sec 3 to this comment) but maybe you don't notice because this depends on the resources available:

The reason is that we previously converted (in the proposal) the input before the user started typing (adding the Unicode), and now we have to wait for the user to type (to add the Unicode if has the conditions - to avoid sending an empty message, or mentions and placeholder works correctly).

In summary, as we have to wait for the user to write it is noticeable when Android converts the input from RTL to LTR.

tgolen · 2023-12-21T17:31:47Z

I think that is OK to live with for now. Thank you for explaining! @allroundexperts are you OK for me to merge this then?

allroundexperts · 2023-12-22T02:14:50Z

@tgolen If that's fine, then I think we can merge this.

tgolen · 2023-12-26T15:41:38Z

Late Christmas present :D I wish I could have merged this yesterday 🎄 🎅

OSBotify · 2023-12-26T15:45:07Z

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

samilabud · 2023-12-26T15:52:49Z

Late Christmas present :D I wish I could have merged this yesterday 🎄 🎅

Thank you all, merry late Christmas! 🎄🎁 😊

OSBotify · 2023-12-28T21:18:57Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.18-8 🚀

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

samilabud requested a review from a team as a code owner November 30, 2023 19:33

melvin-bot bot requested review from allroundexperts and removed request for a team November 30, 2023 19:33

allroundexperts approved these changes Dec 3, 2023

View reviewed changes

melvin-bot bot requested a review from tgolen December 3, 2023 19:45

tgolen requested changes Dec 4, 2023

View reviewed changes

src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js Outdated Show resolved Hide resolved

samilabud added 2 commits December 4, 2023 18:56

Fixed Android - Chat - Message gets displayed from right to left

3a10688

Removed ignore lint rule and doing the early return

7c11e93

samilabud force-pushed the android_chat_displayed_right_to_left_30584 branch from c81a9fe to 7c11e93 Compare December 4, 2023 23:13

Fixed early return when useEffect bug

fbd37e8

Fixed bugs: when mention, sending empty messages, no placeholder

5ffd754

tgolen requested changes Dec 5, 2023

View reviewed changes

Added comments and refactoring code to better understanding

59f4466

Fixed prettier errors

431d7e2

tgolen requested changes Dec 6, 2023

View reviewed changes

Fixed observations, added suggestions and added extra comments

33b5733

Moving functions from convertToLTR to convertToLTRForComposer lib, re…

89d738e

…naming variable from newCommentConverted to newCommentConvertedToLTR

tgolen previously approved these changes Dec 8, 2023

View reviewed changes

Comments and variables changed to better understanding

e737dcb

tgolen reviewed Dec 13, 2023

View reviewed changes

samilabud added 2 commits December 13, 2023 23:46

Fixed grammar in comments

be6ee76

Fixed comments grammar and using @param doc

5be5dc0

tgolen requested changes Dec 15, 2023

View reviewed changes

Using jsdoc format for comments

8a75507

tgolen approved these changes Dec 17, 2023

View reviewed changes

tgolen merged commit 91adf38 into Expensify:main Dec 26, 2023
16 checks passed

github-actions bot mentioned this pull request Dec 27, 2023

Deploy Checklist: New Expensify 2023-12-27 #33644

Closed

41 tasks

neil-marcellini mentioned this pull request Dec 27, 2023

[CP staging] Revert "Fixed Android - Chat - Message gets displayed from right to left" #33677

Merged

lanitochka17 mentioned this pull request Dec 28, 2023

Android - Chat - RTL text jumps from right to left in compose box after pasting #33681

Closed

6 tasks

samilabud deleted the android_chat_displayed_right_to_left_30584 branch April 29, 2024 16:33

		* @newComment: the comment written by the user
		* @force: always remove the LTR unicode, going to be used when composer is consider as empty */

Fixed Android - Chat - Message gets displayed from right to left #32297

Fixed Android - Chat - Message gets displayed from right to left #32297

Conversation

samilabud commented Nov 30, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Nov 30, 2023

allroundexperts commented Dec 3, 2023

Reviewer Checklist

Screenshots/Videos

allroundexperts left a comment

Choose a reason for hiding this comment

HardikChoudhary24 commented Dec 5, 2023 • edited Loading

allroundexperts commented Dec 5, 2023

samilabud commented Dec 5, 2023

tgolen left a comment

Choose a reason for hiding this comment

samilabud commented Dec 6, 2023 • edited Loading

tgolen left a comment

Choose a reason for hiding this comment

samilabud commented Dec 6, 2023

samilabud commented Dec 8, 2023

tgolen commented Dec 8, 2023

samilabud commented Dec 8, 2023

tgolen commented Dec 8, 2023

allroundexperts commented Dec 8, 2023

samilabud commented Dec 13, 2023

tgolen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samilabud commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Dec 17, 2023

allroundexperts commented Dec 19, 2023

samilabud commented Dec 19, 2023

allroundexperts commented Dec 19, 2023

samilabud commented Dec 19, 2023

samilabud commented Dec 19, 2023

tgolen commented Dec 21, 2023

samilabud commented Dec 21, 2023

tgolen commented Dec 21, 2023

allroundexperts commented Dec 22, 2023

tgolen commented Dec 26, 2023

OSBotify commented Dec 26, 2023

samilabud commented Dec 26, 2023

OSBotify commented Dec 28, 2023

samilabud commented Nov 30, 2023 •

edited

Loading

HardikChoudhary24 commented Dec 5, 2023 •

edited

Loading

samilabud commented Dec 6, 2023 •

edited

Loading