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.44k stars 2.8k forks source link

[$500] Android - Split - Input shows the content briefly when pasting word in split amount field #41961

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 4 months 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: 1.4.72-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:

Precondition:

Expected Result:

The input will remain static when pasting text because it only supports numbers

Actual Result:

The input shows the content briefly when pasting word in split amount field

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/ec8162b6-65d2-4b8e-8bd2-7defcc75c048

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ec930cc674fea0dd
  • Upwork Job ID: 1789994615970213888
  • Last Price Increase: 2024-07-01
melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @greg-schroeder (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 4 months ago

@greg-schroeder 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 commented 4 months ago

We think that this bug might be related to #vip-split

melvin-bot[bot] commented 4 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01ec930cc674fea0dd

greg-schroeder commented 4 months ago

Sending to external

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

@rojiphil, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 months ago

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

rojiphil commented 4 months ago

Awaiting Proposals

greg-schroeder commented 4 months ago

Same as above, go away Melvin!

melvin-bot[bot] commented 4 months ago

@rojiphil, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 4 months ago

@rojiphil @greg-schroeder 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!

rojiphil commented 4 months ago

Posted in slack channel here

melvin-bot[bot] commented 4 months ago

📣 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] commented 4 months ago

@rojiphil, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

greg-schroeder commented 4 months ago

Hmm, still not a ton of movement on this one. Perhaps we can try another route ... let me check w/ the team

melvin-bot[bot] commented 4 months ago

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

greg-schroeder commented 4 months ago

Asked Matt to see if he thinks we could double this

melvin-bot[bot] commented 4 months ago

Upwork job price has been updated to $500

greg-schroeder commented 4 months ago

Doubled!

samilabud commented 4 months ago

I think this cannot be handled unless we develop a whole TextInput component (maybe called NumberInput) from scratch and not using the one that comes with React Native.

Please see this comment: https://stackoverflow.com/a/32956836/7543637

QichenZhu commented 4 months ago

Proposal

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

Amount input accepts and shows illegal characters briefly.

What is the root cause of that problem?

  1. react-native disables key filtering for text inputs in Android on purpose.

This is explained in the code:

We override the KeyListener so that all keys on the soft input keyboard as well as hardware keyboards work. Some KeyListeners like DigitsKeyListener will display the keyboard but not accept all input from it

and in this commit message:

Makes it so that when a keyboard is displayed, all keys in that keyboard actually can be set as text. Previously you could display a Numeric keyboard and it would only allow entering numbers despite the keyboard having other keys like comma, plus, space, etc.

(Related issues: https://github.com/facebook/react-native/issues/38155, https://github.com/facebook/react-native/issues/24585)

  1. After resolving the above issue, another problem will arise: Android uses the same value for TYPE_NUMBER_FLAG_DECIMAL and TYPE_TEXT_FLAG_CAP_WORDS flags, but react-native doesn't account for this and disables TYPE_TEXT_FLAG_CAP_WORDS by default, which also disables TYPE_NUMBER_FLAG_DECIMAL.

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

  1. Add a patch to the patches folder.
diff --git a/node_modules/react-native/Libraries/Components/TextInput/TextInput.js b/node_modules/react-native/Libraries/Components/TextInput/TextInput.js
index 481938f..87eed02 100644
--- a/node_modules/react-native/Libraries/Components/TextInput/TextInput.js
+++ b/node_modules/react-native/Libraries/Components/TextInput/TextInput.js
@@ -1475,7 +1475,7 @@ function InternalTextInput(props: Props): React.Node {
       />
     );
   } else if (Platform.OS === 'android') {
-    const autoCapitalize = props.autoCapitalize || 'sentences';
+    const autoCapitalize = props.autoCapitalize || (props.keyboardType === 'decimal-pad' ? 'words' : 'sentences');
     const _accessibilityLabelledBy =
       props?.['aria-labelledby'] ?? props?.accessibilityLabelledBy;
     const placeholder = props.placeholder ?? '';
diff --git a/node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java b/node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
index 081f2b8..f9240e2 100644
--- a/node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
+++ b/node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactEditText.java
@@ -558,7 +558,6 @@ public class ReactEditText extends AppCompatEditText {
     }

     mKeyListener.setInputType(type);
-    setKeyListener(mKeyListener);
   }

   public void setPlaceholder(@Nullable String placeholder) {
  1. Consider creating a PR to react-native.

What alternative solutions did you explore? (Optional)

N/A

greg-schroeder commented 4 months ago

@rojiphil what do you think of this comment? https://github.com/Expensify/App/issues/41961#issuecomment-2148474894

melvin-bot[bot] commented 4 months ago

@rojiphil @greg-schroeder this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 3 months ago

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

greg-schroeder commented 3 months ago

Bump @rojiphil on this if you can take a look! https://github.com/Expensify/App/issues/41961#issuecomment-2153523917

rojiphil commented 3 months ago

Bump @rojiphil on this if you can take a look!

@greg-schroeder Sorry for the delay in response here.

rojiphil commented 3 months ago

@QichenZhu Thanks for the proposal

react-native disables key filtering for text inputs in Android on purpose.

Since we are talking about clipboard paste here, would enabling key filtering work when we paste via clipboard?

We can patch react-native locally by removing this line

Can you please share the details of how the patch will look like? Also, please share a test video of how it will look once we apply the patch.

QichenZhu commented 3 months ago

Proposal

Updated

QichenZhu commented 3 months ago

@rojiphil Thank you for considering my proposal!

Since we are talking about clipboard paste here, would enabling key filtering work when we paste via clipboard?

Yes, it works when pasting.

Can you please share the details of how the patch will look like? Also, please share a test video of how it will look once we apply the patch.

Please find my updated proposal and the below recording.

https://github.com/Expensify/App/assets/57348009/68c9e920-905d-4dfd-8717-69a47dcfef91

melvin-bot[bot] commented 3 months ago

@rojiphil, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

greg-schroeder commented 3 months ago

Proposal review continues

melvin-bot[bot] commented 3 months ago

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

greg-schroeder commented 3 months ago

Bump @rojiphil as the proposal was updated above

rojiphil commented 3 months ago

- setKeyListener(mKeyListener);

@QichenZhu Thanks for sharing the details of the patch here As you mentioned, this change has been intentionally introduced in react-native. In fact, it has been added a good 8+ years back. And I am not that sure if Meta would find our reasoning convincing enough to make the change. Also, wondering if this could introduce any regressions with respect to key inputs. Let me take some advice from internal members on this.

@vit I need expert help in the review here as the proposed changes are in react-native TextInput. Can you please help unblock?

melvin-bot[bot] commented 3 months ago

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

QichenZhu commented 3 months ago

Thanks for the comment. The problem with react-native is that it changed the default behavior of Android text inputs, causing our issue and several upstream issues (https://github.com/facebook/react-native/issues/38155, https://github.com/facebook/react-native/issues/24585). So I propose reverting to the default behavior or, maybe even better, providing an option to enable/disable it.

melvin-bot[bot] commented 3 months ago

@rojiphil, @greg-schroeder Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

greg-schroeder commented 3 months ago

Discussion is ongoing

greg-schroeder commented 3 months ago

What do you think of the above @rojiphil?

melvin-bot[bot] commented 3 months ago

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

greg-schroeder commented 3 months ago

Bump!

rojiphil commented 3 months ago

Looks like the fix may require upstream changes to react-native code and I am still on the fence about this.

Also, comment here and follow-up comment here need to be reviewed. Overall, I think providing an option to enable/disable key filtering seem to be a good way forward. But I also think an expert feedback from someone who has handled upstream changes to react-native text input would come in handy here.

Let me involve an internal engineer to weigh in first and take this from there. 🎀👀🎀

melvin-bot[bot] commented 3 months ago

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

rlinoz commented 3 months ago

This is a pretty minor problem that I honestly think is not worth the work of handling an upstream PR, I am assigned to another issue that needs an upstream PR and almost a year has passed since then.

I think we should close this one and revisit it down the road.

cc: @greg-schroeder