[HOLD for payment 2024-11-29] [HOLD RN Upgrade] Android - Invoice - In invoice details page, reopening amount field shows cursor at beginning

[lanitochka17]

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?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6621403_1727807381706.Screenrecorder-2024-10-01-23-50-53-99_compress_1.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @sakluger

[melvin-bot]

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]

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

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

[melvin-bot]

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

[nkdengineer]

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

Screen.Recording.2024-10-03.at.15.10.49.mov

App/src/pages/iou/request/step/IOURequestStepAmount.tsx

Line 334 in efa3274

const IOURequestStepAmountWithOnyx = withOnyx<IOURequestStepAmountProps, IOURequestStepAmountOnyxProps>({

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

App/src/pages/iou/request/step/IOURequestStepAmount.tsx

Line 334 in efa3274

const IOURequestStepAmountWithOnyx = withOnyx<IOURequestStepAmountProps, IOURequestStepAmountOnyxProps>({

What alternative solutions did you explore? (Optional)

Result

Screen.Recording.2024-10-03.at.15.07.32.mov

[situchan]

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

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

[nkdengineer]

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

And here is the result when we use useOnyx

[QichenZhu]

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]

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

[situchan]

@nkdengineer here's video:

bug.mp4

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

[QichenZhu]

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 #21219 created a PR for them over a year ago, and it's still under review.

[QichenZhu]

@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

3. Start Metro.

npx react-native start

4. Launch the app.

5. Wait a second.

Expected result: The caret is at the end.

Actual result: The caret is at the beginning.

23_1728368555.mp4

[situchan]

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

[QichenZhu]

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

[sakluger]

@situchan what do you think - should we move to monthly and wait for the upstream PR to be released? That's what I think we should do, since this bug isn't a huge impact to customers.

[situchan]

Let's assign @QichenZhu since their upstream PR was merged.
And hold this for RN upgrade.
Proposal: #50055 (comment)
🎀👀🎀 C+ reviewed

[melvin-bot]

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

[sakluger]

@situchan do you still think we should continue holding for the upstream fix to be released, or should we try fixing immediately?

[QichenZhu]

@situchan mentioned he was on parental leave. Not sure if he’s back yet. @sakluger @aldo-expensify Could you respond to the above 2 comments?

[aldo-expensify]

@QichenZhu I understand that the fix you did here has not been released yet, correct?
Do we need to do a "open a pick request" for your fix?

[QichenZhu]

@aldo-expensify

I understand that the fix you did here has not been released yet, correct?

No, it hasn’t been released yet.

Do we need to do a "open a pick request" for your fix?

Yes, I think so if we want it included in 0.75 or 0.76. Otherwise, it's expected to be released in 0.77.

[aldo-expensify]

Do we need to do a "open a pick request" for your fix?

Yes, I think so if we want it included in 0.75 or 0.76, but I'm not sure how long they'll take to process it.

Requested here: reactwg/react-native-releases#622, maybe lets give it a week and then we reassess?

[ikevin127]

Coming from issue #52029, it was decided to move forward with creating a patch with the fixes already merged into react-native because of the lengthy timeline it would take E/App to upgrade to the future version of react-native where the fixes will be available. The patch will also fix this issue, therefore I added it to the PRs Fixed Issues section which triggered the automation.

[aldo-expensify]

I think we should close this issue... this #52029 is pretty a dupe of this one (same root cause). Is anyone missing compensation for this issue or can I just close it?

[ikevin127]

Compensation will be handled in #52029, so I'd say we can close this one.

[QichenZhu]

I think we should close this issue... this #52029 is pretty a dupe of this one (same root cause). Is anyone missing compensation for this issue or can I just close it?

@aldo-expensify @sakluger I’m not sure if I should be paid since I only submitted a PR to react-native, not to our repo. What do you think?

[sakluger]

@QichenZhu we can compensate you for your work on the upstream PR. Please let me know once you've accepted the below offer.

Summarizing payment on this issue:

Contributor: @QichenZhu $250, sent offer via Upwork: https://www.upwork.com/nx/wm/offer/104978993

[QichenZhu]

Thank you so much! Offer accepted. @sakluger

[sakluger]

Of course!

I've paid out the offer, so we're good to close this issue now.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• PATCH: Android - Fix caret resetting to the beginning of the input #52734

If no regressions arise, payment will be issued on 2024-11-29. 🎊

For reference, here are some details about the assignees on this issue:

• @QichenZhu requires payment (Needs manual offer from BZ)
• @situchan requires payment (Needs manual offer from BZ)

[melvin-bot]

@aldo-expensify @sakluger @QichenZhu / @situchan The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]