Skip to content

Commit

Permalink
Fix Picker.onValueChange on Android sometimes not fired due to race c…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@Kudo @facebook-github-bot
Kudo authored and facebook-github-bot committed Jan 18, 2019
1 parent e0910ad commit 5e6c124
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 71 deletions.
51 changes: 9 additions & 42 deletions Libraries/Components/Picker/PickerAndroid.android.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ type Item = $ReadOnly<{|
|}>;

type PickerAndroidState = {|
initialSelectedIndex: number,
selectedIndex: number,
items: $ReadOnlyArray<Item>,
|};
Expand All @@ -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) {
Expand All @@ -97,7 +79,9 @@ class PickerAndroid extends React.Component<
return childProps;
});
return {selectedIndex, items};
};
}

state = PickerAndroid.getDerivedStateFromProps(this.props);

render() {
const Picker =
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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({
Expand Down
62 changes: 33 additions & 29 deletions ReactAndroid/src/main/java/com/facebook/react/views/picker/ReactPicker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}
}

Expand Down

0 comments on commit 5e6c124

Please sign in to comment.