Fix Picker.onValueChange on Android sometimes not fired due to race condition #22821
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
Thanks for this PR and the extensive test plan. Fingers crossed that it is gonna cover all cases and won't cause weird issues but your code looks solid :) |
facebook-github-bot
added
the
Import Started
facebook-github-bot
approved these changes
Jan 18, 2019
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
hramos
removed
Import Started
matt-oakes
pushed a commit
to matt-oakes/react-native
that referenced
this pull request
Feb 7, 2019
…ondition (facebook#22821) Summary: Changelog: ---------- [Android] [Fixed] - Fix Picker.onValueChange sometimes not fired due to race condition. Fixes facebook#15556 Root Cause: ------------ Please check my code snippet at https://snack.expo.io/kudochien/android-picker-issue and try to select different items on Picker to see if console.log() hit. If calling setState() with some latency, e.g. setTimeout() or changes from redux, the second time changing picker item on UI, the onValueChange() will be not fired. The root cause comes from the `forceUpdate` in PickerAndroid.android.js. If user's setState() update comes after forceUpdate(), the flow will be: 1. First time select picker item 2. onValueChange + forceUpdate 3. user's setState() + componentDidUpdate + setNativeProps({ selected: ... }) 4. mSuppressNextEvent = true 5. Second time select picker item 6. Since mSuppressNextEvent is true, the onValueChange will not be fired. Solution: --------- Like other controlled components, disable change listener during setup values from JS. Android Spinner `setSelection(int position)` is asynchronous call, i.e. will fire onItemSelected in next run loop and is not suitable for us. `setSelection(int position, boolean animate)`, however, is synchronous call which I used. Some more references about setSelection: https://stackoverflow.com/a/43512925/2590265 http://androidxref.com/8.1.0_r33/xref/frameworks/base/core/java/android/widget/AbsSpinner.java#276 The two arguments version will use `setSelectionInt()` which set mBlockLayoutRequests as true to prevent onItemSelected call from next layout(). I also moved the setOnItemSelectedListener() call after onLayout to prevent onValueChange() during intialization. Pull Request resolved: facebook#22821 Differential Revision: D13731979 Pulled By: cpojer fbshipit-source-id: e06bd9aa62463b66c8f3fd7214485898d5375054
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog:
[Android] [Fixed] - Fix Picker.onValueChange sometimes not fired due to race condition. Fixes #15556
Root Cause:
Please check my code snippet at https://snack.expo.io/@kudochien/android-picker-issue
and try to select different items on Picker to see if console.log() hit.
If calling setState() with some latency, e.g. setTimeout() or changes from redux,
the second time changing picker item on UI, the onValueChange() will be not fired.
The root cause comes from the
forceUpdatein PickerAndroid.android.js.If user's setState() update comes after forceUpdate(), the flow will be:
Solution:
Like other controlled components, disable change listener during setup values from JS.
Android Spinner
setSelection(int position)is asynchronous call, i.e. will fire onItemSelected in next run loop and is not suitable for us.setSelection(int position, boolean animate), however, is synchronous call which I used.Some more references about setSelection:
https://stackoverflow.com/a/43512925/2590265
http://androidxref.com/8.1.0_r33/xref/frameworks/base/core/java/android/widget/AbsSpinner.java#276
The two arguments version will use
setSelectionInt()which set mBlockLayoutRequests as true to prevent onItemSelected call from next layout().I also moved the setOnItemSelectedListener() call after onLayout to prevent onValueChange() during intialization.
Test Plan: