react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Fix bug when clearing text when securityTextEntry is true

Open adnan-khan1122 opened this issue 1 year ago • 10 comments

When a secureTextEntry is true and I try to edit it, it keeps getting cleared.

This PR solves this by patching function shouldChangeCharactersInRange adding a condition that checks isSecureTextEntry in textfield is true so overrides the behaviour of clearing the text, the change was done in the file RCTBackedTextInputDelegateAdapter.mm

This fixes #35668

Summary:

Motivation: When the secureTextEntry property is set to true on a TextInput component, users experience an issue where the text gets cleared each time they attempt to edit it. This behaviour is particularly problematic in cases where users need to modify their input, such as entering or correcting a password, leading to a frustrating user experience.

Resolution: To address this, a condition was added in the shouldChangeCharactersInRange function within the RCTBackedTextInputDelegateAdapter.mm file. This condition checks whether isSecureTextEntry is enabled on the UITextField. If it is, the function now overrides the default behaviour, preventing the text from being cleared when users attempt to edit it. This ensures a smoother and more intuitive experience for users when dealing with secure text inputs.

Changelog:

[iOS] [Fixed] - Resolved an issue where text would be cleared when editing a TextInput with secureTextEntry enabled. The shouldChangeCharactersInRange function was patched to prevent this behaviour.

Test Plan:

  • The first attached video demonstrates the issue where enabling secure entry causes the entered password to be cleared when attempting to retype it.
  • The second video shows that the issue has been resolved, allowing continued typing after enabling secure entry.

https://github.com/user-attachments/assets/a34f1606-c68b-4449-a0c3-8aeaf1b61f4f

https://github.com/user-attachments/assets/23e4d54f-0fe4-4e1c-83cf-b688a84faa5b

adnan-khan1122 avatar Aug 09 '24 21:08 adnan-khan1122

Our patchwork solution remains compatible with the new React Native version 0.75.1, as the underlying issue still persists.

adnan-khan1122 avatar Aug 15 '24 20:08 adnan-khan1122

We shouldn't deviate from the iOS behaviour by default. If we want to add this behaviour, I think we should enable it through a prop.

But even then, I don't think we want to add and maintain a new prop to deviate from the default iOS behaviour.

AntoineDoubovetzky avatar Sep 04 '24 23:09 AntoineDoubovetzky

We shouldn't deviate from the iOS behaviour by default. If we want to add this behaviour, I think we should enable it through a prop.

But even then, I don't think we want to add and maintain a new prop to deviate from the default iOS behaviour.

With all due respect, the default iOS behaviour is bad, (and that's why you might see several issues / threads on internet talking about this) and probably maintaining a version that is safer is the way to go, users of apps also shouldn't feel the difference between one mobile or the other in these type of details, sorry but your reasoning doesn't make much sense to me.

angvp avatar Sep 05 '24 03:09 angvp

@cortinico Hi there! Sorry to tag you unexpectedly, but since you added the "author needs feedback" tag on #46154, I was hoping you could help us get a review? We've been waiting for some time now, and your input would be greatly appreciated. Thanks so much!

angvp avatar Oct 03 '24 22:10 angvp

@angvp could you please rebase on top of main? I'd like to verify that everything builds in CI before importing this PR.

cipolleschi avatar Oct 04 '24 14:10 cipolleschi

@angvp could you please rebase on top of main? I'd like to verify that everything builds in CI before importing this PR.

For sure, I'm not the author but I'll ping him @adnan-khan1122. Thanks for reviewing it again!

angvp avatar Oct 04 '24 15:10 angvp

@cipolleschi Updated to top of main, thanks!

adnan-khan1122 avatar Oct 04 '24 16:10 adnan-khan1122

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Oct 09 '24 16:10 facebook-github-bot

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Oct 09 '24 16:10 facebook-github-bot

@cipolleschi First of all thank you for your effort!, sorry to bug you again but we would like to know when and if this will be merged? Thanks

angvp avatar Oct 22 '24 17:10 angvp

There have been some internal discussions about when to respect web behavior and when to respect native behavior. An iOS users might be puzzled by this behavior that align more to web, and moving on with this change will hint that the app has been developed with React Native instead of being a native application.

We have not reach consensus on what to do yet, so we have to pause this for a bit, while we come to a resolution.

cipolleschi avatar Oct 28 '24 14:10 cipolleschi

I don't think any user on iOS would be happy with the current behavior. Anything's better than the status quo, which is broken.

melyux avatar Oct 28 '24 19:10 melyux

There have been some internal discussions about when to respect web behavior and when to respect native behavior. An iOS users might be puzzled by this behavior that align more to web, and moving on with this change will hint that the app has been developed with React Native instead of being a native application.

We have not reach consensus on what to do yet, so we have to pause this for a bit, while we come to a resolution.

Makes sense, thanks for your response

angvp avatar Oct 29 '24 20:10 angvp

I don't think any user on iOS would be happy with the current behavior. Anything's better than the status quo, which is broken.

It's not broken, it is how the platform works. If you try and setup a project with Apple technologies (Objective-C or Swift, UIKit or SwiftUI) you'll see that this is how the platform behaves.

cipolleschi avatar Oct 31 '24 15:10 cipolleschi

We would not go forward with this. In the past we worked hardly to remove all those React Native behaviors that were tells that the app was built with React Native.

One of the goals of the framework is to appear as a much as Native as possible. Implementing this behavior would make every TextInput with a show/hide of a secret text behaving different than the platform and people would be able to tell that the app could have been built with React Native.

If this is an issue for your users, I suggest to create a custom component that implement this new behavior.

Thanks for the patience and the understanding.

cipolleschi avatar Nov 18 '24 11:11 cipolleschi