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.46k stars 2.81k forks source link

Android - Invoice - In invoice details page, reopening amount field shows cursor at beginning #50055

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week 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: 9.0.43-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch app
  2. Log in with new expensifail account
  3. Tap fab -- new workspace - more features - enable invoice
  4. Navigate to LHN
  5. Tap fab - send invoice
  6. Enter an amount and tap next
  7. Select a user
  8. Tap next
  9. Enter company name and address
  10. Tap send invoice
  11. Open invoice - amount
  12. Reopen the amount field

Expected Result:

In invoice details page, reopening amount field must not show cursor at beginning

Actual Result:

In invoice details page, reopening amount field shows cursor at beginning

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/2521a545-b9bd-4969-b404-82170b95ef48

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @situchan
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 1 week ago

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

melvin-bot[bot] commented 1 week ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 1 week ago

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

nkdengineer commented 1 week ago

Proposal

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

In invoice details page, reopening amount field shows cursor at beginning

What is the root cause of that problem?

The weird bug happens by the old HOC withOnyx in IOURequestStepAmount. That causes the currency to flicker from USD to the current currency

https://github.com/user-attachments/assets/72b281ec-0320-4195-b49f-da77feecfada

https://github.com/Expensify/App/blob/efa3274237e9da1d4fdcea10db8f04458a3105e8/src/pages/iou/request/step/IOURequestStepAmount.tsx#L334

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

Remove withOnyx in IOURequestStepAmount and use useOnyx. The test branch here

https://github.com/Expensify/App/blob/efa3274237e9da1d4fdcea10db8f04458a3105e8/src/pages/iou/request/step/IOURequestStepAmount.tsx#L334

What alternative solutions did you explore? (Optional)

Result

https://github.com/user-attachments/assets/205fd6c0-b888-4e0d-86e8-aea7aec3e84d

situchan commented 6 days ago

@nkdengineer thanks for the proposal. The root cause is not clear. Please explain why this bug happens only on android. I tested your branch and bug is still reproducible.

nkdengineer commented 6 days ago

@situchan Can you share the video that you tested in my test branch.

nkdengineer commented 6 days ago

The root cause is not clear. Please explain why this bug happens only on android.

@situchan That is because the withOnyx is outdated and on Android, there are more delays on the other platform when we merge the Onyx data then which can affect the weird lifecycle on Android.

We can see the log here in IOURequestStepAmount, this component is mounted and unmounted two times when we open this page

Screenshot 2024-10-04 at 15 03 49

And here is the result when we use useOnyx

Screenshot 2024-10-04 at 15 03 15
QichenZhu commented 6 days ago

Proposal

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

Sometimes the caret in the currency amount input is initially at the beginning instead of the end.

What is the root cause of that problem?

The native component ReactEditText is not initially selectable. It only becomes selectable after it is attached to window.

https://github.com/facebook/react-native/blob/699f73a6b0734fe716b01d263e9579bca38c4652/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java#L1053C3-L1060

public void onAttachedToWindow() {
  super.setTextIsSelectable(true);

TextInput component sets the selection in the code below.

https://github.com/facebook/react-native/blob/699f73a6b0734fe716b01d263e9579bca38c4652/packages/react-native/Libraries/Components/TextInput/TextInput.js#L1104-L1132

useLayoutEffect(() => {
  viewCommands.setTextAndSelection(

The execution order of making it selectable and setting the selection is random. If making it selectable runs after setting the selection, the selection resets to the beginning.

To validate this, we can print logs before and after setTextIsSelectable().

FLog.w(TAG, "Before: selectable = " + isTextSelectable() + " selectionStart = " + getSelectionStart() + " selectionEnd = " + getSelectionEnd());
super.setTextIsSelectable(true);
FLog.w(TAG, "After: selectable = " + isTextSelectable() + " selectionStart = " + getSelectionStart() + " selectionEnd = " + getSelectionEnd());

We’ll notice when this bug occurs, the selection is at the right (Before: selectable = false selectionStart = 4 selectionEnd = 4) then resets to the left (After: selectable = true selectionStart = 0 selectionEnd = 0).

This is not always reproducible and happened in my tests only about 1 out of 10 times.

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

We could add a patch to node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java to save the selection before calling setTextIsSelectable() and restore it afterward.

int prevSelectionStart = getSelectionStart();
int prevSelectionEnd = getSelectionEnd();
super.setTextIsSelectable(true);
setSelection(prevSelectionStart, prevSelectionEnd);

What alternative solutions did you explore? (Optional)

N/A

melvin-bot[bot] commented 2 days ago

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

situchan commented 2 days ago

@nkdengineer here's video:

https://github.com/user-attachments/assets/7551ceaf-f19e-41ea-8f89-5c96fc5de81b

@QichenZhu if it's RN bug, it should be fixed in upstream. Can you please create issue in RN?

QichenZhu commented 2 days ago

Can you please create issue in RN?

@situchan Sure, I'll try to make a reproducer as required by RN first.

BTW, I don’t expect them to process it quickly. For reference, the contributor from issue https://github.com/Expensify/App/issues/21219 created a PR for them over a year ago, and it's still under review.

QichenZhu commented 2 days ago

@situchan Here's a minimal reproduction:

import { TextInput } from 'react-native';
import { useEffect, useRef} from 'react';

export default function App() {
  const input = useRef(null);
  useEffect(() => { setTimeout(() => input.current?.focus(), 1000); }, []);
  return <TextInput ref={input} value='1.00' selection={{start: 4, end: 4}} />;
}

Reproduction Steps

  1. Clone the reproducer https://github.com/QichenZhu/reproducer-react-native-textinput.git

  2. Build the Android app with New Arch enabled.

cd ReproducerApp
yarn
yarn android
  1. Start Metro.
npx react-native start
  1. Launch the app.

  2. Wait a second.

Expected result: The caret is at the end.

Actual result: The caret is at the beginning.

https://github.com/user-attachments/assets/5f3094ec-f253-45ac-8438-0d0d38db5268

situchan commented 1 day ago

@QichenZhu thanks. Let's try to fix upstream first. If it takes too long, we can consider patch.

QichenZhu commented 58 minutes ago

The upstream issue https://github.com/facebook/react-native/issues/46943 is resolved and the PR https://github.com/facebook/react-native/pull/46948 is merged. It may take one or two months for the changes to be included in a release.