Separate scrim/sheet custom lifecycles in onSheetClosed
This patch fixes a subtle bug in the BottomSheetController where a
custom scrim lifecycle inadvertently blocked high priority content
from suppressing low priority content. The block of code no longer
returns early, each scrim and sheet check are now gated on
BottomSheetContent's #hasCustomScrimLifecycle and #hasCustomLifecycle
respectively.
Bug: 986310
Change-Id: I506874b0f8d63e5d00177b409a71b92a51cfb454
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1779103
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751450}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java b/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java
index 1dc072d..4138c21 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetController.java
@@ -327,21 +327,24 @@
@Override
public void onSheetClosed(@StateChangeReason int reason) {
- if (mBottomSheet.getCurrentSheetContent() != null
- && mBottomSheet.getCurrentSheetContent().hasCustomScrimLifecycle()) {
- return;
+ // Hide the scrim if the current content doesn't have a custom scrim lifecycle.
+ if (mBottomSheet.getCurrentSheetContent() == null
+ || !mBottomSheet.getCurrentSheetContent().hasCustomScrimLifecycle()) {
+ scrim.get().hideScrim(true);
}
- scrim.get().hideScrim(true);
-
- // If the sheet is closed, it is an opportunity for another content to try to
- // take its place if it is a higher priority.
- BottomSheetContent content = mBottomSheet.getCurrentSheetContent();
- BottomSheetContent nextContent = mContentQueue.peek();
- if (content != null && nextContent != null
- && nextContent.getPriority() < content.getPriority()) {
- mContentQueue.add(content);
- mBottomSheet.setSheetState(SheetState.HIDDEN, true);
+ // Try to swap contents unless the sheet's content has a custom lifecycle.
+ if (mBottomSheet.getCurrentSheetContent() != null
+ && !mBottomSheet.getCurrentSheetContent().hasCustomLifecycle()) {
+ // If the sheet is closed, it is an opportunity for another content to try to
+ // take its place if it is a higher priority.
+ BottomSheetContent content = mBottomSheet.getCurrentSheetContent();
+ BottomSheetContent nextContent = mContentQueue.peek();
+ if (content != null && nextContent != null
+ && nextContent.getPriority() < content.getPriority()) {
+ mContentQueue.add(content);
+ mBottomSheet.setSheetState(SheetState.HIDDEN, true);
+ }
}
}
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetControllerTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetControllerTest.java
index 5c9d78d..fb42251f 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetControllerTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/BottomSheetControllerTest.java
@@ -11,6 +11,7 @@
import android.support.test.InstrumentationRegistry;
import android.support.test.filters.MediumTest;
import android.support.test.filters.SmallTest;
+import android.view.View;
import android.view.ViewGroup;
import org.junit.Before;
@@ -348,6 +349,20 @@
@Test
@MediumTest
+ public void testCustomScrimLifecycle() throws TimeoutException {
+ TestBottomSheetContent customScrimContent = new TestBottomSheetContent(
+ mActivityTestRule.getActivity(), BottomSheetContent.ContentPriority.LOW, true);
+ customScrimContent.setHasCustomScrimLifecycle(true);
+ requestContentInSheet(customScrimContent, true);
+
+ expandSheet();
+
+ assertEquals("The scrim should not be visible with a custom scrim lifecycle.", View.GONE,
+ mScrimView.getVisibility());
+ }
+
+ @Test
+ @MediumTest
public void testCloseEvent() throws TimeoutException {
requestContentInSheet(mHighPriorityContent, true);
expandSheet();
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/TestBottomSheetContent.java b/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/TestBottomSheetContent.java
index 2a9e85e..6357b12 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/TestBottomSheetContent.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/widget/bottomsheet/TestBottomSheetContent.java
@@ -35,6 +35,9 @@
/** Whether this content is browser specific. */
private boolean mHasCustomLifecycle;
+ /** Whether this content has a custom scrim lifecycle. */
+ private boolean mHasCustomScrimLifecycle;
+
/** The peek height of this content. */
private int mPeekHeight;
@@ -151,6 +154,15 @@
return mFullHeight;
}
+ public void setHasCustomScrimLifecycle(boolean hasCustomScrimLifecycle) {
+ mHasCustomScrimLifecycle = hasCustomScrimLifecycle;
+ }
+
+ @Override
+ public boolean hasCustomScrimLifecycle() {
+ return mHasCustomScrimLifecycle;
+ }
+
@Override
public boolean hasCustomLifecycle() {
return mHasCustomLifecycle;
