Fix Picker.onValueChange on Android sometimes not fired due to race c…

…ondition
facebook · Dec 28, 2018 · 43eaccf · 43eaccf
1 parent f3e5cce
commit 43eaccf
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 71 deletions.
diff --git a/Libraries/Components/Picker/PickerAndroid.android.js b/Libraries/Components/Picker/PickerAndroid.android.js
@@ -49,7 +49,6 @@ type Item = $ReadOnly<{|
 |}>;
 
 type PickerAndroidState = {|
-  initialSelectedIndex: number,
   selectedIndex: number,
   items: $ReadOnlyArray<Item>,
 |};
@@ -62,26 +61,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) {
@@ -99,7 +81,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
@@ -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 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 void requestLayout() {
     post(measureAndLayout);
   }
 
+  @Override
+  protected void onLayout(boolean changed, int left, int top, int right, int bottom) {
+    super.onLayout(changed, left, top, right, bottom);
+
+    // 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) {
-    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
-            public void onNothingSelected(AdapterView<?> parent) {
-              if (!mSuppressNextEvent && mOnSelectListener != null) {
-                mOnSelectListener.onItemSelected(-1);
-              }
-              mSuppressNextEvent = false;
-            }
-          });
-    }
     mOnSelectListener = onSelectListener;
   }
 
@@ -130,8 +133,9 @@ public void updateStagedSelection() {
    */
   private void setSelectionWithSuppressEvent(int position) {
     if (position != getSelectedItemPosition()) {
-      mSuppressNextEvent = true;
-      setSelection(position);
+      setOnItemSelectedListener(null);
+      setSelection(position, false);
+      setOnItemSelectedListener(mItemSelectedListener);
     }
   }