Skip to content

Commit

Permalink
[ChipGroup] Fix ChipGroup.getCheckedChipIds() returns wrong state
Browse files Browse the repository at this point in the history
In the Chip implementation, onCheckedChangeListener was called before onCheckedChangeListenerInternal. This causes an issue that in onCheckedChangeListener's callback, the checkable group's checked state is not updated yet, therefore ChipGroup.getCheckedChipIds() will return the outdated checked state.

Fixes this by overriding Chip.setOnCheckedChangeListener to get full control of the execution order between onCheckedChangeListener and onCheckedChangeListenerInternal.

Resolves #2691

PiperOrigin-RevId: 449100861
(cherry picked from commit 413a047)
  • Loading branch information
drchen authored and pekingme committed May 25, 2022
1 parent bc9595b commit 768d0cf
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 8 deletions.
29 changes: 22 additions & 7 deletions lib/java/com/google/android/material/chip/Chip.java
Expand Up @@ -54,6 +54,7 @@
import android.view.ViewParent;
import android.view.accessibility.AccessibilityEvent;
import android.view.accessibility.AccessibilityNodeInfo;
import android.widget.CompoundButton;
import androidx.annotation.AnimatorRes;
import androidx.annotation.BoolRes;
import androidx.annotation.CallSuper;
Expand Down Expand Up @@ -151,6 +152,7 @@ public class Chip extends AppCompatCheckBox
@Nullable private RippleDrawable ripple;

@Nullable private OnClickListener onCloseIconClickListener;
@Nullable private CompoundButton.OnCheckedChangeListener onCheckedChangeListener;
@Nullable private MaterialCheckable.OnCheckedChangeListener<Chip> onCheckedChangeListenerInternal;
private boolean deferredCheckedValue;
private boolean closeIconPressed;
Expand Down Expand Up @@ -251,6 +253,19 @@ public Chip(Context context, AttributeSet attrs, int defStyleAttr) {
setMinHeight(minTouchTargetSize);
}
lastLayoutDirection = ViewCompat.getLayoutDirection(this);

super.setOnCheckedChangeListener(
new CompoundButton.OnCheckedChangeListener() {
@Override
public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
if (onCheckedChangeListenerInternal != null) {
onCheckedChangeListenerInternal.onCheckedChanged(Chip.this, isChecked);
}
if (onCheckedChangeListener != null) {
onCheckedChangeListener.onCheckedChanged(buttonView, isChecked);
}
}
});
}

@Override
Expand Down Expand Up @@ -707,17 +722,17 @@ public void setChecked(boolean checked) {
// Defer the setChecked() call until after initialization.
deferredCheckedValue = checked;
} else if (chipDrawable.isCheckable()) {
boolean wasChecked = isChecked();
super.setChecked(checked);

if (wasChecked != checked) {
if (onCheckedChangeListenerInternal != null) {
onCheckedChangeListenerInternal.onCheckedChanged(this, checked);
}
}
}
}

@Override
public void setOnCheckedChangeListener(
@Nullable CompoundButton.OnCheckedChangeListener listener) {
// Do not call super here - the wrapped listener set in the constructor will call the listener.
onCheckedChangeListener = listener;
}

/** Register a callback to be invoked when the close icon is clicked. */
public void setOnCloseIconClickListener(OnClickListener listener) {
this.onCloseIconClickListener = listener;
Expand Down
Expand Up @@ -15,15 +15,18 @@
*/
package com.google.android.material.chip;

import com.google.android.material.R;
import android.widget.CompoundButton.OnCheckedChangeListener;
import com.google.android.material.test.R;

import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

import android.content.Context;
import androidx.appcompat.app.AppCompatActivity;
import android.view.View;
import android.widget.CompoundButton;
import androidx.core.view.ViewCompat;
import androidx.core.view.accessibility.AccessibilityNodeInfoCompat;
import androidx.core.view.accessibility.AccessibilityNodeInfoCompat.CollectionInfoCompat;
Expand Down Expand Up @@ -196,6 +199,39 @@ public void onCheckedChanged(ChipGroup group, List<Integer> checkedIds) {
assertThat(checkedIds).contains(second.getId());
}

@Test
public void multipleSelection_chipListener() {
chipgroup.setSingleSelection(false);

Chip first = (Chip) chipgroup.getChildAt(0);
first.setOnCheckedChangeListener(new OnCheckedChangeListener() {
@Override
public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
onChipCheckedStateChanged(buttonView, isChecked);
}
});

Chip second = (Chip) chipgroup.getChildAt(1);
second.setOnCheckedChangeListener(new OnCheckedChangeListener() {
@Override
public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) {
onChipCheckedStateChanged(buttonView, isChecked);
}
});

first.performClick();
getInstrumentation().waitForIdleSync();

assertThat(checkedChangeCallCount).isEqualTo(1);
assertThat(checkedIds).containsExactly(first.getId());

second.performClick();
getInstrumentation().waitForIdleSync();

assertThat(checkedChangeCallCount).isEqualTo(2);
assertThat(checkedIds).containsExactly(first.getId(), second.getId());
}

@Test
public void multiSelection_withSelectionRequired_unSelectsIfTwo() {
chipgroup.setSingleSelection(false);
Expand Down Expand Up @@ -260,4 +296,9 @@ public void isNotSingleLine_initializesAccessibilityNodeInfo() {
assertEquals(1, itemInfo.getRowIndex());
assertTrue(itemInfo.isSelected());
}

private void onChipCheckedStateChanged(CompoundButton chip, boolean checked) {
checkedChangeCallCount++;
checkedIds = chipgroup.getCheckedChipIds();
}
}

0 comments on commit 768d0cf

Please sign in to comment.