From 6a3d9c06ce30433e4b1ded8f59f83697185282e9 Mon Sep 17 00:00:00 2001 From: Kudo Chien Date: Fri, 18 Jan 2019 08:09:07 -0800 Subject: [PATCH] Fix Picker.onValueChange on Android sometimes not fired due to race condition (#22821) Summary: 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 `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: https://github.com/facebook/react-native/pull/22821 Differential Revision: D13731979 Pulled By: cpojer fbshipit-source-id: e06bd9aa62463b66c8f3fd7214485898d5375054 --- .../Picker/PickerAndroid.android.js | 51 +++------------ .../react/views/picker/ReactPicker.java | 62 ++++++++++--------- 2 files changed, 42 insertions(+), 71 deletions(-) diff --git a/Libraries/Components/Picker/PickerAndroid.android.js b/Libraries/Components/Picker/PickerAndroid.android.js index 46b19cd1812..af72f744592 100644 --- a/Libraries/Components/Picker/PickerAndroid.android.js +++ b/Libraries/Components/Picker/PickerAndroid.android.js @@ -47,7 +47,6 @@ type Item = $ReadOnly<{| |}>; type PickerAndroidState = {| - initialSelectedIndex: number, selectedIndex: number, items: $ReadOnlyArray, |}; @@ -60,26 +59,9 @@ class PickerAndroid extends React.Component< PickerAndroidProps, PickerAndroidState, > { - /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found - * when making Flow check .android.js files. */ - constructor(props, context) { - super(props, context); - const state = this._stateFromProps(props); - - this.state = { - ...state, - initialSelectedIndex: state.selectedIndex, - }; - } - - /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found - * when making Flow check .android.js files. */ - UNSAFE_componentWillReceiveProps(nextProps) { - this.setState(this._stateFromProps(nextProps)); - } - - // Translate prop and children into stuff that the native picker understands. - _stateFromProps = props => { + static getDerivedStateFromProps( + props: PickerAndroidProps, + ): PickerAndroidState { let selectedIndex = 0; const items = React.Children.map(props.children, (child, index) => { if (child.props.value === props.selectedValue) { @@ -97,7 +79,9 @@ class PickerAndroid extends React.Component< return childProps; }); return {selectedIndex, items}; - }; + } + + state = PickerAndroid.getDerivedStateFromProps(this.props); render() { const Picker = @@ -111,7 +95,7 @@ class PickerAndroid extends React.Component< mode: this.props.mode, onSelect: this._onChange, prompt: this.props.prompt, - selected: this.state.initialSelectedIndex, + selected: this.state.selectedIndex, testID: this.props.testID, style: [styles.pickerAndroid, this.props.style], /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found @@ -135,19 +119,7 @@ class PickerAndroid extends React.Component< this.props.onValueChange(null, position); } } - /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found - * when making Flow check .android.js files. */ - this._lastNativePosition = event.nativeEvent.position; - this.forceUpdate(); - }; - componentDidMount() { - /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found - * when making Flow check .android.js files. */ - this._lastNativePosition = this.state.initialSelectedIndex; - } - - componentDidUpdate() { // The picker is a controlled component. This means we expect the // on*Change handlers to be in charge of updating our // `selectedValue` prop. That way they can also @@ -156,18 +128,13 @@ class PickerAndroid extends React.Component< // truth, not the native component. if ( this.refs[REF_PICKER] && - /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found - * when making Flow check .android.js files. */ - this.state.selectedIndex !== this._lastNativePosition + this.state.selectedIndex !== event.nativeEvent.position ) { this.refs[REF_PICKER].setNativeProps({ selected: this.state.selectedIndex, }); - /* $FlowFixMe(>=0.78.0 site=react_native_android_fb) This issue was found - * when making Flow check .android.js files. */ - this._lastNativePosition = this.state.selectedIndex; } - } + }; } const styles = StyleSheet.create({ diff --git a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java index 4f38fab0109..b1d7578ff14 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java +++ b/ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java @@ -7,8 +7,6 @@ package com.facebook.react.views.picker; -import javax.annotation.Nullable; - import android.content.Context; import android.util.AttributeSet; import android.view.View; @@ -17,14 +15,31 @@ import android.widget.Spinner; import com.facebook.react.common.annotations.VisibleForTesting; +import javax.annotation.Nullable; + public class ReactPicker extends Spinner { private int mMode = MODE_DIALOG; private @Nullable Integer mPrimaryColor; - private boolean mSuppressNextEvent; private @Nullable OnSelectListener mOnSelectListener; private @Nullable Integer mStagedSelection; + private final OnItemSelectedListener mItemSelectedListener = new OnItemSelectedListener() { + @Override + public void onItemSelected(AdapterView parent, View view, int position, long id) { + if (mOnSelectListener != null) { + mOnSelectListener.onItemSelected(position); + } + } + + @Override + public void onNothingSelected(AdapterView parent) { + if (mOnSelectListener != null) { + mOnSelectListener.onItemSelected(-1); + } + } + }; + /** * Listener interface for ReactPicker events. */ @@ -75,31 +90,19 @@ public class ReactPicker extends Spinner { post(measureAndLayout); } - public void setOnSelectListener(@Nullable OnSelectListener onSelectListener) { - if (getOnItemSelectedListener() == null) { - // onItemSelected gets fired immediately after layout because checkSelectionChanged() in - // AdapterView updates the selection position from the default INVALID_POSITION. To match iOS - // behavior, we don't want the event emitter for onItemSelected to fire right after layout. - mSuppressNextEvent = true; - setOnItemSelectedListener( - new OnItemSelectedListener() { - @Override - public void onItemSelected(AdapterView parent, View view, int position, long id) { - if (!mSuppressNextEvent && mOnSelectListener != null) { - mOnSelectListener.onItemSelected(position); - } - mSuppressNextEvent = false; - } + @Override + protected void onLayout(boolean changed, int left, int top, int right, int bottom) { + super.onLayout(changed, left, top, right, bottom); - @Override - public void onNothingSelected(AdapterView parent) { - if (!mSuppressNextEvent && mOnSelectListener != null) { - mOnSelectListener.onItemSelected(-1); - } - mSuppressNextEvent = false; - } - }); - } + // onItemSelected gets fired immediately after layout because checkSelectionChanged() in + // AdapterView updates the selection position from the default INVALID_POSITION. + // To match iOS behavior, which no onItemSelected during initial layout. + // We setup the listener after layout. + if (getOnItemSelectedListener() == null) + setOnItemSelectedListener(mItemSelectedListener); + } + + public void setOnSelectListener(@Nullable OnSelectListener onSelectListener) { mOnSelectListener = onSelectListener; } @@ -130,8 +133,9 @@ public class ReactPicker extends Spinner { */ private void setSelectionWithSuppressEvent(int position) { if (position != getSelectedItemPosition()) { - mSuppressNextEvent = true; - setSelection(position); + setOnItemSelectedListener(null); + setSelection(position, false); + setOnItemSelectedListener(mItemSelectedListener); } }