[AF Android] Maybe omit new dropdown in landscape mode
When the new dropdown is shown in landscape mode, most vertical space
is occupied with the on-screen keyboard and the sticky footer options,
leaving not enough space to actually show the suggestions.
This CL adds a quick fix that prevents the most common cases by
allowing the dropdown to only be shown in landscape mode when we are
sure that we have enough vertical space (e.g. tablets).
This will allow us to at least to experiment on the new dropdown in
M72 and collect metrics to understand the size of the affected
population and make an informed decision on how we should address
this issue.
Moving forward, the plan to properly fix this is:
- [short term] Make the footer non-sticky;
- [long term] Move away from the dropdown and show suggestions in the
keyboard accessory.
A follow-up CL will add a new bucket to Autofill.FormEvents to track
how often that happens.
Bug: 905081
Change-Id: Ia8f8b14f987e13e8d9f0afc021d156fbf4238af6
Reviewed-on: https://chromium-review.googlesource.com/c/1346734
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613410}diff --git a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
index b5b3e9b..3425c94c 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java
@@ -7,7 +7,7 @@
import android.app.Activity;
import android.content.Context;
import android.content.DialogInterface;
-import android.os.Handler;
+import android.content.res.Configuration;
import android.support.v7.app.AlertDialog;
import android.view.View;
@@ -39,12 +39,9 @@
WindowAndroid windowAndroid) {
mNativeAutofillPopup = nativeAutofillPopupViewAndroid;
Activity activity = windowAndroid.getActivity().get();
- if (activity == null) {
+ if (activity == null || notEnoughScreenSpace(activity)) {
mAutofillPopup = null;
mContext = null;
- // Clean up the native counterpart. This is posted to allow the native counterpart
- // to fully finish the construction of this glue object before we attempt to delete it.
- new Handler().post(() -> dismissed());
} else {
mAutofillPopup = new AutofillPopup(activity, anchorView, this);
mContext = activity;
@@ -121,10 +118,29 @@
mDeletionDialog.show();
}
+ @CalledByNative
+ private boolean wasSuppressed() {
+ return mAutofillPopup == null;
+ }
+
private static boolean shouldUseRefreshStyle() {
return ChromeFeatureList.isEnabled(ChromeFeatureList.AUTOFILL_REFRESH_STYLE_ANDROID);
}
+ private static boolean notEnoughScreenSpace(Context context) {
+ Configuration config = context.getResources().getConfiguration();
+ // In landscape mode, most vertical space is used by the on-screen keyboard. When refresh
+ // style is used, the footer is sticky, so there is not much space to even show the first
+ // suggestion. In those cases, the dropdown should only be shown on very large screen
+ // devices, such as tablets.
+ //
+ // TODO(crbug.com/907634): This is a simple first approach to not provide a degraded
+ // experience. Explore other alternatives when this happens such as
+ // showing suggestions on the keyboard accessory.
+ return shouldUseRefreshStyle() && config.orientation == Configuration.ORIENTATION_LANDSCAPE
+ && !config.isLayoutSizeAtLeast(Configuration.SCREENLAYOUT_SIZE_XLARGE);
+ }
+
// Helper methods for AutofillSuggestion
@CalledByNative
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
index 9593417..349097d8 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillPopupTest.java
@@ -4,17 +4,24 @@
package org.chromium.chrome.browser.autofill;
+import android.app.Activity;
+import android.content.pm.ActivityInfo;
+import android.content.res.Configuration;
+import android.support.annotation.Nullable;
+import android.support.test.InstrumentationRegistry;
import android.support.test.filters.MediumTest;
import android.text.TextUtils;
import android.view.View;
import android.view.ViewGroup;
+import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.chromium.base.Callback;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Feature;
import org.chromium.base.test.util.RetryOnFailure;
@@ -26,6 +33,7 @@
import org.chromium.chrome.test.ChromeActivityTestRule;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.chrome.test.util.browser.Features;
+import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.content_public.browser.ImeAdapter;
import org.chromium.content_public.browser.WebContents;
import org.chromium.content_public.browser.test.util.Criteria;
@@ -148,20 +156,31 @@
Features.getInstance().enable(ChromeFeatureList.AUTOFILL_ALLOW_NON_HTTP_ACTIVATION);
}
- private void loadAndFillForm(final String formDataUrl, final String inputText)
+ @After
+ public void tearDown() throws Exception {
+ mActivityTestRule.getActivity().setRequestedOrientation(
+ ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED);
+ InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+ }
+
+ private void loadForm(final String formDataUrl, final String inputText,
+ @Nullable Callback<Activity> updateActivity)
throws InterruptedException, ExecutionException, TimeoutException {
mActivityTestRule.startMainActivityWithURL(formDataUrl);
- mHelper = new AutofillTestHelper();
+ if (updateActivity != null) {
+ updateActivity.onResult(mActivityTestRule.getActivity());
+ InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+ }
// The TestInputMethodManagerWrapper intercepts showSoftInput so that a keyboard is never
// brought up.
final WebContents webContents = mActivityTestRule.getActivity().getCurrentWebContents();
- final ViewGroup view = webContents.getViewAndroidDelegate().getContainerView();
final ImeAdapter imeAdapter = WebContentsUtils.getImeAdapter(webContents);
TestInputMethodManagerWrapper immw = TestInputMethodManagerWrapper.create(imeAdapter);
imeAdapter.setInputMethodManagerWrapper(immw);
// Add an Autofill profile.
+ mHelper = new AutofillTestHelper();
AutofillProfile profile = new AutofillProfile(
"" /* guid */, ORIGIN, FIRST_NAME + " " + LAST_NAME, COMPANY_NAME,
STREET_ADDRESS_TEXTAREA,
@@ -179,6 +198,16 @@
imeAdapter.setComposingTextForTest(inputText, 1);
+ InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+ }
+
+ private void loadAndFillForm(final String formDataUrl, final String inputText,
+ @Nullable Callback<Activity> updateActivity)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ loadForm(formDataUrl, inputText, updateActivity);
+
+ final WebContents webContents = mActivityTestRule.getActivity().getCurrentWebContents();
+ final ViewGroup view = webContents.getViewAndroidDelegate().getContainerView();
waitForAnchorViewAdd(view);
View anchorView = view.findViewById(R.id.dropdown_popup_window);
@@ -186,11 +215,16 @@
final DropdownPopupWindowInterface popup =
(DropdownPopupWindowInterface) anchorView.getTag();
- waitForAutofillPopopShow(popup);
+ waitForAutofillPopupShow(popup);
TouchCommon.singleClickView(popup.getListView(), 10, 10);
- waitForInputFieldFill(webContents);
+ waitForInputFieldFill();
+ }
+
+ private void loadAndFillForm(final String formDataUrl, final String inputText)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ loadAndFillForm(formDataUrl, inputText, null);
}
/**
@@ -299,6 +333,52 @@
// Country will not be logged since "US" is not a valid <option>.
}
+ @Test
+ @MediumTest
+ @Feature({"autofill"})
+ @EnableFeatures(ChromeFeatureList.AUTOFILL_REFRESH_STYLE_ANDROID)
+ public void testScreenOrientationPortrait()
+ throws InterruptedException, ExecutionException, TimeoutException {
+ runTestScreenOrientation(ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
+ }
+
+ @Test
+ @MediumTest
+ @Feature({"autofill"})
+ @EnableFeatures(ChromeFeatureList.AUTOFILL_REFRESH_STYLE_ANDROID)
+ public void testScreenOrientationLandscape()
+ throws InterruptedException, ExecutionException, TimeoutException {
+ runTestScreenOrientation(ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE);
+ }
+
+ private void runTestScreenOrientation(int orientation)
+ throws InterruptedException, ExecutionException, TimeoutException {
+ // TODO(crbug.com/905081): Also test different screen sizes.
+ loadForm(BASIC_PAGE_DATA, "J", activity -> activity.setRequestedOrientation(orientation));
+
+ ChromeActivity activity = mActivityTestRule.getActivity();
+ final WebContents webContents = activity.getCurrentWebContents();
+ final Configuration config = activity.getResources().getConfiguration();
+ final boolean shouldShowPopup = config.orientation == Configuration.ORIENTATION_PORTRAIT
+ || config.isLayoutSizeAtLeast(Configuration.SCREENLAYOUT_SIZE_XLARGE);
+ final ViewGroup view = webContents.getViewAndroidDelegate().getContainerView();
+ if (shouldShowPopup) {
+ waitForAnchorViewAdd(view);
+ } else {
+ InstrumentationRegistry.getInstrumentation().waitForIdleSync();
+ }
+ final View popup = view.findViewById(R.id.dropdown_popup_window);
+
+ final String message = "Mismatched dropdown_popup_window for orientation: "
+ + (orientation == ActivityInfo.SCREEN_ORIENTATION_LANDSCAPE ? "landscape"
+ : "portrait");
+ if (shouldShowPopup) {
+ Assert.assertNotNull(message, popup);
+ } else {
+ Assert.assertNull(message, popup);
+ }
+ }
+
// Wait and assert helper methods -------------------------------------------------------------
private void waitForKeyboardShowRequest(final TestInputMethodManagerWrapper immw,
@@ -317,7 +397,7 @@
});
}
- private void waitForAutofillPopopShow(final DropdownPopupWindowInterface popup) {
+ private void waitForAutofillPopupShow(final DropdownPopupWindowInterface popup) {
CriteriaHelper.pollUiThread(
new Criteria("Autofill Popup anchor view was never added.") {
@Override
@@ -329,14 +409,16 @@
});
}
- private void waitForInputFieldFill(final WebContents webContents) {
+ private void waitForInputFieldFill() {
CriteriaHelper.pollInstrumentationThread(
new Criteria("First name field was never filled.") {
@Override
public boolean isSatisfied() {
try {
return TextUtils.equals(FIRST_NAME,
- DOMUtils.getNodeValue(webContents, "fn"));
+ DOMUtils.getNodeValue(
+ mActivityTestRule.getActivity().getCurrentWebContents(),
+ "fn"));
} catch (InterruptedException e) {
return false;
} catch (TimeoutException e) {
diff --git a/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc b/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
index 4d33af44..bb2be610 100644
--- a/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
+++ b/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
@@ -4,6 +4,8 @@
#include "chrome/browser/ui/android/autofill/autofill_popup_view_android.h"
+#include <memory>
+
#include "base/android/jni_android.h"
#include "base/android/jni_string.h"
#include "base/command_line.h"
@@ -35,19 +37,6 @@
AutofillPopupViewAndroid::~AutofillPopupViewAndroid() {}
void AutofillPopupViewAndroid::Show() {
- JNIEnv* env = base::android::AttachCurrentThread();
- ui::ViewAndroid* view_android = controller_->container_view();
-
- DCHECK(view_android);
- popup_view_ = view_android->AcquireAnchorView();
- const ScopedJavaLocalRef<jobject> view = popup_view_.view();
- if (view.is_null())
- return;
-
- java_object_.Reset(Java_AutofillPopupBridge_create(
- env, view, reinterpret_cast<intptr_t>(this),
- view_android->GetWindowAndroid()->GetJavaObject()));
-
OnSuggestionsChanged();
}
@@ -164,15 +153,39 @@
delete this;
}
+void AutofillPopupViewAndroid::Init() {
+ JNIEnv* env = base::android::AttachCurrentThread();
+ ui::ViewAndroid* view_android = controller_->container_view();
+
+ DCHECK(view_android);
+ popup_view_ = view_android->AcquireAnchorView();
+ const ScopedJavaLocalRef<jobject> view = popup_view_.view();
+ if (view.is_null())
+ return;
+
+ java_object_.Reset(Java_AutofillPopupBridge_create(
+ env, view, reinterpret_cast<intptr_t>(this),
+ view_android->GetWindowAndroid()->GetJavaObject()));
+}
+
+bool AutofillPopupViewAndroid::WasSuppressed() {
+ return java_object_ &&
+ Java_AutofillPopupBridge_wasSuppressed(
+ base::android::AttachCurrentThread(), java_object_);
+}
+
// static
AutofillPopupView* AutofillPopupView::Create(
AutofillPopupController* controller) {
- if (IsKeyboardAccessoryEnabled())
+ if (IsKeyboardAccessoryEnabled()) {
return new AutofillKeyboardAccessoryView(
controller, GetKeyboardAccessoryAnimationDuration(),
ShouldLimitKeyboardAccessorySuggestionLabelWidth());
+ }
- return new AutofillPopupViewAndroid(controller);
+ auto popup_view = std::make_unique<AutofillPopupViewAndroid>(controller);
+ popup_view->Init();
+ return popup_view->WasSuppressed() ? nullptr : popup_view.release();
}
} // namespace autofill
diff --git a/chrome/browser/ui/android/autofill/autofill_popup_view_android.h b/chrome/browser/ui/android/autofill/autofill_popup_view_android.h
index 21a2c164..5028762 100644
--- a/chrome/browser/ui/android/autofill/autofill_popup_view_android.h
+++ b/chrome/browser/ui/android/autofill/autofill_popup_view_android.h
@@ -21,6 +21,7 @@
class AutofillPopupViewAndroid : public AutofillPopupView {
public:
explicit AutofillPopupViewAndroid(AutofillPopupController* controller);
+ ~AutofillPopupViewAndroid() override;
// --------------------------------------------------------------------------
// Methods called from Java via JNI
@@ -49,7 +50,12 @@
void OnSuggestionsChanged() override;
private:
- ~AutofillPopupViewAndroid() override;
+ friend class AutofillPopupView;
+ // Creates the AutofillPopupBridge Java object.
+ void Init();
+ // Returns whether the dropdown was suppressed (mainly due to not enough
+ // screen space available).
+ bool WasSuppressed();
AutofillPopupController* controller_; // weak.
