App icon indicating copy to clipboard operation
App copied to clipboard

[$2000] Android - App adds additional letters after selecting a suggested emoji and pressing Enter (gboard)

Open kavimuru opened this issue 2 years ago • 94 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open the Android app
  2. Open any chat
  3. Type colon and 2 letters to get emoji suggestions (eg: ':he',':jo',':bl')
  4. Select any one suggested emoji and press enter

Expected Result:

You should go to the next line (this is typical "Enter" behaviour)

Actual Result:

App goes to next line but adds previously typed 2 letters after the emoji (see video below)

Workaround:

Unknown

Platforms:

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

  • [x] Android / native
  • [ ] Android / Chrome
  • [ ] iOS / native
  • [ ] iOS / Safari
  • [ ] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop

Version Number: 1.2.88-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 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/226214883-7c2c6493-01c5-4e54-a2c3-36c6c329610a.mp4

https://user-images.githubusercontent.com/43996225/226214886-2e91473f-7288-485a-a7af-58629075b170.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679074253884739

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010623c6559be6640a
  • Upwork Job ID: 1638426418352893952
  • Last Price Increase: 2023-04-24

kavimuru avatar Mar 19 '23 22:03 kavimuru

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

MelvinBot avatar Mar 19 '23 22:03 MelvinBot

Bug0 Triage Checklist (Main S/O)

  • [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • [ ] This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • [ ] This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot avatar Mar 19 '23 22:03 MelvinBot

I am heading out of office for the next 3 days, reassigning.

davidcardoza avatar Mar 21 '23 23:03 davidcardoza

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

MelvinBot avatar Mar 21 '23 23:03 MelvinBot

Bug0 Triage Checklist (Main S/O)

  • [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • [x] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • [x] This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • [x] This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot avatar Mar 21 '23 23:03 MelvinBot

What a weird bug, I can repro on v88 (Pixel 3a).

jliexpensify avatar Mar 22 '23 06:03 jliexpensify

Job added to Upwork: https://www.upwork.com/jobs/~010623c6559be6640a

MelvinBot avatar Mar 22 '23 06:03 MelvinBot

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

MelvinBot avatar Mar 22 '23 06:03 MelvinBot

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

MelvinBot avatar Mar 22 '23 06:03 MelvinBot

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

MelvinBot avatar Mar 22 '23 06:03 MelvinBot

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

Android - App adds additional letters after selecting a suggested emoji and pressing Enter(gboard)

What is the root cause of that problem ?

As per updateComment in the below file https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionCompose.js#L518 if you are updating emojis using : ear : (start and end with colon without space) it will show proper emojis without any additional letters but if you are updating :ear (only start with colon and select from suggestion list), it will update as an emoji icon + ear like (👂 ear ) It will show the additional letter if you are updating emojis without closing colon like (:ear)

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

For this issue, you need to create a platform specific java file to reset the keyboard input value. Download the file from the below path or create a new file RNTextInputResetPackage.java and RNTextInputResetModule.java in the below mention path: Expensify/android/app/src/main/java/com/expensify/chat/

Download from here: https://drive.google.com/file/d/1C6i56Cq3QE-rkUBHMz17gx2OjtGEQDcY/view?usp=share_link https://drive.google.com/file/d/1vPZJRIV6qLQCoMAlfEshJ9qvCm5V-KAB/view?usp=share_link

You need to add the package name in mentioned file: https://github.com/Expensify/App/blob/main/android/app/src/main/java/com/expensify/chat/MainApplication.java#L41 packages.add(new RNTextInputResetPackage());

Now call the native module to react native files: Replace below file code: https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionCompose.js#L3-L8

Import {
    View,
    TouchableOpacity,
    InteractionManager,
    LayoutAnimation,
     NativeModules,
    findNodeHandle
} from 'react-native';
const { RNTextInputReset } = NativeModules;

Now call the native module to clear the keyboard input value, add RNTextInputReset && RNTextInputReset.resetKeyboardInput(findNodeHandle(this.textInput));

after this.updateComment() in below file: https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionCompose.js#L448

Video file: https://drive.google.com/file/d/1k-AxB0Cz3N4JXefvskyuLnck7zELukyL/view?usp=share_link

kaushiktd avatar Mar 23 '23 06:03 kaushiktd

I can reproduce it with the Pixel 2 simulator but not the SM Flip 3 physical device.


@kaushiktd I try your solution, but it's still not working, and your proposal does not explain why selecting the emoji with :jo or :ea is adding the text again but not with :joy or :ear.

Here I'm applying your solution.

https://user-images.githubusercontent.com/25520267/227272387-5cdd6a72-0934-415e-b0ca-983f391dada2.mov

mollfpr avatar Mar 23 '23 16:03 mollfpr

@mollfpr See the video here, https://drive.google.com/file/d/1e1zo_yAowYTd9pIuR4YmEUGm3MNn9BkQ/view?usp=share_link

kaushiktd avatar Mar 23 '23 18:03 kaushiktd

@kaushiktd Have you try with :jo or :ea like in my video?

mollfpr avatar Mar 23 '23 18:03 mollfpr

@mollfpr You are correct. Will come up with a new solution. Thanks.

kaushiktd avatar Mar 24 '23 04:03 kaushiktd

@mollfpr updated my proposal, https://github.com/Expensify/App/issues/16106#issuecomment-1480687210

kaushiktd avatar Mar 24 '23 06:03 kaushiktd

@kaushiktd Thanks for the update! I see that you are removing the extra selection for the space, although it's fixing this issue we are missing the extra space after the emoji which is important.

mollfpr avatar Mar 24 '23 11:03 mollfpr

Proposal

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

App adds additional letters after selecting a suggested emoji and pressing Enter on some Android devices.

What is the root cause of that problem?

This is a known issue with RN / Android where on some android keyboards the contents of a TextInput can not be properly cleared when an input value is changed dynamically. More about this can be read here and here.

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

We have to reset the input on Android whenever we're changing the input dynamically ie changing the text without any input keys being pressed. In order to do that, we need to call the restartInput method that Android exposes on the InputMethodManager class.

This method is not exposed to the JS side of React native so we need to create a library that exposes this method on the JS side. A library which is doing something similar can be checked out here. However, it is outdated and it clears the text input as well which is why we should either do our own implementation of it or find another one which does the same job. Once the function is exposed, we can call it here:

TextInputReset.resetKeyboardInput(findNodeHandle(this.textInput));

This will also fix this issue as it has the same root cause.

To test the solution above, we can do the following:

  1. Run npm install react-native-text-input-reset
  2. In node_modules/react-native-text-input-reset/android/build.gradle, update compile on line 36 to implementation.
  3. Remove the following block in file node_modules/react-native-text-input-reset/android/src/main/java/com/nikolaiwarner/RNTextInputReset/RNTextInputResetModule.java:
try {
                    TextView textView = (TextView) viewToReset;
                    // textView.setText("");
                  } catch (Exception e) {}
  1. Run npm run android to start the app.

We can add the same function (the one which react-native-text-input-reset is exposing), into our app's Android src folder as well here. This will result in NPM package no longer being needed.

I however, prefer creating a new package for this so that others might be able to make use of it as well. I don't see any particular benefit in adding it directly into our native files. It might cause conflicts later on when we try to update RN version if the new version includes changes to the file structure.

Result

https://user-images.githubusercontent.com/30054992/227550911-fece30ad-04d3-4f42-abab-3018172714d0.mov

What alternative solutions did you explore? (Optional)

This can fix the issue as well but its more of a hack.

allroundexperts avatar Mar 24 '23 14:03 allroundexperts

Hope you had a good weekend @mollfpr I have updated the proposal after your input.

kaushiktd avatar Mar 27 '23 05:03 kaushiktd

Not overdue, proposals are currently being evaluated by @mollfpr

MariaHCD avatar Mar 27 '23 16:03 MariaHCD

I'm not making it for today; I'll review this in the morning. Thank you!

mollfpr avatar Mar 27 '23 16:03 mollfpr

@mollfpr Were you able to review the updates to the proposals here? :)

MariaHCD avatar Mar 30 '23 10:03 MariaHCD

@MariaHCD Sorry, I forgot about this, and thank you for the ping! I'll review this tonight.

mollfpr avatar Mar 30 '23 11:03 mollfpr

@kaushiktd Could you explain why we need to call this.setTextInputShouldClear(true); to clear the composer value? and why it's not happen in all Android device?

mollfpr avatar Mar 30 '23 12:03 mollfpr

@allroundexperts If this issue is expected on the RN project, I prefer we come up with solution to fix it on the upstream.

mollfpr avatar Mar 30 '23 12:03 mollfpr

@allroundexperts If this issue is expected on the RN project, I prefer we come up with solution to fix it on the upstream.

The issue is not on the RN project itself. It is an issue with the gboard. We need sort of an additional function on android that resets the keyboard once an emoji is selected.

allroundexperts avatar Mar 30 '23 12:03 allroundexperts

@allroundexperts Do you suggesting to use react-native-text-input-reset or else? As you said, the react-native-text-input-reset is not maintained well. It's not worth installing a new library to solve an issue not happening in most Android devices.

The recent proposal from @kaushiktd is fixing the issue, but I need more context about the solution.

mollfpr avatar Mar 30 '23 12:03 mollfpr

@allroundexperts Do you suggesting to use react-native-text-input-reset or else? As you said, the react-native-text-input-reset is not maintained well. It's not worth installing a new library to solve an issue not happening in most Android devices.

The recent proposal from @kaushiktd is fixing the issue, but I need more context about the solution.

My suggestion is to create a fork of it. It really doesn't do much and just exposes a keyboard reset native function on to the JS layer.

allroundexperts avatar Mar 30 '23 12:03 allroundexperts

As per the rule, I always make certain that the proposal I provide is unique, and I also considered the same solution as my fellow contributor allroundexperts at first place.

I worked on that solution first and discovered that it is not a valid solution for this problem because there is no problem with gboard. It causes problems with all keyboards, including gboard and the Samsung default keyboard.

Also, it is not user friendly because if anyone compiles or runs the "npm install" command, it will override the native java file from node modules, and every time user can not change the native java file.

kaushiktd avatar Mar 30 '23 12:03 kaushiktd

As per the rule, I always make certain that the proposal I provide is unique, and I also considered the same solution as my fellow contributor allroundexperts at first place.

I worked on that solution first and discovered that it is not a valid solution for this problem because there is no problem with gboard. It causes problems with all keyboards, including gboard and the Samsung default keyboard.

Also, it is not user friendly because if anyone compiles or runs the "npm install" command, it will override the native java file from node modules, and every time user can not change the native java file,.

I did not realise that I was posting a duplicate proposal. Can you please share a link where you proposed the same thing? Apologies if my proposal caused inconvenience. I'll be happy to revoke it in your favour after checking the link @kaushiktd

allroundexperts avatar Mar 30 '23 12:03 allroundexperts