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

Fix excessive toggles on the Switch component #26496

Closed
wants to merge 3 commits into from

Conversation

rurikoaraki
Copy link
Contributor

@rurikoaraki rurikoaraki commented Sep 18, 2019

Summary

In Windows, if you clicked on a Switch component to toggle it, you could see it "shimmy" back and forth once before settling. The native Switch ends up toggling three times every time it's invoked.

Switch.js prevents the native switch from toggling to the new value by explicitly setting the switch to this.props.value when it receives an onChange event. The re-setting of the value wasn't fast enough to prevent the Switch from starting to toggle, causing the visual shimmy.

The solution is taken from TextInput. TextInput.js stores _lastNativeText when it receives an onChange event. In componentDidUpdate, it puts this.props.text back on the native textbox if the value of this.props.text isn't the same as _lastNativeText, which is how it ensures that it is a controlled component. Porting this to the Switch results in only one toggle happening per invoke, removing the shimmy, while preserving the controlled component behavior.

This bug is not visible on Android or iOS, only Windows, however the code causing the bug was in Switch.js and it seems reasonable to avoid changing the value of the native switch excessively.

Changelog

[General] [Fixed] - Fix excessive toggles on the Switch component

Test Plan

Used RNTester on Android and iOS to test the Switch component and made sure that all scenarios behave as expected visually. Also ensured through the debugger that the value of the native switch is only being changed once, instead of three times.

Ruriko Araki
Microsoft Corp.

…dealing with JS-side settings to ensure that native switch isn't set to new values unnecessarily.
@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@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 Sep 18, 2019
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@JoshuaGross
Copy link
Contributor

This seems reasonable to me. I'll import to test internally.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@JoshuaGross
Copy link
Contributor

Thanks for fixing this @rurikoaraki!

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @rurikoaraki in b782934.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 19, 2019
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: Switch Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants