From ba9803a809b56540964b12b47e7a47f5916b37d2 Mon Sep 17 00:00:00 2001 From: conradchen Date: Mon, 16 May 2022 21:17:28 -0400 Subject: [PATCH] [ChipGroup] Fix ChipGroup.getCheckedChipIds() returns wrong state 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 https://github.com/material-components/material-components-android/issues/2691 PiperOrigin-RevId: 449100861 (cherry picked from commit 413a0479574b66be74f42bf9e8a11e1245c72a09) --- .../google/android/material/chip/Chip.java | 26 ++++++++--- .../android/material/chip/ChipGroupTest.java | 45 ++++++++++++++++++- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/lib/java/com/google/android/material/chip/Chip.java b/lib/java/com/google/android/material/chip/Chip.java index c85dda446e7..1529ba70b51 100644 --- a/lib/java/com/google/android/material/chip/Chip.java +++ b/lib/java/com/google/android/material/chip/Chip.java @@ -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; @@ -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 onCheckedChangeListenerInternal; private boolean deferredCheckedValue; private boolean closeIconPressed; @@ -251,6 +253,16 @@ public Chip(Context context, AttributeSet attrs, int defStyleAttr) { setMinHeight(minTouchTargetSize); } lastLayoutDirection = ViewCompat.getLayoutDirection(this); + + super.setOnCheckedChangeListener( + (buttonView, isChecked) -> { + if (onCheckedChangeListenerInternal != null) { + onCheckedChangeListenerInternal.onCheckedChanged(Chip.this, isChecked); + } + if (onCheckedChangeListener != null) { + onCheckedChangeListener.onCheckedChanged(buttonView, isChecked); + } + }); } @Override @@ -707,17 +719,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; diff --git a/lib/javatests/com/google/android/material/chip/ChipGroupTest.java b/lib/javatests/com/google/android/material/chip/ChipGroupTest.java index a0b43b679d9..824813f3073 100644 --- a/lib/javatests/com/google/android/material/chip/ChipGroupTest.java +++ b/lib/javatests/com/google/android/material/chip/ChipGroupTest.java @@ -15,8 +15,10 @@ */ 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; @@ -24,6 +26,7 @@ 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; @@ -196,6 +199,41 @@ public void onCheckedChanged(ChipGroup group, List 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); @@ -260,4 +298,9 @@ public void isNotSingleLine_initializesAccessibilityNodeInfo() { assertEquals(1, itemInfo.getRowIndex()); assertTrue(itemInfo.isSelected()); } + + private void onChipCheckedStateChanged(CompoundButton chip, boolean checked) { + checkedChangeCallCount++; + checkedIds = chipgroup.getCheckedChipIds(); + } }