[$500] [POLISH][Search v2.1] Amount filter - Native - The digit exceeding the max allowed decimal places is briefly inserted into the input

[rayane-djouah]

This is a follow-up issue.
Coming from: #47343 (comment) and #47343 (comment)

Bug in iOS and Android native apps: When entering a number, the digit exceeding the maximum allowed decimal places is briefly inserted into the input field before being deleted. They should not be inserted at all.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-15.at.15.52.28.mp4

cc @luacmartins @289Adam289

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~017b65a6e5c7b77ba2
• Upwork Job ID: 1827006416140957560
• Last Price Increase: 2024-12-18
• Automatic offers:
• brunovjk | Reviewer | 104524279

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @brunovjk

[melvin-bot]

Triggered auto assignment to @isabelastisser (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.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~017b65a6e5c7b77ba2

[melvin-bot]

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

[eyobai]

Validation Function: Create a function to check if the input exceeds the allowed decimal places.
Event Listener: Use the input event to validate each change to the input field.
Immediate Correction: If the input is invalid, immediately revert to the last valid state.

[melvin-bot]

📣 @eyobai! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[huult]

Proposal

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

The digit exceeding the max allowed decimal places is briefly inserted into the input

What is the root cause of that problem?

This issue occurs because we use onChangeText to validate the amount between lines 37 and line 40. If the amount is not valid, we return and remove the text change in the text input

App/src/components/AmountWithoutCurrencyForm.tsx

Lines 29 to 43 in 0c84552

	const setNewAmount = useCallback(
	(newAmount: string) => {
	// Remove spaces from the newAmount value because Safari on iOS adds spaces when pasting a copied value
	// More info: https://github.com/Expensify/App/issues/16974
	const newAmountWithoutSpaces = stripSpacesFromAmount(newAmount);
	const replacedCommasAmount = replaceCommasWithPeriod(newAmountWithoutSpaces);
	const withLeadingZero = addLeadingZero(replacedCommasAmount);
	if (!validateAmount(withLeadingZero, 2)) {
	return;
	}
	onInputChange?.(withLeadingZero);
	},
	[onInputChange],
	);

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

We should check that the decimal limit has been reached and no more decimals can be entered before the text changes.

// src/components/AmountWithoutCurrencyForm.tsx#L17
+ const decimals = 2;

// src/components/AmountWithoutCurrencyForm.tsx#L37
- if (!validateAmount(withLeadingZero, 2)) {
+ if (!validateAmount(withLeadingZero, decimals)) {

// src/components/AmountWithoutCurrencyForm.tsx#L45
+     const getDecimalLength = (numStr: string) => {
+        const parts = numStr.split('.');
+        return parts[1] ? parts[1].length : 0;
+    };

// src/components/AmountWithoutCurrencyForm.tsx#L46
    return (
        <TextInput
            value={formattedAmount}
            onChangeText={setNewAmount}
            inputID={inputID}
            name={name}
            label={label}
            defaultValue={defaultValue}
            accessibilityLabel={accessibilityLabel}
            role={role}
            ref={ref}
            keyboardType={CONST.KEYBOARD_TYPE.DECIMAL_PAD}
+           maxLength={getDecimalLength(formattedAmount) === decimals ? formattedAmount.length : undefined}
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...rest}
        />
    );

POC

Screen.Recording.2024-08-25.at.01.13.48.mov

[brunovjk]

Thank you for the proposal @huult, but I tested it and it doesn't seem to resolve the issue. The RCA focuses on maxLength, but the problem occurs only on native platforms. If you believe your solution fixes this, please share a test branch for review.
For context, I just reproduced the issue on web and native. On web, typing 1111111111111111111111.111111111111 shows 11111111.11 without issue. However, on native (I only tested on iOS so far), the number stays as 1111111111111111111111.111111111111 for a couple of seconds before correcting to 11111111.11. The issue may require further investigation into why native platforms are behaving differently.

[huult]

@brunovjk , Thanks for reviewing my proposal. I double-checked and found that it works when the user types from the keyboard but does not work when pasting a number into the input field or entering multiple decimal points. I am investigating further.

[AlfredoAlc]

This most likely is related to a known issue for the TextInput component from react-native, specially for iOS platforms. See this issue for reference: facebook/react-native#36494

[melvin-bot]

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

[brunovjk]

Thank you, @AlfredoAlc, for the reference. It aligns with our issue, but I haven’t found a fix yet, only workarounds.
@huult, your proposal appears reasonable as it directly enforces decimal limits. However, despite the straightforward nature of the suggested changes, they haven’t resolved the issue in my tests. Do you have any new findings or a test branch to share? Thank you all!

[huult]

@brunovjk , test branch for my proposal

POC

Screen.Recording.2024-08-29.at.09.27.32.mp4

[brunovjk]

@brunovjk , test branch for my proposal

Thank you @huult :D I just tested. Almost there, but the problem is not just in the decimals:

Screen.Recording.2024-08-29.at.10.36.14.mov

If the video above is no longer available when you watch it, let me know.

[huult]

@brunovjk , I've updated the test branch to fix the issue you tested.

Screen.Recording.2024-08-29.at.21.24.51.mp4

[brunovjk]

Thank you for your efforts, @huult. Unfortunately, the solution still isn’t working as expected on my end. As we can't attack the root cause directly, we might need a "workaround" to address this effectively. I’m confident we can find one that won’t introduce regressions. Let’s keep exploring options—I’ll revisit this next week, and we can involve an internal engineer if necessary.

Screen.Recording.2024-08-29.at.16.57.22.mov

[melvin-bot]

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

[melvin-bot]

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

[brunovjk]

I can still reproduce this issue:

Screen.Recording.2024-12-02.at.09.39.46.mov

Looking for proposals/solutions.

[melvin-bot]

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

[brunovjk]

Same as above

[kirillzyusko]

@brunovjk I created a PoC here: #53695

Feel free to play with that changes and if you have any questions I'll be happy to answer them 😊

It's worth to note, that I didn't integrate a new type of input (masked input) into existing codebase. I simply showed how masked input can work - so styling of input is broken and most likely form submitting mechanism is broken as well, it's just a PoC to show, that masked input will not allow you to enter more symbols if mask template has been filled properly.

If we decided to go with this approach (and I really hope we will do that) then I'll properly integrate the new input into existing codebase so that we can use it preserving all styles and all generic components that we already use 🤞

[brunovjk]

Great @kirillzyusko! I'll review it calmly on Monday 🤞 and let you know, but in any case we eventually need a proposal, following the template, to present to the internal team, only they, after a proposal is approved, can assign you for the task. But I'm already very grateful for your effort.

[kirillzyusko]

Great @kirillzyusko! I'll review it calmly on Monday 🤞 and let you know, but in any case we eventually need a proposal, following the template, to present to the internal team, only they, after a proposal is approved, can assign you for the task. But I'm already very grateful for your effort.

Thanks, appreciate your help here! Then, please, test, and if everything is alright I'll prepare a proposal following template 👀

[brunovjk]

Testing today

[brunovjk]

Thanks for providing the POC, @kirillzyusko. I tested the implementation, but unfortunately, it doesn't seem to work as expected on my side. I don’t think I missed anything, but there’s a chance something went wrong during my setup. Could you share how your testing went? Thank you!

Screen.Recording.2024-12-09.at.17.54.41.mov

[kirillzyusko]

@brunovjk try to set a dark theme and you will see the white input below (in the light theme the white input is barely visible).

You are testing old implementation. On this page I re-worked only one input (the second one) and kept first one to take it as a reference in terms of functionality/formatting rules.

Would you mind to do testing again? I'll share a video tomorrow 🙃

[brunovjk]

Thanks for clarifying, testing in a moment

[brunovjk]

Very good! It seems promising to me:

test Poc

Screen.Recording.2024-12-11.at.09.11.39.mov

I can still reproduce some bugs:

• Maximum number of integers conflicting with decimals: I type 11 characters and can no longer type the comma or decimals. Adjusting to [12345678].[90] would allow up to 8 integers and enable decimals without deleting numbers.
• , and . should work the same way.
• Copy and Paste should work normally.

@kirillzyusko do you believe these bugs were expected and we will fix them later? I'm here if you need anything. Again, thank you very much for your effort.

[kirillzyusko]

do you believe these bugs were expected and we will fix them later?

Well, I believe I can fix some of them straight away (such as , and . separator or mask adjustments) and pasting probably would require a little bit more time but also should be fixable 🤞

My main concern is that we would need to add a new library... Does it make sense to open an issue about adding a new library and see whether Expensify team is happy to have it in package.json or not?

[brunovjk]

Great question @kirillzyusko, I'm going to ask you a favor, write a proposal with all the information you think is important, along with these questions, we will forward it to an internal member of the Expensify team, they knew exactly what to do next. How does it sound?

[melvin-bot]

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

[kirillzyusko]

Proposal

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

The digit exceeding the max allowed decimal places is briefly typed into the input

What is the root cause of that problem?

The problem in current implementation is that the communication between JS/Native thread is asynchronous. So when we type a value we immediately apply update on a native thread and show a newly typed digit. But formatting happens on JS, thus we always have a delay of 16ms and we can observer how digits are flickering.

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

We should use a solution, where formatting happens synchronously on a native thread.

In react-native this problem is solved long time ago using react-native-text-input-mask. This library relies on native libraries developed by RedMadRobot for android and ios (react-native-text-input-mask acts as a simple wrapper).

Note

The functionality with formatting dates/phone numbers/currency/card numbers and other inputs is a common task for native developers too. The input-mask package solves this problem in a really graceful way (by introducing mask concept).

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

It can be hard to automate this thing, just because only humans can notice the flickering.

What alternative solutions did you explore? (Optional)

react-native-text-input-mask can not be integrated into Expensify project just because it's abandoned project and it wasn't updated for a quite long time (this lib doesn't support Fabric architecture yet).

As an alternative we can use a fork called react-native-advanced-input-mask. Advantages of this fork:

• constantly gets updates;
• support both fabric and paper architectures;
• new features are in development (for example combining mask-placeholder with the text that you typed).

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@brunovjk please, review this proposal and in case if anything is missing - let me know 😊

[brunovjk]

I asked on Slack and luacmartins will look into it when they have a moment.

[kirillzyusko]

@brunovjk please, keep me updated here as well 🙌 It looks like I don't have an access to this channel/conversation 😔

[brunovjk]

Of course, we are still waiting for @luacmartins's thoughts.

[luacmartins]

So it seems like the suggestion is to go with the alternate solution, given the main library is abandoned and doesn't support fabric. I'll raise the library request in Slack and we'll see what the team decides

[luacmartins]

Created an issue for the lib request here and started a discussion in Slack

[melvin-bot]

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

[isabelastisser]

FYI, I will be OOO from Dec 23 to Jan 6, so please reassign the issue to another team member for urgency, IF needed.