Commit 433ffe10 authored by Andrei Chulkov's avatar Andrei Chulkov Committed by Commit Bot

Hide picture in illustration dialogs when screen height is to small

The text is usually the most important part of the illustration dialogs.
However, the image takes up most of the space in landscape or
multi-window mode, making it impossible to read the text on many devices.
This CL implements hiding the illustration in cases where there isn't
enough vertical space to show it.


Bug: 983445
Change-Id: I990d1beb2d89bedd9b0d135a8de614de471a1a10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1761601
Commit-Queue: Andrei Chulkov <achulkov@google.com>
Reviewed-by: default avatarTheresa  <twellington@chromium.org>
Reviewed-by: default avatarFriedrich [CET] <fhorschig@chromium.org>
Reviewed-by: default avatarPeter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690839}
parent 682a7e27
......@@ -6,34 +6,50 @@
<org.chromium.chrome.browser.password_manager.PasswordManagerDialogView
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
xmlns:app="http://schemas.android.com/apk/res-auto"
android:orientation="vertical"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:padding="@dimen/password_manager_dialog_padding"
android:gravity="center">
<ImageView
android:id="@+id/password_manager_dialog_illustration"
<!-- PasswordManagerDialogView extends ScrollView, which can only have one child.
Thus an additional layout to wrap everything is needed. -->
<LinearLayout
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:paddingTop="@dimen/password_manager_illustration_margin"
android:layout_marginStart="@dimen/password_manager_illustration_margin"
android:layout_marginEnd="@dimen/password_manager_illustration_margin"
android:scaleType="fitCenter"
tools:ignore="ContentDescription" />
android:layout_height="match_parent"
android:orientation="vertical">
<org.chromium.ui.widget.TextViewWithLeading
android:id="@+id/password_manager_dialog_title"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginTop="@dimen/dialog_header_margin"
android:layout_marginBottom="@dimen/dialog_header_margin"
android:textAppearance="@style/TextAppearance.BlackHeadline" />
<ImageView
android:id="@+id/password_manager_dialog_illustration"
android:layout_width="match_parent"
android:layout_height="@dimen/password_manager_dialog_illustration_height"
android:layout_gravity="center"
android:adjustViewBounds="true"
android:scaleType="fitCenter"
tools:ignore="ContentDescription" />
<org.chromium.ui.widget.TextViewWithLeading
android:id="@+id/password_manager_dialog_details"
android:layout_width="match_parent"
android:layout_height="wrap_content"
style="@style/TextAppearance.BlackBody" />
<org.chromium.ui.widget.TextViewWithLeading
android:id="@+id/password_manager_dialog_title"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:paddingTop="@dimen/password_manager_dialog_title_padding_top"
android:paddingLeft="@dimen/password_manager_dialog_text_horizontal_padding"
android:paddingRight="@dimen/password_manager_dialog_text_horizontal_padding"
app:leading="@dimen/headline_size_leading"
android:textAppearance="@style/TextAppearance.BlackHeadline" />
<org.chromium.ui.widget.TextViewWithLeading
android:id="@+id/password_manager_dialog_details"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:paddingTop="@dimen/password_manager_dialog_details_padding_top"
android:paddingBottom="@dimen/password_manager_dialog_details_padding_bottom"
android:paddingLeft="@dimen/password_manager_dialog_text_horizontal_padding"
android:paddingRight="@dimen/password_manager_dialog_text_horizontal_padding"
app:leading="@dimen/text_size_medium_leading"
style="@style/TextAppearance.BlackBody" />
</LinearLayout>
</org.chromium.chrome.browser.password_manager.PasswordManagerDialogView>
\ No newline at end of file
......@@ -159,8 +159,13 @@
<dimen name="password_generation_dialog_padding">24dp</dimen>
<!-- Password manager illustration dialogs -->
<dimen name="password_manager_dialog_padding">16dp</dimen>
<dimen name="password_manager_illustration_margin">24dp</dimen>
<dimen name="password_manager_dialog_padding">8dp</dimen>
<dimen name="password_manager_dialog_illustration_height">156dp</dimen>
<dimen name="password_manager_dialog_min_vertical_space_to_show_illustration">350dp</dimen>
<dimen name="password_manager_dialog_text_horizontal_padding">16dp</dimen>
<dimen name="password_manager_dialog_title_padding_top">16dp</dimen>
<dimen name="password_manager_dialog_details_padding_bottom">22dp</dimen>
<dimen name="password_manager_dialog_details_padding_top">12dp</dimen>
<!-- Promo dialogs -->
<dimen name="promo_dialog_illustration_margin">24dp</dimen>
......
......@@ -215,6 +215,23 @@ public class TabModalPresenter
}
}
// Calculate the top margin of the dialog container and the dialog scrim
// so that the scrim doesn't overlap the toolbar.
public static int getContainerTopMargin(Resources resources, int containerHeightResource) {
int scrimVerticalMargin =
resources.getDimensionPixelSize(R.dimen.tab_modal_scrim_vertical_margin);
int containerVerticalMargin = -scrimVerticalMargin;
if (containerHeightResource != ChromeActivity.NO_CONTROL_CONTAINER) {
containerVerticalMargin += resources.getDimensionPixelSize(containerHeightResource);
}
return containerVerticalMargin;
}
// Calculate the bottom margin of the dialog container.
public static int getContainerBottomMargin(ChromeFullscreenManager manager) {
return manager.getBottomControlsHeight();
}
/**
* Inflate the dialog container in the dialog container view stub.
*/
......@@ -236,25 +253,18 @@ public class TabModalPresenter
mChromeActivity.findViewById(R.id.tab_modal_dialog_container_sibling_view);
assert mDefaultNextSiblingView != null;
// Set the margin of the container and the dialog scrim so that the scrim doesn't overlap
// the toolbar.
Resources resources = mChromeActivity.getResources();
int scrimVerticalMargin =
resources.getDimensionPixelSize(R.dimen.tab_modal_scrim_vertical_margin);
int containerVerticalMargin = -scrimVerticalMargin;
int containerHeightResource = mChromeActivity.getControlContainerHeightResource();
if (containerHeightResource != ChromeActivity.NO_CONTROL_CONTAINER) {
containerVerticalMargin += resources.getDimensionPixelSize(containerHeightResource);
}
MarginLayoutParams params = (MarginLayoutParams) mDialogContainer.getLayoutParams();
params.width = ViewGroup.MarginLayoutParams.MATCH_PARENT;
params.height = ViewGroup.MarginLayoutParams.MATCH_PARENT;
params.topMargin = containerVerticalMargin;
params.bottomMargin = mChromeActivity.getFullscreenManager().getBottomControlsHeight();
params.topMargin = getContainerTopMargin(
resources, mChromeActivity.getControlContainerHeightResource());
params.bottomMargin = getContainerBottomMargin(mChromeActivity.getFullscreenManager());
mDialogContainer.setLayoutParams(params);
int scrimVerticalMargin =
resources.getDimensionPixelSize(R.dimen.tab_modal_scrim_vertical_margin);
View scrimView = mDialogContainer.findViewById(R.id.scrim);
params = (MarginLayoutParams) scrimView.getLayoutParams();
params.width = MarginLayoutParams.MATCH_PARENT;
......
......@@ -18,7 +18,10 @@ public class CredentialLeakDialogBridge {
WindowAndroid windowAndroid, long nativeCredentialLeakDialogViewAndroid) {
mNativeCredentialLeakDialogViewAndroid = nativeCredentialLeakDialogViewAndroid;
ChromeActivity activity = (ChromeActivity) windowAndroid.getActivity().get();
mCredentialLeakDialog = new PasswordManagerDialogCoordinator(activity);
mCredentialLeakDialog = new PasswordManagerDialogCoordinator(
windowAndroid.getContext().get(), activity.getModalDialogManager(),
activity.findViewById(android.R.id.content), activity.getFullscreenManager(),
activity.getControlContainerHeightResource());
}
@CalledByNative
......
......@@ -20,8 +20,11 @@ public class OnboardingDialogBridge {
private OnboardingDialogBridge(WindowAndroid windowAndroid, long nativeOnboardingDialogView) {
mNativeOnboardingDialogView = nativeOnboardingDialogView;
// TODO(crbug.com/983445): Get rid of this in favor of passing in direct dependencies.
ChromeActivity activity = (ChromeActivity) windowAndroid.getActivity().get();
mOnboardingDialog = new PasswordManagerDialogCoordinator(activity);
mOnboardingDialog = new PasswordManagerDialogCoordinator(windowAndroid.getContext().get(),
activity.getModalDialogManager(), activity.findViewById(android.R.id.content),
activity.getFullscreenManager(), activity.getControlContainerHeightResource());
mResources = activity.getResources();
}
......
......@@ -4,15 +4,17 @@
package org.chromium.chrome.browser.password_manager;
import android.content.Context;
import android.support.annotation.DrawableRes;
import android.support.annotation.NonNull;
import android.view.LayoutInflater;
import android.view.View;
import org.chromium.base.Callback;
import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeActivity;
import org.chromium.chrome.browser.fullscreen.ChromeFullscreenManager;
import org.chromium.ui.modaldialog.DialogDismissalCause;
import org.chromium.ui.modaldialog.ModalDialogManager;
import org.chromium.ui.modaldialog.ModalDialogProperties;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.PropertyModelChangeProcessor;
......@@ -24,12 +26,15 @@ import org.chromium.ui.modelutil.PropertyModelChangeProcessor;
public class PasswordManagerDialogCoordinator {
private final PasswordManagerDialogMediator mMediator;
PasswordManagerDialogCoordinator(@NonNull ChromeActivity activity) {
PasswordManagerDialogCoordinator(Context context, ModalDialogManager modalDialogManager,
View androidContentView, ChromeFullscreenManager fullscreenManager,
int containerHeightResource) {
PropertyModel mModel = PasswordManagerDialogProperties.defaultModelBuilder().build();
View customView =
LayoutInflater.from(activity).inflate(R.layout.password_manager_dialog, null);
mMediator = new PasswordManagerDialogMediator(
mModel, activity.getModalDialogManager(), createDialogModelBuilder(customView));
LayoutInflater.from(context).inflate(R.layout.password_manager_dialog, null);
mMediator = new PasswordManagerDialogMediator(mModel, createDialogModelBuilder(customView),
modalDialogManager, androidContentView, customView.getResources(),
fullscreenManager, containerHeightResource);
PropertyModelChangeProcessor.create(
mModel, customView, PasswordManagerDialogViewBinder::bind);
}
......@@ -49,4 +54,9 @@ public class PasswordManagerDialogCoordinator {
return new PropertyModel.Builder(ModalDialogProperties.ALL_KEYS)
.with(ModalDialogProperties.CUSTOM_VIEW, customView);
}
@VisibleForTesting
public PasswordManagerDialogMediator getMediatorForTesting() {
return mMediator;
}
}
......@@ -6,11 +6,18 @@ package org.chromium.chrome.browser.password_manager;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.DETAILS;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.ILLUSTRATION;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.ILLUSTRATION_VISIBLE;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.TITLE;
import android.content.res.Resources;
import android.support.annotation.DrawableRes;
import android.view.View;
import org.chromium.base.Callback;
import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.fullscreen.ChromeFullscreenManager;
import org.chromium.chrome.browser.modaldialog.TabModalPresenter;
import org.chromium.ui.modaldialog.ModalDialogManager;
import org.chromium.ui.modaldialog.ModalDialogProperties;
import org.chromium.ui.modelutil.PropertyModel;
......@@ -19,11 +26,15 @@ import org.chromium.ui.modelutil.PropertyModel;
* Mediator class responsible for the logic of showing the password manager dialog (e.g. onboarding
* dialog).
*/
class PasswordManagerDialogMediator {
class PasswordManagerDialogMediator implements View.OnLayoutChangeListener {
private final PropertyModel mModel;
private final ModalDialogManager mDialogManager;
private PropertyModel.Builder mModalDialogBuilder;
private PropertyModel mDialogModel;
private final View mAndroidContentView;
private final Resources mResources;
private final ChromeFullscreenManager mFullscreenManager;
private final int mContainerHeightResource;
private static class DialogClickHandler implements ModalDialogProperties.Controller {
private Callback<Boolean> mCallback;
......@@ -52,11 +63,17 @@ class PasswordManagerDialogMediator {
}
}
PasswordManagerDialogMediator(
PropertyModel model, ModalDialogManager manager, PropertyModel.Builder dialogBuilder) {
PasswordManagerDialogMediator(PropertyModel model, PropertyModel.Builder dialogBuilder,
ModalDialogManager manager, View androidContentView, Resources resources,
ChromeFullscreenManager fullscreenManager, int containerHeightResource) {
mModel = model;
mDialogManager = manager;
mModalDialogBuilder = dialogBuilder;
mAndroidContentView = androidContentView;
mResources = resources;
mFullscreenManager = fullscreenManager;
mContainerHeightResource = containerHeightResource;
mAndroidContentView.addOnLayoutChangeListener(this);
}
void setContents(String title, String details, @DrawableRes int drawableId) {
......@@ -72,12 +89,36 @@ class PasswordManagerDialogMediator {
.with(ModalDialogProperties.NEGATIVE_BUTTON_TEXT, negativeButtonText);
}
private boolean hasSufficientSpaceForIllustration(int heightPx) {
heightPx -= TabModalPresenter.getContainerTopMargin(mResources, mContainerHeightResource);
heightPx -= TabModalPresenter.getContainerBottomMargin(mFullscreenManager);
return heightPx >= mResources.getDimensionPixelSize(
R.dimen.password_manager_dialog_min_vertical_space_to_show_illustration);
}
@Override
public void onLayoutChange(View view, int left, int top, int right, int bottom, int oldLeft,
int oldTop, int oldRight, int oldBottom) {
int oldHeight = oldBottom - oldTop;
int newHeight = bottom - top;
if (newHeight == oldHeight) return;
mModel.set(ILLUSTRATION_VISIBLE, hasSufficientSpaceForIllustration(newHeight));
}
void showDialog() {
mModel.set(ILLUSTRATION_VISIBLE,
hasSufficientSpaceForIllustration(mAndroidContentView.getHeight()));
mDialogModel = mModalDialogBuilder.build();
mDialogManager.showDialog(mDialogModel, ModalDialogManager.ModalDialogType.TAB);
}
void dismissDialog(int dismissalClause) {
mDialogManager.dismissDialog(mDialogModel, dismissalClause);
mAndroidContentView.removeOnLayoutChangeListener(this);
}
@VisibleForTesting
public PropertyModel getModelForTesting() {
return mModel;
}
}
......@@ -5,6 +5,7 @@
package org.chromium.chrome.browser.password_manager;
import org.chromium.ui.modelutil.PropertyModel;
import org.chromium.ui.modelutil.PropertyModel.WritableBooleanPropertyKey;
import org.chromium.ui.modelutil.PropertyModel.WritableIntPropertyKey;
import org.chromium.ui.modelutil.PropertyModel.WritableObjectPropertyKey;
......@@ -15,6 +16,9 @@ class PasswordManagerDialogProperties {
// Illustration drawable resource id for the password manager.
static final WritableIntPropertyKey ILLUSTRATION = new WritableIntPropertyKey();
// Boolean indicating whether there is enough space for the illustration to be shown.
static final WritableBooleanPropertyKey ILLUSTRATION_VISIBLE = new WritableBooleanPropertyKey();
// Title that appears below the illustration.
static final WritableObjectPropertyKey<String> TITLE = new WritableObjectPropertyKey<>();
......@@ -24,6 +28,6 @@ class PasswordManagerDialogProperties {
private PasswordManagerDialogProperties() {}
static PropertyModel.Builder defaultModelBuilder() {
return new PropertyModel.Builder(ILLUSTRATION, TITLE, DETAILS);
return new PropertyModel.Builder(ILLUSTRATION, ILLUSTRATION_VISIBLE, TITLE, DETAILS);
}
}
......@@ -8,7 +8,7 @@ import android.content.Context;
import android.support.annotation.Nullable;
import android.util.AttributeSet;
import android.widget.ImageView;
import android.widget.LinearLayout;
import android.widget.ScrollView;
import android.widget.TextView;
import org.chromium.chrome.R;
......@@ -17,7 +17,7 @@ import org.chromium.chrome.R;
* The dialog content view for illustration dialogs used by the password manager (e.g. onboarding,
* leak warning).
*/
public class PasswordManagerDialogView extends LinearLayout {
public class PasswordManagerDialogView extends ScrollView {
private ImageView mIllustrationView;
private TextView mTitleView;
private TextView mDetailsView;
......@@ -39,6 +39,16 @@ public class PasswordManagerDialogView extends LinearLayout {
mIllustrationView.setImageResource(illustration);
}
public void updateIllustrationVisibility(boolean illustrationVisible) {
if (illustrationVisible) {
mIllustrationView.setVisibility(VISIBLE);
} else {
mIllustrationView.setVisibility(GONE);
requestLayout(); // Sometimes the visibility isn't propagated correctly and the image
// appears as INVISIBLE.
}
}
void setTitle(String title) {
mTitleView.setText(title);
}
......
......@@ -6,6 +6,7 @@ package org.chromium.chrome.browser.password_manager;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.DETAILS;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.ILLUSTRATION;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.ILLUSTRATION_VISIBLE;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.TITLE;
import android.view.View;
......@@ -21,6 +22,8 @@ class PasswordManagerDialogViewBinder {
PasswordManagerDialogView dialogView = (PasswordManagerDialogView) view;
if (ILLUSTRATION == propertyKey) {
dialogView.setIllustration(model.get(ILLUSTRATION));
} else if (ILLUSTRATION_VISIBLE == propertyKey) {
dialogView.updateIllustrationVisibility(model.get(ILLUSTRATION_VISIBLE));
} else if (TITLE == propertyKey) {
dialogView.setTitle(model.get(TITLE));
} else if (DETAILS == propertyKey) {
......
......@@ -7,12 +7,17 @@ package org.chromium.chrome.browser.password_manager;
import static android.support.test.espresso.Espresso.onView;
import static android.support.test.espresso.action.ViewActions.click;
import static android.support.test.espresso.assertion.ViewAssertions.matches;
import static android.support.test.espresso.matcher.ViewMatchers.assertThat;
import static android.support.test.espresso.matcher.ViewMatchers.isDisplayed;
import static android.support.test.espresso.matcher.ViewMatchers.withId;
import static android.support.test.espresso.matcher.ViewMatchers.withText;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.not;
import static org.mockito.Mockito.verify;
import static org.chromium.chrome.browser.password_manager.PasswordManagerDialogProperties.ILLUSTRATION_VISIBLE;
import android.support.test.filters.SmallTest;
import org.junit.Before;
......@@ -32,12 +37,15 @@ import org.chromium.chrome.browser.ChromeSwitches;
import org.chromium.chrome.test.ChromeActivityTestRule;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.ui.modelutil.PropertyModel;
/** Test for the password manager illustration modal dialog. */
@RunWith(ChromeJUnit4ClassRunner.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
public class PasswordManagerDialogTest {
private PasswordManagerDialogCoordinator mDialog;
private PasswordManagerDialogCoordinator mCoordinator;
private PasswordManagerDialogMediator mMediator;
private PropertyModel mModel;
private static final String TITLE = "Title";
private static final String DETAILS = "Explanation text.";
private static final String OK_BUTTON = "OK";
......@@ -56,10 +64,16 @@ public class PasswordManagerDialogTest {
@Before
public void setUp() throws InterruptedException {
mActivityTestRule.startMainActivityOnBlankPage();
mDialog = new PasswordManagerDialogCoordinator(mActivityTestRule.getActivity());
ChromeActivity activity = (ChromeActivity) mActivityTestRule.getActivity();
mCoordinator = new PasswordManagerDialogCoordinator(
activity.getWindowAndroid().getContext().get(), activity.getModalDialogManager(),
activity.findViewById(android.R.id.content), activity.getFullscreenManager(),
activity.getControlContainerHeightResource());
mMediator = mCoordinator.getMediatorForTesting();
mModel = mMediator.getModelForTesting();
TestThreadUtils.runOnUiThreadBlocking(() -> {
mDialog.showDialog(TITLE, DETAILS, R.drawable.data_reduction_illustration, OK_BUTTON,
CANCEL_BUTTON, mOnClick);
mCoordinator.showDialog(TITLE, DETAILS, R.drawable.data_reduction_illustration,
OK_BUTTON, CANCEL_BUTTON, mOnClick);
});
}
......@@ -86,4 +100,42 @@ public class PasswordManagerDialogTest {
onView(withId(R.id.negative_button)).perform(click());
verify(mOnClick).onResult(false);
}
@Test
@SmallTest
public void testSettingImageVisibility() {
TestThreadUtils.runOnUiThreadBlocking(() -> { mModel.set(ILLUSTRATION_VISIBLE, false); });
onView(withId(R.id.password_manager_dialog_illustration))
.check(matches(not(isDisplayed())));
TestThreadUtils.runOnUiThreadBlocking(() -> { mModel.set(ILLUSTRATION_VISIBLE, true); });
onView(withId(R.id.password_manager_dialog_illustration)).check(matches(isDisplayed()));
}
@Test
@SmallTest
public void testWatchingLayoutChanges() {
float dipScale =
mActivityTestRule.getActivity().getWindowAndroid().getDisplay().getDipScale();
// Dimensions resembling landscape orientation.
int testHeightDip = 300; // Height of the android content view.
int testWidthDip = 500; // Width of the android content view.
mMediator.onLayoutChange(null, 0, 0, (int) (testWidthDip * dipScale),
(int) (testHeightDip * dipScale), 0, 0, 0, 0);
assertThat(mModel.get(ILLUSTRATION_VISIBLE), is(false));
// Dimensions resembling portrait orientation.
testHeightDip = 500;
testWidthDip = 320;
mMediator.onLayoutChange(null, 0, 0, (int) (testWidthDip * dipScale),
(int) (testHeightDip * dipScale), 0, 0, 0, 0);
assertThat(mModel.get(ILLUSTRATION_VISIBLE), is(true));
// Dimensions resembling multi-window mode.
testHeightDip = 250;
testWidthDip = 320;
mMediator.onLayoutChange(null, 0, 0, (int) (testWidthDip * dipScale),
(int) (testHeightDip * dipScale), 0, 0, 0, 0);
assertThat(mModel.get(ILLUSTRATION_VISIBLE), is(false));
}
}
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