Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

[HOLD for payment 2024-01-04] [$500] Android - Chat - Message gets displayed from right to left #30584

Closed lanitochka17 closed 8 months ago

lanitochka17 commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.3.93-0 Reproducible in staging?:Y Reproducible in production?:Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Issue found when executing PR https://github.com/Expensify/App/pull/30521

Action Performed:

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

Expected Result:

Verify that the content gets displayed from left to right

Actual Result:

The content gets displayed from right to left

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Android: Native ![Bug6256757_1698673344323!30521_Android](https://github.com/Expensify/App/assets/78819774/1475c32b-36ad-4bb3-b3ed-10cc860f8496) https://github.com/Expensify/App/assets/78819774/d7ac6f63-be15-4b10-9a4b-58e66b55d53a
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01278feafdf81fcb73
  • Upwork Job ID: 1718998877593833472
  • Last Price Increase: 2023-11-20
  • Automatic offers:
    • samilabud | Contributor | 27900958
Issue OwnerCurrent Issue Owner: @johncschuster
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01278feafdf81fcb73

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

Krishna2323 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - Chat - Message gets displayed from right to left

What is the root cause of that problem?

This issue only happens on few android devices so the root cause is unclear.

What changes do you think we should make in order to solve the problem?

We can set selection ({start: 0}) when inputText onblur. like this answer suggested or we can use I18nManager to solve this issue like:

import { I18nManager } from "react-native";

I18nManager.forceRTL(false);
I18nManager.allowRTL(false);

Result

situchan commented 1 year ago

cc: @HardikChoudhary24

HardikChoudhary24 commented 1 year ago

For android it will be working like that it is mentioned in comment. Also this can be seen for other apps like linkedin where the web version is having the desired result and their android app is not showing it in LTR format

allroundexperts commented 1 year ago

Hm... @HardikChoudhary24 I see your comment, but I think this is still a bug. I guess we can keep this open and see if we can get a concrete fix in place?

HardikChoudhary24 commented 1 year ago

Sure @allroundexperts but this is something that is not supported in android as per my knowledge. This can be seen in other apps like LinkedIn, discord, and Puma. These are the ones that I have tested and found that they are displaying it in the desired format on web but not on native android.

samilabud commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Chat - Message gets displayed from right to left

What is the root cause of that problem?

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.

What changes do you think we should make in order to solve the problem?

The best solution is to add \u2066 unicode character before starting to write, I suggest verifying (when the user is using Android) if the input value is empty and doing the next in the ComposerWithSuggestions component:

https://github.com/Expensify/App/blob/5d45f0f6c86e97d4ae58ac167b6c162a610b817c/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions.js.

useEffect(() => {
        if (value === '') {
            setValue(convertToLTR(value));
        }
    }, [value]);

The convertToLTR function adds the unicode character needed for this to work in Android.

This way the input in Android never changes to direction = RTL, please see the result:

https://github.com/Expensify/App/assets/5216036/1b3b1a36-a98a-4086-b9f6-7d8e9961719a

samilabud commented 1 year ago

Proposal

Updated

johncschuster commented 1 year ago

@allroundexperts can you take a look at the updated proposal above when you get a moment?

melvin-bot[bot] commented 1 year ago

@johncschuster, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

allroundexperts commented 1 year ago

@samilabud's proposal seems to work well.

@HardikChoudhary24 did you explore this solution before? If so, do you know if it has some shortcomings? cc @situchan as well since you seem to have been involved in that conversation.

fabOnReact commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The text is displayed right to left.

What is the root cause of that problem?

The ReactRoot view default textDirection is firstStrong (screenshot1, screenshot2)

https://developer.android.com/reference/android/view/View.html#attr_android:textDirection

firstStrong
Default for the root view. The first strong directional character determines the paragraph direction. 
If there is no strong directional character, the paragraph direction is the view\u2019s resolved layout direction.

Testing textDirection: firstStrong and textDirection: ltr on Android

https://developer.android.com/reference/android/view/View.html#attr_android:textDirection

https://github.com/fabriziobertoglio1987/text-input-cursor-flickering-android/blob/31db5796c6abbd17ffaa87de1663c38604d1da33/app/src/main/java/com/example/javaexample/CustomEditText.java#L29-L43

TextDirection firstStrong TextDirection ltr

What changes do you think we should make in order to solve the problem?

Expose textDirection style prop in the React Native API to allow changing the Views (View, TextInput, Text) textDirection.

1) Add a JavaScript textDirection style prop to the React Native API:

<View style={{textDirection: "ltr"}}>
   <TextInput style={{textDirection: "inherit"}}>The TextInput inherits the style from the parent</TextInput>
   <TextInput style={{textDirection: "firstStrong"}}>The TextInput content aligns based on the first character content</TextInput>
</View>

2) Set the textDirection of the Android View using setTextDirection (android code snippet).

melvin-bot[bot] commented 1 year ago

@johncschuster, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 year ago

@johncschuster, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

HardikChoudhary24 commented 1 year ago

@samilabud's proposal seems to work well.

@HardikChoudhary24 did you explore this solution before? If so, do you know if it has some shortcomings? cc @situchan as well since you seem to have been involved in that conversation.

@allroundexperts we are already adding \u2066 before the RTL characters to make them appear as LTR in convertToLTRForComposer function. It is avoided in case of android because the composer does not behave as expected, as seen below.

https://github.com/Expensify/App/assets/126149345/0da6a1dd-42f5-4818-b24d-a0db7febcfb0

samilabud commented 1 year ago

@HardikChoudhary24, we are adding the Unicode \u2066 when we send the message, my proposal is to add it before starting to write and when the user is using Android.

Please see how it looks in Android:

https://github.com/Expensify/App/assets/5216036/1b3b1a36-a98a-4086-b9f6-7d8e9961719a

allroundexperts commented 1 year ago

@samilabud Why doesn't the message show up in the chat once you send it in the above video?

samilabud commented 1 year ago

@samilabud Why doesn't the message show up in the chat once you send it in the above video?

Oh that's was for my connection (was slow) and here the video should be max 10mb 😅, but I can share another video if you want.

HardikChoudhary24 commented 1 year ago

@HardikChoudhary24, we are adding the Unicode \u2066 when we send the message, my proposal is to add it before starting to write and when the user is using Android.

Please see how it looks in Android:

Android.chat.messgage.gets.displayed.from.right.to.left.done.mov

Yes that is what we are doing using the convertToLTRForComposer function. We are not adding it when we send the message, we add it as we start to write.

melvin-bot[bot] commented 1 year ago

@johncschuster @allroundexperts this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

@johncschuster @allroundexperts this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

@johncschuster @allroundexperts this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

samilabud commented 1 year ago

@HardikChoudhary24, we are adding the Unicode \u2066 when we send the message, my proposal is to add it before starting to write and when the user is using Android. Please see how it looks in Android: Android.chat.messgage.gets.displayed.from.right.to.left.done.mov

Yes that is what we are doing using the convertToLTRForComposer function. We are not adding it when we send the message, we add it as we start to write.

As far as I can see we are using this function to do what you say: https://github.com/Expensify/App/blob/0d048314505f20f1c77c3d27ba3845f566943ddb/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.js#L230

But this function for Android returns the same text (text) => text:

https://github.com/Expensify/App/blob/0d048314505f20f1c77c3d27ba3845f566943ddb/src/libs/convertToLTRForComposer/index.android.ts#L7

Perhaps the confusion is because there is another function with a similar name that does what you say, which is this one: https://github.com/Expensify/App/blob/main/src/libs/convertToLTR/index.android.ts

PD: When using Android we can't add the Unicode when the user is writing because the input control does not change its behavior, when must do that before writing see what I mean:

Unicode added before writing (in Android):

https://github.com/Expensify/App/assets/5216036/09f46832-629c-45ec-9cfb-311728fe6f3e

allroundexperts commented 1 year ago

@HardikChoudhary24 Can you please share the link to that conversation? I would like to go through it once before choosing the way forward here. Thanks

HardikChoudhary24 commented 1 year ago

@allroundexperts in this PR its discussed over here

samilabud commented 12 months ago

@HardikChoudhary24, we are adding the Unicode \u2066 when we send the message, my proposal is to add it before starting to write and when the user is using Android. Please see how it looks in Android: Android.chat.messgage.gets.displayed.from.right.to.left.done.mov

Yes that is what we are doing using the convertToLTRForComposer function. We are not adding it when we send the message, we add it as we start to write.

Sorry I was looking at your PR, specifically this commit, and it looks like when the user is using Android it does nothing:

image

(This remains the same in the main branch)

melvin-bot[bot] commented 12 months ago

@johncschuster @allroundexperts this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 12 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 11 months ago

@johncschuster, @allroundexperts 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

johncschuster commented 11 months ago

@allroundexperts what are your thoughts after reviewing the discussion @HardikChoudhary24 linked to in this comment?

allroundexperts commented 11 months ago

Okay. It seems like @samilabud's proposal works well unlike what @HardikChoudhary24 showed in there video. Let's proceed with @samilabud's proposal!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 11 months ago

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 11 months ago

@tgolen @johncschuster @allroundexperts this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 11 months ago

Current assignee @allroundexperts is eligible for the Internal assigner, not assigning anyone new.

tgolen commented 11 months ago

I'm OK with that proposal. Assigning this issue to @samilabud

tgolen commented 11 months ago

@samilabud Are you working on a PR for this?

samilabud commented 11 months ago

@samilabud Are you working on a PR for this?

Not yet, can I start? I have not received an offer yet

tgolen commented 11 months ago

Oh, you're right. Strange... I will try to reassign you and see if maybe the automation is broken

melvin-bot[bot] commented 11 months ago

Current assignee @allroundexperts is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 11 months ago

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($500)

melvin-bot[bot] commented 11 months ago

📣 @samilabud 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

tgolen commented 11 months ago

There we go. It was because of the Internal label and not External.

samilabud commented 11 months ago

There we go. It was because of the Internal label and not External.

Got it thanks 👍

melvin-bot[bot] commented 11 months ago

@tgolen, @samilabud, @johncschuster, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

tgolen commented 11 months ago

We are still working through things on the PR.