Fix PickerAndroid will reset selected value during items update. #24793
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
Summary:
Two root causes:
1. Android Spinner will reset selection to undefined after setAdapter()
which will trigger onValueChange().
The behavior is not expected for RN.
And the solution is to setSelection() explicitly
2. In original implementation, it setups `items` immediately,
but delays the `selected` after update transaction.
There will be some race condition and incosistency
if update `items` only.
The fix will update both after transaction.
Fixes facebook#13351
facebook-github-bot
added
CLA Signed
react-native-bot
added
Component: Picker
Platform: Android
cpojer
approved these changes
May 10, 2019
facebook-github-bot
approved these changes
May 10, 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.
|
This pull request was successfully merged by @Kudo in 310cc38. When will my fix make it into a release? | Upcoming Releases |
|
@Kudo - Looks like this PR is causing an increase in ArrayOutOfBoundsException at getView. Do you have context on why this is happening ? I dont have context on this - do you think this is related ? |
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 20, 2019
…e from a long list (#25276) Summary: axe-fb reported this side effect from my previous commit in #24793 (comment) After revisited the implementation of Android Spinner, it seems the formal way to update existing adapter is mutating it, i.e. `arrayAdapter.clear()` & `arrayAdapter.addAll()` to update a Spinner Adapter. `setAdapter()` will reset everything including `mDataChanged`. A race condition may happens between rendering a long picker list and reseting adapter. Here is a code snippet: https://snack.expo.io/kudochien/80f810 To reproduce the issue, please select large item (e.g. 500) first and click the button right hand side. Please not to verify this on Expo directly in the meantime, because Expo with RN 0.59 does not include my previous commit. ## Changelog [Android] [Fixed] - Fix Picker ArrayOutOfBoundsException during Picker.Item update from a long list Pull Request resolved: #25276 Test Plan: 1. Check the test case https://snack.expo.io/kudochien/80f810 will have exception or not. 2. Regression of https://snack.expo.io/Sy1JClEag from #13351 3. Regression of https://snack.expo.io/kudochien/android-picker-issue from #22821 4. RNTester Picker example Reviewed By: mdvacca Differential Revision: D15857426 Pulled By: axe-fb fbshipit-source-id: 8ef902447fdd1b8aeab50ad061545cd14c735e51
M-i-k-e-l
pushed a commit
to M-i-k-e-l/react-native
that referenced
this pull request
Mar 10, 2020
…e from a long list (facebook#25276) Summary: axe-fb reported this side effect from my previous commit in facebook#24793 (comment) After revisited the implementation of Android Spinner, it seems the formal way to update existing adapter is mutating it, i.e. `arrayAdapter.clear()` & `arrayAdapter.addAll()` to update a Spinner Adapter. `setAdapter()` will reset everything including `mDataChanged`. A race condition may happens between rendering a long picker list and reseting adapter. Here is a code snippet: https://snack.expo.io/kudochien/80f810 To reproduce the issue, please select large item (e.g. 500) first and click the button right hand side. Please not to verify this on Expo directly in the meantime, because Expo with RN 0.59 does not include my previous commit. ## Changelog [Android] [Fixed] - Fix Picker ArrayOutOfBoundsException during Picker.Item update from a long list Pull Request resolved: facebook#25276 Test Plan: 1. Check the test case https://snack.expo.io/kudochien/80f810 will have exception or not. 2. Regression of https://snack.expo.io/Sy1JClEag from facebook#13351 3. Regression of https://snack.expo.io/kudochien/android-picker-issue from facebook#22821 4. RNTester Picker example Reviewed By: mdvacca Differential Revision: D15857426 Pulled By: axe-fb fbshipit-source-id: 8ef902447fdd1b8aeab50ad061545cd14c735e51
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes #13351
Two root causes:
Android Spinner will reset selection to undefined after setAdapter()
which will trigger onValueChange().
The behavior is not expected for RN.
And the solution is to setSelection() explicitly
In original implementation, it setups
itemsimmediately,but delays the
selectedsetup after update transaction.There will be some race condition and incosistency
if update
itemsonly.The fix will do the setup all after update transaction.
Changelog
[Android] [Fixed] - Fix #13351 PickerAndroid will reset selected value during items update.
Test Plan