Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Picker.onValueChange on Android sometimes not fired due to race condition #22821

Closed
wants to merge 1 commit into from
Closed

Fix Picker.onValueChange on Android sometimes not fired due to race condition #22821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 9 additions & 42 deletions Libraries/Components/Picker/PickerAndroid.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ type Item = $ReadOnly<{|
|}>;

type PickerAndroidState = {|
initialSelectedIndex: number,
selectedIndex: number,
items: $ReadOnlyArray<Item>,
|};
Expand All @@ -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) {
Expand All @@ -99,7 +81,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
Open in desktop
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