Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle update of secureTextEntry of TextInput in iOS #18587

Closed
wants to merge 1 commit into from
Closed

Handle update of secureTextEntry of TextInput in iOS #18587

wants to merge 1 commit into from

Conversation

lucasbento
Copy link

@lucasbento lucasbento commented Mar 27, 2018

This PR fixes #5859, which causes the TextInput to have a space appended to the end when you change the value of secureTextEntry prop in iOS:

Before After
before after

I found a few workarounds online and the simplest one in my opinion is this SO answer.

I'm definitely open to hear more opinions on the implementation.

Test Plan

My test plan is very basic, having a <TextInput /> and dynamically switching secureTextEntry prop between true/false will show the bug.

Release Notes

[IOS] [BUGFIX] [Libraries/Text/TextInput/RCTBaseTextInputView.m] - Fix bug with space being appended to the end of input value when changing secureTextEntry.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 27, 2018
@react-native-bot react-native-bot added 📋Release Notes Platform: iOS iOS applications. Component: TextInput Related to the TextInput component. labels Mar 27, 2018
@RWOverdijk
Copy link

Perhaps document that this is a "hack". For the rest, I'm just glad to see this working, but can't help but think that this should be fixed elsewhere.

</two_cents>

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a setter for secureTextEntry instead of using didSetProps? Something like:

- (void)setSecureTextEntry:(BOOL)secureTextEntry
{
  [super setSecureTextEntry:secureTextEntry];
  
  ...
}

Also I agree we should add a comment about what this does and a link to the SO post.

@lucasbento
Copy link
Author

Hey @janicduplessis, thank you for reviewing.

Where exactly should I put it? Do I have to add it and listen to the prop changes somewhere?

@lucasbento
Copy link
Author

@janicduplessis: can you take a look? :)

@facebook-github-bot

This comment has been minimized.

@lucasbento
Copy link
Author

@janicduplessis: would you mind taking a look? :)

@RWOverdijk

This comment has been minimized.

@RWOverdijk

This comment has been minimized.

@lucasbento
Copy link
Author

@shergin: perhaps you can take a look at this?

@RWOverdijk

This comment has been minimized.

@hramos
Copy link
Contributor

hramos commented Aug 1, 2018

By Facebook team, they were probably referring to me as I was busy with some other tasks around that time (April). Looking at this PR, I see @janicduplessis already reviewed it and provided some feedback that doesn't seem to be addressed yet.

@hramos hramos requested a review from shergin August 1, 2018 00:22
@lucasbento
Copy link
Author

@hramos: I'm waiting for a reply on this: #18587 (comment).

I just need to know where to put the code specified by @janicduplessis.

@justinsoong

This comment has been minimized.

@hramos
Copy link
Contributor

hramos commented Sep 26, 2018

Looks like the review feedback hasn't been addressed yet. I know there's a question out to Janic on how the feedback should be implemented, but that's not for me to answer.

I'll add that this PR would greatly benefit from adding a test case.

@ericlewis
Copy link
Contributor

@hramos I opened #23524 which addresses Janic's feedback.

@cpojer
Copy link
Contributor

cpojer commented Feb 22, 2019

Closing in favor of the new PR.

@cpojer cpojer closed this Feb 22, 2019
facebook-github-bot pushed a commit that referenced this pull request Feb 22, 2019
Summary:
This is a fix for #5859, based on the feedback in #18587. Instead of using `didSetProps` it uses a setter. I will also note that setting to `nil` no longer works (crashes) so setting it to a blank string then back to the original works fine.

[iOS] [Fixed] - Toggling secureTextEntry correctly places cursor.
Pull Request resolved: #23524

Differential Revision: D14143028

Pulled By: cpojer

fbshipit-source-id: 5f3203d56b1329eb7359465f8ab50eb4f4fa5507
grabbou pushed a commit that referenced this pull request Feb 27, 2019
Summary:
This is a fix for #5859, based on the feedback in #18587. Instead of using `didSetProps` it uses a setter. I will also note that setting to `nil` no longer works (crashes) so setting it to a blank string then back to the original works fine.

[iOS] [Fixed] - Toggling secureTextEntry correctly places cursor.
Pull Request resolved: #23524

Differential Revision: D14143028

Pulled By: cpojer

fbshipit-source-id: 5f3203d56b1329eb7359465f8ab50eb4f4fa5507
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TextInput Related to the TextInput component. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextInput] Dynamic changing of secureTextEntry breaks font and doesn't change cursor position
9 participants