Skip to content

Commit

Permalink
[Android] Fix popup bounds to honor top and bottom controls
Browse files Browse the repository at this point in the history
(Copy and continuation of https://crrev.com/c/4101981)

This CL makes the anchored popup view check the webcontents rect
if one is available.
This prevents overlay of the keyboard or the Omnibox as shown in
https://crbug.com/856040#c9 and reacts to live changes of the
available space (e.g. after rotation or accessory animation).

Additionally, it ensures a minimal Popup size of 50x50dip which
prevents the popup to show up and be selectable when contents are
not readable.

Known issues:
- Fairly large for a potential merge.
- If web contents scroll, the popup disappears; seems pre-existing.

Images and video in https://crbug.com/856040#c10 and #c9

Fixed: 856040, 1274887, 1398579
Change-Id: I0f4f2f3091392dddfb309d4abd405ad8df49cd40
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4123739
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1095024}
  • Loading branch information
FHorschig authored and Chromium LUCI CQ committed Jan 20, 2023
1 parent a32ab6f commit ecb6917
Show file tree
Hide file tree
Showing 12 changed files with 349 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void deleteSuggestion(int listIndex) {}

@Override
public void accessibilityFocusCleared() {}
});
}, null);
} catch (RuntimeException e) {
// Deliberately swallowing exception because bad fraemwork implementation can
// throw exceptions in ListPopupWindow constructor.
Expand Down
1 change: 1 addition & 0 deletions chrome/android/chrome_java_sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ chrome_java_sources = [
"java/src/org/chromium/chrome/browser/autofill/SaveUpdateAddressProfilePromptController.java",
"java/src/org/chromium/chrome/browser/autofill/VirtualCardEnrollmentDelegate.java",
"java/src/org/chromium/chrome/browser/autofill/VirtualCardEnrollmentDialogViewBridge.java",
"java/src/org/chromium/chrome/browser/autofill/WebContentsViewRectProvider.java",
"java/src/org/chromium/chrome/browser/autofill/prefeditor/DropdownFieldAdapter.java",
"java/src/org/chromium/chrome/browser/autofill/prefeditor/EditorBase.java",
"java/src/org/chromium/chrome/browser/autofill/prefeditor/EditorDialog.java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@
import android.view.View;

import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.appcompat.app.AlertDialog;

import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.JNINamespace;
import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.supplier.Supplier;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.fullscreen.BrowserControlsManagerSupplier;
import org.chromium.chrome.browser.keyboard_accessory.ManualFillingComponent;
import org.chromium.chrome.browser.keyboard_accessory.ManualFillingComponentSupplier;
import org.chromium.chrome.browser.tab.Tab;
Expand All @@ -29,6 +31,7 @@
import org.chromium.content_public.browser.WebContents;
import org.chromium.content_public.browser.WebContentsAccessibility;
import org.chromium.ui.DropdownItem;
import org.chromium.ui.base.ViewAndroidDelegate;
import org.chromium.ui.base.WindowAndroid;
import org.chromium.url.GURL;

Expand All @@ -41,7 +44,8 @@ public class AutofillPopupBridge implements AutofillDelegate, DialogInterface.On
private final AutofillPopup mAutofillPopup;
private AlertDialog mDeletionDialog;
private final Context mContext;
private WebContentsAccessibility mWebContentsAccessibility;
private final WebContentsAccessibility mWebContentsAccessibility;
private final WebContentsViewRectProvider mWebContentsViewRectProvider;

public AutofillPopupBridge(@NonNull View anchorView, long nativeAutofillPopupViewAndroid,
@NonNull WindowAndroid windowAndroid) {
Expand All @@ -56,13 +60,15 @@ public AutofillPopupBridge(@NonNull View anchorView, long nativeAutofillPopupVie
if (activity == null || notEnoughScreenSpace(activity) || webContents == null) {
mAutofillPopup = null;
mContext = null;
mWebContentsViewRectProvider = null;
mWebContentsAccessibility = null;
} else {
mAutofillPopup = new AutofillPopup(activity, anchorView, this);
mContext = activity;

Supplier<ManualFillingComponent> manualFillingComponentSupplier =
ObservableSupplier<ManualFillingComponent> manualFillingComponentSupplier =
ManualFillingComponentSupplier.from(windowAndroid);
// Could be null if this ctor is called as the activity is being destroyed.
mWebContentsViewRectProvider = tryCreateRectProvider(webContents, windowAndroid);
mAutofillPopup =
new AutofillPopup(activity, anchorView, this, mWebContentsViewRectProvider);
if (manualFillingComponentSupplier != null
&& manualFillingComponentSupplier.hasValue()) {
manualFillingComponentSupplier.get().notifyPopupAvailable(mAutofillPopup);
Expand Down Expand Up @@ -119,6 +125,7 @@ private void dismiss() {
mNativeAutofillPopup = 0;
if (mAutofillPopup != null) mAutofillPopup.dismiss();
if (mDeletionDialog != null) mDeletionDialog.dismiss();
if (mWebContentsViewRectProvider != null) mWebContentsViewRectProvider.dismiss();
mWebContentsAccessibility.onAutofillPopupDismissed();
}

Expand Down Expand Up @@ -226,6 +233,16 @@ private void addToAutofillSuggestionArray(AutofillSuggestion[] array, int index,
array[index] = builder.build();
}

private @Nullable WebContentsViewRectProvider tryCreateRectProvider(
WebContents webContents, WindowAndroid windowAndroid) {
ObservableSupplier<ManualFillingComponent> manualFillingComponentSupplier =
ManualFillingComponentSupplier.from(windowAndroid);
ViewAndroidDelegate viewDelegate = webContents.getViewAndroidDelegate();
if (viewDelegate == null || viewDelegate.getContainerView() == null) return null;
return new WebContentsViewRectProvider(webContents,
BrowserControlsManagerSupplier.from(windowAndroid), manualFillingComponentSupplier);
}

@NativeMethods
interface Natives {
void suggestionSelected(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.autofill;

import android.graphics.Rect;

import androidx.annotation.Nullable;

import org.chromium.base.Callback;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.Supplier;
import org.chromium.chrome.browser.fullscreen.BrowserControlsManager;
import org.chromium.chrome.browser.keyboard_accessory.ManualFillingComponent;
import org.chromium.content_public.browser.WebContents;
import org.chromium.ui.base.ViewAndroidDelegate;
import org.chromium.ui.widget.RectProvider;

/**
* Helper class that computes the visible rect of a given {@link WebContents} objects. It observes
* the given {@link WebContents} object to update the observed suppliers of browser controls and
* filling component. Use like a standard {@link RectProvider}.
*
* Examples for observed changes:
* - {@link BrowserControlsManager} providing height for elements like the Omnibox.
* - {@link ManualFillingComponent} providing filling surface height like Keyboard Accessory.
* - A Bottom Inset supplier for the Soft-keyboard.
*/
class WebContentsViewRectProvider extends RectProvider {
private final WebContents mWebContents;
private ObservableSupplier<BrowserControlsManager> mBrowserControlsSupplier;
private ObservableSupplier<ManualFillingComponent> mManualFillingComponentSupplier;
private ObservableSupplier<Integer> mBottomInsetSupplier;

private final Callback<ManualFillingComponent> mOnManualFillingComponentChanged =
fillComponent -> observeBottomInsetSupplier(fillComponent.getBottomInsetSupplier());
private final Callback<Integer> mOnBottomInsetChanged = bottomInset
-> updateVisibleRectForPopup(bottomInset, getValueOrNull(mBrowserControlsSupplier));
private final Callback<BrowserControlsManager> mOnBrowserControlsChanged =
ctrlMgr -> updateVisibleRectForPopup(getValueOrNull(mBottomInsetSupplier), ctrlMgr);

/**
* Creates a new RectProvider and starts observing given parameters. If they provide a valid
* rect, it's immediately computed.
*
* @param webContents A required, non-null {@link WebContents} object.
* @param browserControlsSupplier A {@link ObservableSupplier<BrowserControlsManager>}.
* @param manualFillingComponentSupplier A {@link ObservableSupplier<ManualFillingComponent>}.
*/
public WebContentsViewRectProvider(WebContents webContents,
ObservableSupplier<BrowserControlsManager> browserControlsSupplier,
ObservableSupplier<ManualFillingComponent> manualFillingComponentSupplier) {
assert webContents != null;
assert webContents.getViewAndroidDelegate() != null;
assert webContents.getViewAndroidDelegate().getContainerView() != null;

mWebContents = webContents;
observeManualFillingComponentSupplier(manualFillingComponentSupplier);
observeBrowserControlsSupplier(browserControlsSupplier);
}

/**
* Stops observing the suppliers given in the constructor.
*/
public void dismiss() {
observeManualFillingComponentSupplier(null);
observeBrowserControlsSupplier(null);
}

private void observeBrowserControlsSupplier(
@Nullable ObservableSupplier<BrowserControlsManager> supplier) {
if (mBrowserControlsSupplier != null) {
mBrowserControlsSupplier.removeObserver(mOnBrowserControlsChanged);
}
mBrowserControlsSupplier = supplier;
if (mBrowserControlsSupplier != null) {
mBrowserControlsSupplier.addObserver(mOnBrowserControlsChanged);
}
updateVisibleRectForPopup(
getValueOrNull(mBottomInsetSupplier), getValueOrNull(mBrowserControlsSupplier));
}

private void observeManualFillingComponentSupplier(
@Nullable ObservableSupplier<ManualFillingComponent> supplier) {
if (mManualFillingComponentSupplier != null) {
observeBottomInsetSupplier(null);
mManualFillingComponentSupplier.removeObserver(mOnManualFillingComponentChanged);
}
mManualFillingComponentSupplier = supplier;
if (mManualFillingComponentSupplier != null) {
mManualFillingComponentSupplier.addObserver(mOnManualFillingComponentChanged);
observeBottomInsetSupplier(mManualFillingComponentSupplier.hasValue()
? mManualFillingComponentSupplier.get().getBottomInsetSupplier()
: null);
}
}

private void observeBottomInsetSupplier(@Nullable ObservableSupplier<Integer> supplier) {
if (mBottomInsetSupplier != null) {
mBottomInsetSupplier.removeObserver(mOnBottomInsetChanged);
}
mBottomInsetSupplier = supplier;
if (mBottomInsetSupplier != null) {
mBottomInsetSupplier.addObserver(mOnBottomInsetChanged);
}
updateVisibleRectForPopup(
getValueOrNull(mBottomInsetSupplier), getValueOrNull(mBrowserControlsSupplier));
}

private void updateVisibleRectForPopup(
@Nullable Integer bottomInset, @Nullable BrowserControlsManager browserControls) {
ViewAndroidDelegate viewDelegate = mWebContents.getViewAndroidDelegate();

// Stop computing a rect if the WebContents view is gone for some reason.
if (viewDelegate == null || viewDelegate.getContainerView() == null) return;

Rect rect = new Rect();
viewDelegate.getContainerView().getGlobalVisibleRect(rect);
if (browserControls != null) {
rect.top += browserControls.getTopControlsHeight();
rect.bottom -= browserControls.getBottomControlOffset();
}
if (bottomInset != null) rect.bottom -= bottomInset;

if (!mRect.equals(rect)) setRect(rect); // Update and notify only if the rect changes.
}

private static @Nullable<T> T getValueOrNull(@Nullable Supplier<T> supplier) {
return supplier != null ? supplier.get() : null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public void setUp() throws Exception {
viewDelegate.setViewPosition(anchorView, 50f, 500f, 500f, 500f, 10, 10);

mAutofillPopup = new AutofillPopup(
sActivityTestRule.getActivity(), anchorView, mMockAutofillCallback);
sActivityTestRule.getActivity(), anchorView, mMockAutofillCallback, null);
mAutofillPopup.filterAndShow(
new AutofillSuggestion[0], /* isRtl= */ false, /* isRefresh= */ false);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ public void deleteSuggestion(int listIndex) {}
public void accessibilityFocusCleared() {
mWebContentsAccessibility.onAutofillPopupAccessibilityFocusCleared();
}
});
}, null);
} catch (RuntimeException e) {
// Deliberately swallowing exception because bad framework implementation can
// throw exceptions in ListPopupWindow constructor.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@
import android.widget.AdapterView;
import android.widget.PopupWindow;

import androidx.annotation.Nullable;

import org.chromium.ui.DropdownItem;
import org.chromium.ui.DropdownPopupWindow;
import org.chromium.ui.widget.RectProvider;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -49,12 +52,15 @@ public void run() {

/**
* Creates an AutofillWindow with specified parameters.
*
* @param context Application context.
* @param anchorView View anchored for popup.
* @param autofillDelegate An object that handles the calls to the native AutofillPopupView.
* @param visibleWebContentsRectProvider The {@link RectProvider} for popup limits.
*/
public AutofillPopup(Context context, View anchorView, AutofillDelegate autofillDelegate) {
super(context, anchorView);
public AutofillPopup(Context context, View anchorView, AutofillDelegate autofillDelegate,
@Nullable RectProvider visibleWebContentsRectProvider) {
super(context, anchorView, visibleWebContentsRectProvider);
mContext = context;
mAutofillDelegate = autofillDelegate;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import android.view.Gravity;
import android.view.MotionEvent;
import android.view.View;
import android.view.ViewGroup;
import android.view.WindowManager;
import android.widget.FrameLayout;
import android.widget.PopupWindow;
Expand Down Expand Up @@ -46,6 +47,8 @@
@Config(manifest = Config.NONE, shadows = {ShadowPhoneWindow.class})
@LooperMode(LooperMode.Mode.LEGACY)
public class ContextMenuDialogUnitTest {
private static final int DIALOG_SIZE_DIP = 50;

@Rule
public MockitoRule mockitoRule = MockitoJUnit.rule();

Expand Down Expand Up @@ -82,6 +85,11 @@ public void setup() {
.when(mSpyPopupWindow)
.showAtLocation(any(View.class), anyInt(), anyInt(), anyInt());
Mockito.doNothing().when(mSpyPopupWindow).dismiss();

View mockContentView = Mockito.mock(ViewGroup.class);
Mockito.when(mockContentView.getMeasuredHeight()).thenReturn(DIALOG_SIZE_DIP);
Mockito.when(mockContentView.getMeasuredWidth()).thenReturn(DIALOG_SIZE_DIP);
Mockito.doReturn(mockContentView).when(mSpyPopupWindow).getContentView();
}

@Test
Expand Down
16 changes: 14 additions & 2 deletions ui/android/java/src/org/chromium/ui/DropdownPopupWindow.java
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,32 @@
import android.widget.ListView;
import android.widget.PopupWindow;

import androidx.annotation.Nullable;

import org.chromium.ui.widget.AnchoredPopupWindow;
import org.chromium.ui.widget.RectProvider;

// TODO(https://crbug.com/1400723): This class is a noop now, so we should remove it.
/**
* The dropdown popup window that decides what widget should be used for the popup.
*/
public class DropdownPopupWindow {
private DropdownPopupWindowInterface mPopup;

public DropdownPopupWindow(Context context, View anchorView) {
this(context, anchorView, null);
}

/**
* Creates an DropdownPopupWindow with specified parameters.
* @param context Application context.
* @param anchorView Popup view to be anchored.
* @param visibleWebContentsRectProvider The {@link RectProvider} which will be used for {@link
* AnchoredPopupWindow}.
*/
public DropdownPopupWindow(Context context, View anchorView) {
mPopup = new DropdownPopupWindowImpl(context, anchorView);
public DropdownPopupWindow(Context context, View anchorView,
@Nullable RectProvider visibleWebContentsRectProvider) {
mPopup = new DropdownPopupWindowImpl(context, anchorView, visibleWebContentsRectProvider);
}

/**
Expand Down
15 changes: 12 additions & 3 deletions ui/android/java/src/org/chromium/ui/DropdownPopupWindowImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
import android.widget.ListView;
import android.widget.PopupWindow;

import androidx.annotation.Nullable;
import androidx.appcompat.content.res.AppCompatResources;

import org.chromium.ui.widget.AnchoredPopupWindow;
import org.chromium.ui.widget.RectProvider;
import org.chromium.ui.widget.ViewRectProvider;

/**
Expand All @@ -46,12 +48,19 @@ class DropdownPopupWindowImpl
private Drawable mBackground;
private int mHorizontalPadding;

public DropdownPopupWindowImpl(Context context, View anchorView) {
this(context, anchorView, null);
}

/**
* Creates an DropdownPopupWindowImpl with specified parameters.
* @param context Application context.
* @param anchorView Popup view to be anchored.
* @param visibleWebContentsRectProvider The {@link RectProvider} which will be used for {@link
* AnchoredPopupWindow}.
*/
public DropdownPopupWindowImpl(Context context, View anchorView) {
public DropdownPopupWindowImpl(Context context, View anchorView,
@Nullable RectProvider visibleWebContentsRectProvider) {
mContext = context;
mAnchorView = anchorView;

Expand Down Expand Up @@ -84,8 +93,8 @@ public void onDismiss() {
ViewRectProvider rectProvider = new ViewRectProvider(mAnchorView);
rectProvider.setIncludePadding(true);
mBackground = AppCompatResources.getDrawable(context, R.drawable.menu_bg_baseline);
mAnchoredPopupWindow = new AnchoredPopupWindow(
context, mAnchorView, mBackground, mContentView, rectProvider);
mAnchoredPopupWindow = new AnchoredPopupWindow(context, mAnchorView, mBackground,
mContentView, rectProvider, visibleWebContentsRectProvider);
mAnchoredPopupWindow.addOnDismissListener(onDismissLitener);
mAnchoredPopupWindow.setLayoutObserver(this);
mAnchoredPopupWindow.setElevation(
Expand Down

0 comments on commit ecb6917

Please sign in to comment.