Commit 373f5da7 authored by Friedrich Horschig's avatar Friedrich Horschig Committed by Commit Bot

[Android][TouchToFill] Use HALF state instead of PEEK and add fail-safe

Before this CL, the touch to fill sheet was intially shown in a very
high PEEK state (which provides pixel-exact height control).
The PEEK state had the side effect that the scrim view wasn't brought up
reliably (see linked bug). Therefore, it is skipped now and instead, the
half state is invoked right away. To the user, this looks and behaves
exaclty the same but triggers the scrim view more reliably.

Additionally, there is a small fail-safe that would have fixed the issue
on its own (just not as clean). The fail-safe ensures the sheet is
dismissed as soon as there the BottomSheet enters the HIDDEN state, even
if the observer wasn't notified that is was closed beforehand.

Minifix: the bottom sheet vertical size seems to align with Chrome now,
so set the margin of the "Manage Passwords" button back to 8dp.

Bug: 1017727
Change-Id: I66ddef169a96d43a059bd7f065195c03783d595f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879447Reviewed-by: default avatarJan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709118}
parent 06f995e7
...@@ -45,7 +45,7 @@ ...@@ -45,7 +45,7 @@
android:text="@string/manage_passwords" android:text="@string/manage_passwords"
android:layout_width="match_parent" android:layout_width="match_parent"
android:layout_height="wrap_content" android:layout_height="wrap_content"
android:layout_marginBottom="24dp" android:layout_marginBottom="8dp"
android:paddingStart="@dimen/touch_to_fill_sheet_margin" android:paddingStart="@dimen/touch_to_fill_sheet_margin"
android:paddingEnd="@dimen/touch_to_fill_sheet_margin" android:paddingEnd="@dimen/touch_to_fill_sheet_margin"
android:minHeight="48dp" android:minHeight="48dp"
......
...@@ -39,6 +39,15 @@ class TouchToFillView implements BottomSheet.BottomSheetContent { ...@@ -39,6 +39,15 @@ class TouchToFillView implements BottomSheet.BottomSheetContent {
mDismissHandler.onResult(reason); mDismissHandler.onResult(reason);
mBottomSheetController.getBottomSheet().removeObserver(mBottomSheetObserver); mBottomSheetController.getBottomSheet().removeObserver(mBottomSheetObserver);
} }
@Override
public void onSheetStateChanged(int newState) {
super.onSheetStateChanged(newState);
if (newState != BottomSheet.SheetState.HIDDEN) return;
// This is a fail-safe for cases where onSheetClosed isn't triggered.
mDismissHandler.onResult(BottomSheet.StateChangeReason.NONE);
mBottomSheetController.getBottomSheet().removeObserver(mBottomSheetObserver);
}
}; };
/** /**
...@@ -129,8 +138,14 @@ class TouchToFillView implements BottomSheet.BottomSheetContent { ...@@ -129,8 +138,14 @@ class TouchToFillView implements BottomSheet.BottomSheetContent {
@Override @Override
public int getPeekHeight() { public int getPeekHeight() {
return BottomSheet.HeightMode.DISABLED;
}
@Override
public float getCustomHalfRatio() {
return Math.min(mContext.getResources().getDimensionPixelSize(getDesiredSheetHeight()), return Math.min(mContext.getResources().getDimensionPixelSize(getDesiredSheetHeight()),
(int) mBottomSheetController.getBottomSheet().getSheetContainerHeight()); (int) mBottomSheetController.getBottomSheet().getSheetContainerHeight())
/ mBottomSheetController.getBottomSheet().getSheetContainerHeight();
} }
@Override @Override
......
...@@ -82,7 +82,7 @@ public class TouchToFillIntegrationTest { ...@@ -82,7 +82,7 @@ public class TouchToFillIntegrationTest {
runOnUiThreadBlocking(() -> { runOnUiThreadBlocking(() -> {
mTouchToFill.showCredentials(EXAMPLE_URL, true, Collections.singletonList(ANA)); mTouchToFill.showCredentials(EXAMPLE_URL, true, Collections.singletonList(ANA));
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
pollUiThread(() -> getCredentials().getChildAt(0) != null); pollUiThread(() -> getCredentials().getChildAt(0) != null);
TouchCommon.singleClickView(getCredentials().getChildAt(0)); TouchCommon.singleClickView(getCredentials().getChildAt(0));
...@@ -98,7 +98,7 @@ public class TouchToFillIntegrationTest { ...@@ -98,7 +98,7 @@ public class TouchToFillIntegrationTest {
runOnUiThreadBlocking(() -> { runOnUiThreadBlocking(() -> {
mTouchToFill.showCredentials(EXAMPLE_URL, true, Arrays.asList(ANA, BOB)); mTouchToFill.showCredentials(EXAMPLE_URL, true, Arrays.asList(ANA, BOB));
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
Espresso.pressBack(); Espresso.pressBack();
......
...@@ -97,7 +97,7 @@ public class TouchToFillViewTest { ...@@ -97,7 +97,7 @@ public class TouchToFillViewTest {
public void testVisibilityChangedByModel() { public void testVisibilityChangedByModel() {
// After setting the visibility to true, the view should exist and be visible. // After setting the visibility to true, the view should exist and be visible.
TestThreadUtils.runOnUiThreadBlocking(() -> mModel.set(VISIBLE, true)); TestThreadUtils.runOnUiThreadBlocking(() -> mModel.set(VISIBLE, true));
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
assertThat(mTouchToFillView.getContentView().isShown(), is(true)); assertThat(mTouchToFillView.getContentView().isShown(), is(true));
// After hiding the view, the view should still exist but be invisible. // After hiding the view, the view should still exist but be invisible.
...@@ -118,7 +118,7 @@ public class TouchToFillViewTest { ...@@ -118,7 +118,7 @@ public class TouchToFillViewTest {
.build())); .build()));
mModel.set(VISIBLE, true); mModel.set(VISIBLE, true);
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
TextView subtitle = TextView subtitle =
mTouchToFillView.getContentView().findViewById(R.id.touch_to_fill_sheet_subtitle); mTouchToFillView.getContentView().findViewById(R.id.touch_to_fill_sheet_subtitle);
...@@ -137,7 +137,7 @@ public class TouchToFillViewTest { ...@@ -137,7 +137,7 @@ public class TouchToFillViewTest {
.build())); .build()));
mModel.set(VISIBLE, true); mModel.set(VISIBLE, true);
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
TextView subtitle = TextView subtitle =
mTouchToFillView.getContentView().findViewById(R.id.touch_to_fill_sheet_subtitle); mTouchToFillView.getContentView().findViewById(R.id.touch_to_fill_sheet_subtitle);
...@@ -154,7 +154,7 @@ public class TouchToFillViewTest { ...@@ -154,7 +154,7 @@ public class TouchToFillViewTest {
buildCredentialItem(BOB))); buildCredentialItem(BOB)));
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
assertThat(getCredentials().getChildCount(), is(3)); assertThat(getCredentials().getChildCount(), is(3));
assertThat(getCredentialOriginAt(0).getVisibility(), is(View.GONE)); assertThat(getCredentialOriginAt(0).getVisibility(), is(View.GONE));
assertThat(getCredentialNameAt(0).getText(), is(ANA.getFormattedUsername())); assertThat(getCredentialNameAt(0).getText(), is(ANA.getFormattedUsername()));
...@@ -182,7 +182,7 @@ public class TouchToFillViewTest { ...@@ -182,7 +182,7 @@ public class TouchToFillViewTest {
mModel.get(SHEET_ITEMS).addAll(Collections.singletonList(buildCredentialItem(ANA))); mModel.get(SHEET_ITEMS).addAll(Collections.singletonList(buildCredentialItem(ANA)));
mModel.set(VISIBLE, true); mModel.set(VISIBLE, true);
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
assertNotNull(getCredentials().getChildAt(0)); assertNotNull(getCredentials().getChildAt(0));
...@@ -199,7 +199,7 @@ public class TouchToFillViewTest { ...@@ -199,7 +199,7 @@ public class TouchToFillViewTest {
mModel.set(ON_CLICK_MANAGE, () -> manageButtonClicked.set(true)); mModel.set(ON_CLICK_MANAGE, () -> manageButtonClicked.set(true));
mModel.set(VISIBLE, true); mModel.set(VISIBLE, true);
}); });
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
TestThreadUtils.runOnUiThreadBlocking( TestThreadUtils.runOnUiThreadBlocking(
() -> { getActivity().getBottomSheet().setSheetState(SheetState.FULL, false); }); () -> { getActivity().getBottomSheet().setSheetState(SheetState.FULL, false); });
...@@ -216,7 +216,7 @@ public class TouchToFillViewTest { ...@@ -216,7 +216,7 @@ public class TouchToFillViewTest {
@MediumTest @MediumTest
public void testDismissesWhenHidden() { public void testDismissesWhenHidden() {
TestThreadUtils.runOnUiThreadBlocking(() -> mModel.set(VISIBLE, true)); TestThreadUtils.runOnUiThreadBlocking(() -> mModel.set(VISIBLE, true));
pollUiThread(() -> getBottomSheetState() == SheetState.PEEK); pollUiThread(() -> getBottomSheetState() == SheetState.HALF);
TestThreadUtils.runOnUiThreadBlocking(() -> mModel.set(VISIBLE, false)); TestThreadUtils.runOnUiThreadBlocking(() -> mModel.set(VISIBLE, false));
pollUiThread(() -> getBottomSheetState() == SheetState.HIDDEN); pollUiThread(() -> getBottomSheetState() == SheetState.HIDDEN);
verify(mDismissHandler).onResult(StateChangeReason.NONE); verify(mDismissHandler).onResult(StateChangeReason.NONE);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment