App icon indicating copy to clipboard operation
App copied to clipboard

[$500] Android - Taxes - Cursor is placed before the value when reopening saved value

Open lanitochka17 opened this issue 1 year ago • 32 comments

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


Version Number: 1.4.57-2 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

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

Action Performed:

Precondition:

  • User is admin of Collect workspace
  1. Go to staging.new.expensify.com
  2. Go to Profile > Workspaces > Collect workspace
  3. Go to Taxes > Add rate > Value
  4. Enter any value and save it
  5. Tap Value again

Expected Result:

Cursor is placed behind the value

Actual Result:

Cursor is placed in front of the value. Sometimes it appears behind the value

Workaround:

Unknown

Platforms:

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

  • [x] Android: Native
  • [ ] Android: mWeb Chrome
  • [ ] iOS: Native
  • [ ] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/92f43526-549a-43cf-8d7b-de7644b2ab7f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01515a93e0ea4bae26
  • Upwork Job ID: 1774175994999578624
  • Last Price Increase: 2024-04-13

lanitochka17 avatar Mar 27 '24 16:03 lanitochka17

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

melvin-bot[bot] avatar Mar 27 '24 16:03 melvin-bot[bot]

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

lanitochka17 avatar Mar 27 '24 16:03 lanitochka17

We think that this bug might be related to #wave-control

lanitochka17 avatar Mar 27 '24 16:03 lanitochka17

Proposal

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

Android - Taxes - Cursor is placed before the value when reopening saved value

What is the root cause of that problem?

We are not updating the selection correctly and due to that sometimes the selection is set to 0,0. We should set selection exactly like we do in MoneyRequestAmountForm. https://github.com/Expensify/App/blob/c7d692662d069ef8d9c756833767c47e1f7410a2/src/components/AmountForm.tsx#L220-L225 https://github.com/Expensify/App/blob/c7d692662d069ef8d9c756833767c47e1f7410a2/src/pages/iou/steps/MoneyRequestAmountForm.tsx#L323-L331

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

We need to calculate the start and end of the selection instead of using e.nativeEvent.selection. Or we can refrain from updating selection on initial render. For that we can use a ref or state.

onSelectionChange={(e: NativeSyntheticEvent<TextInputSelectionChangeEventData>) => {
    if (!shouldUpdateSelection) {
        return;
    }
    const maxSelection = formattedAmount.length;
    const start = Math.min(e.nativeEvent.selection.start, maxSelection);
    const end = Math.min(e.nativeEvent.selection.end, maxSelection);
    setSelection({start, end});
}}

What alternative solutions did you explore? (Optional)

We don't clear the amount when going back to Add rate page, we should also reset the value when the component AmountForm component mounts.

We need to add changes here: https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/components/AmountPicker/AmountSelectorModal.tsx#L19

To:

    useEffect(() => {
        setValue(value);
    }, [isVisible, value]);

Or we can update onBackButtonPress here like: https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/components/AmountPicker/AmountSelectorModal.tsx#L37

onBackButtonPress={() => {
    onClose();
    setValue(value);
}}

What alternative solutions did you explore? (Optional) 2

We should focus on the input using inputCallbackRef from the useAutoFocusInput hook. We need to utilize the ref passed from AmountForm to AmountSelectorModal and then apply inputCallbackRef.

Or we can follow the code we use in IOURequestStepAmount: https://github.com/Expensify/App/blob/e542ba7c2d9e216c2cd401de0de8ed225fff4a7b/src/pages/iou/request/step/IOURequestStepAmount.js#L87-L97

If we want to fix this issue comprehensively, we can check for the autoFocus prop and replace it with the implementation mentioned above. Additionally, we can cease accepting the autoFocus prop altogether to prevent encountering this issue again in the future.

pseudo-code:


    const textInput = useRef<BaseTextInputRef | null>(null);
    const focusTimeoutRef = useRef<NodeJS.Timeout | null>(null);

    useFocusEffect(
        useCallback(() => {
            if (!isVisible) {
                return;
            }
            focusTimeoutRef.current = setTimeout(() => textInput.current && textInput.current.focus(), CONST.ANIMATED_TRANSITION);
            return () => {
                if (!focusTimeoutRef.current) {
                    return;
                }
                clearTimeout(focusTimeoutRef.current);
            };
        }, [isVisible]),
    );

Krishna2323 avatar Mar 27 '24 22:03 Krishna2323

Job added to Upwork: https://www.upwork.com/jobs/~01515a93e0ea4bae26

melvin-bot[bot] avatar Mar 30 '24 20:03 melvin-bot[bot]

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

melvin-bot[bot] avatar Mar 30 '24 20:03 melvin-bot[bot]

Proposal Updated

  • Added optional

Krishna2323 avatar Mar 30 '24 23:03 Krishna2323

Not reproducible on main in my case:

https://github.com/Expensify/App/assets/59809993/d3da0e06-f5a5-4a53-9311-712ca89c26aa

abzokhattab avatar Mar 30 '24 23:03 abzokhattab

Proposal Updated

  • Update optional

Krishna2323 avatar Mar 30 '24 23:03 Krishna2323

Proposal Updated

  • Updated optional

Krishna2323 avatar Mar 31 '24 02:03 Krishna2323

Proposals ready for you @cubuspl42

MitchExpensify avatar Apr 01 '24 18:04 MitchExpensify

Sorry if this is obvious, but how can I enter the "Taxes" screen?

This is a Collect workspace, I'm the admin.

image

jakub-trzebiatowski avatar Apr 02 '24 12:04 jakub-trzebiatowski

@cubuspl42 below Profile and Members you should also see More Features. Enable Taxes under More Features. My collect workspace was created on old dot. I don't know if that makes a difference.

kmbcook avatar Apr 02 '24 20:04 kmbcook

image

image

jakub-trzebiatowski avatar Apr 03 '24 11:04 jakub-trzebiatowski

@cubuspl42, I guess you are not the admin of the workspace.

Krishna2323 avatar Apr 03 '24 11:04 Krishna2323

I thought "owner" and "admin" are the same. You can see that I'm the owner on the screenshots.

jakub-trzebiatowski avatar Apr 03 '24 11:04 jakub-trzebiatowski

It should have all these options. image

Krishna2323 avatar Apr 03 '24 11:04 Krishna2323

I found a solution; see this Slack message. I'll see if I can reproduce this issue now.

jakub-trzebiatowski avatar Apr 03 '24 15:04 jakub-trzebiatowski

📣 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] avatar Apr 06 '24 16:04 melvin-bot[bot]

@lanitochka17 @MitchExpensify I can't reproduce, same as in this comment.. I tested on latest main.

jakub-trzebiatowski avatar Apr 08 '24 09:04 jakub-trzebiatowski

@cubuspl42, I can reproduce on physical device when opening amount page multiple times and sometimes I had to only visit once or twice. Also, what do you think about the other bug? The amount is not cleared even if it is not saved.

https://github.com/Expensify/App/assets/85894871/a9b7734a-fcb7-4ca1-a8b7-b1203f0a5866

cc: @MitchExpensify

Krishna2323 avatar Apr 08 '24 11:04 Krishna2323

@Krishna2323 Actually, before we resume discussing your proposal, could you stick to the proposal template and fix the markdown of your proposal? Currently, I can see something like this:

image

First, "Result" and "Optional" aren't valid H3 headers from the proposal template, second, "Result" doesn't seem to render. Possibly, the same information could be provided in a H4 header, or in a separate GH comment.

jakub-trzebiatowski avatar Apr 08 '24 12:04 jakub-trzebiatowski

@cubuspl42, thanks for the feedback, proposal template updated.

Krishna2323 avatar Apr 09 '24 05:04 Krishna2323

@cubuspl42 More difficult to reproduce it now, but it exists. Need to go to the page many times now.

https://github.com/Expensify/App/assets/78819774/1ac0914e-469e-4acc-addd-88431d5ded3f

lanitochka17 avatar Apr 09 '24 16:04 lanitochka17

Waiting on @cubuspl42 to check out the latest updated proposal

MitchExpensify avatar Apr 09 '24 21:04 MitchExpensify

I'll run the app on my old physical Galaxy S8 to see if I can reproduce. It will help during testing, but as a plan B we could go forward without me reproducing, as we have double confirmation that the bug still occurs.

jakub-trzebiatowski avatar Apr 10 '24 09:04 jakub-trzebiatowski

@cubuspl42 @MitchExpensify 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] avatar Apr 10 '24 18:04 melvin-bot[bot]

There are some building issues on Android, but I'll try working it around.

jakub-trzebiatowski avatar Apr 11 '24 07:04 jakub-trzebiatowski

This fix for Android is now merged, I'm now building the app again

jakub-trzebiatowski avatar Apr 12 '24 08:04 jakub-trzebiatowski

Proposal Updated

  • Moved main solution to alternative 2
  • Added new main solution

Krishna2323 avatar Apr 12 '24 09:04 Krishna2323