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

Password box bug #4607

Merged
2 commits merged into from
Apr 15, 2020
Merged

Password box bug #4607

2 commits merged into from
Apr 15, 2020

Conversation

kmelmon
Copy link
Contributor

@kmelmon kmelmon commented Apr 15, 2020

This fixes a bug in TextInput. If you don't specify a height and also set secureTextEntry = true, the PasswordBox ends up with a 0 height.

The bug was caused by some missing logic in the code that swaps the TextBox out for a PasswordBox. This code replaces the backing XAML element, but forgot to notify the UIManager which maintains a mapping from ShadowNode => XAML element. Thus, after the swap, bad stuff happens in layout, causing us to push a 0 height into the PasswordBox.

The fix is straightforward - after swapping the control, notify the UIManager so it can update its mapping to the new XAML element.

Also changed our existing Playground test page to not specify a height, this exposes the issue.

Microsoft Reviewers: Open in CodeFlow

@kmelmon kmelmon requested a review from a team as a code owner April 15, 2020 05:42
@kmelmon
Copy link
Contributor Author

kmelmon commented Apr 15, 2020

Fixes #4371

@kmelmon kmelmon added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 15, 2020
@ghost
Copy link

ghost commented Apr 15, 2020

Hello @kmelmon!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@asklar
Copy link
Member

asklar commented Apr 15, 2020

thanks for fixing this! I think this is an issue that the e2eTest is hitting (and failing) so it will be interesting to see if this fixes it

@ghost ghost merged commit 5ec607d into microsoft:master Apr 15, 2020
@namrog84
Copy link
Contributor

Can we get this fix cherry picked onto 61?

kmelmon added a commit to kmelmon/react-native-windows that referenced this pull request Apr 15, 2020
* bug fix, update test

* Change files
kmelmon added a commit that referenced this pull request Apr 15, 2020
* bug fix, update test

* Change files
asklar pushed a commit to asklar/react-native-windows that referenced this pull request Apr 19, 2020
* bug fix, update test

* Change files
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants