[WebLayer] Never expand browser controls if onlyExpandTopControlsAtPageTop is enabled

This CL prevents the top view from being force shown unless the renderer
crashes or us unresponsive.

Bug: 1127076
Change-Id: I0092a473726376c11181863d3041aa60de15eb1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419384
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809622}

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/BrowserControlsTest.java

@@ -4,6 +4,11 @@
 
 package org.chromium.weblayer.test;
 
+import static androidx.test.espresso.Espresso.onView;
+import static androidx.test.espresso.assertion.ViewAssertions.matches;
+import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed;
+import static androidx.test.espresso.matcher.ViewMatchers.withText;
+
 import android.os.Build;
 import android.os.RemoteException;
 import android.view.View;
@@ -320,6 +325,41 @@ public class BrowserControlsTest {
         });
     }
 
+    // Makes sure that the top controls are not shown when a js dialog is shown when
+    // onlyExpandTopControlsAtPageTop is true and the page is scrolled down.
+    //
+    // Disabled on L bots due to unexplained flakes. See crbug.com/1035894.
+    @MinAndroidSdkLevel(Build.VERSION_CODES.M)
+    @Test
+    @SmallTest
+    public void testAlertDoesntShowTopControlsIfOnlyExpandTopControlsAtPageTop() throws Exception {
+        InstrumentationActivity activity = mActivityTestRule.getActivity();
+        View topContents = activity.getTopContentsContainer();
+        TestThreadUtils.runOnUiThreadBlocking(
+                ()
+                        -> activity.getBrowser().setTopView(
+                                topContents, 0, /*onlyExpandControlsAtPageTop=*/true, false));
+
+        // Scroll past the top-controls and wait until they're not visible.
+        EventUtils.simulateDragFromCenterOfView(
+                activity.getWindow().getDecorView(), 0, -2 * mTopViewHeight);
+        CriteriaHelper.pollUiThread(() -> {
+            Criteria.checkThat(activity.getTopContentsContainer().getVisibility(),
+                    Matchers.is(View.INVISIBLE));
+        });
+
+        // Trigger an alert dialog.
+        mActivityTestRule.executeScriptSync(
+                "window.setTimeout(function() { alert('alert dialog'); }, 1);", false);
+        onView(withText("alert dialog")).check(matches(isDisplayed()));
+
+        // Top controls shouldn't be shown.
+        CriteriaHelper.pollUiThread(() -> {
+            Criteria.checkThat(activity.getTopContentsContainer().getVisibility(),
+                    Matchers.is(View.INVISIBLE));
+        });
+    }
+
     /**
      * Makes sure that the top controls are shown when a js dialog is shown.
      *

weblayer/browser/browser_controls_navigation_state_handler.cc

@@ -18,6 +18,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "weblayer/browser/browser_controls_navigation_state_handler_delegate.h"
+#include "weblayer/browser/controls_visibility_reason.h"
 #include "weblayer/browser/weblayer_features.h"
 
 namespace weblayer {
@@ -101,14 +102,11 @@ void BrowserControlsNavigationStateHandler::RenderProcessGone(
     base::TerminationStatus status) {
   is_crashed_ = true;
   UpdateState();
-  delegate_->OnForceBrowserControlsShown();
 }
 
 void BrowserControlsNavigationStateHandler::OnRendererUnresponsive(
     content::RenderProcessHost* render_process_host) {
   UpdateState();
-  if (IsRendererHungOrCrashed())
-    delegate_->OnForceBrowserControlsShown();
 }
 
 void BrowserControlsNavigationStateHandler::OnRendererResponsive(
@@ -133,24 +131,41 @@ void BrowserControlsNavigationStateHandler::ScheduleStopDelayedForceShow() {
 }
 
 void BrowserControlsNavigationStateHandler::UpdateState() {
-  const content::BrowserControlsState current_state = CalculateCurrentState();
-  if (current_state == last_state_)
-    return;
-  last_state_ = current_state;
-  delegate_->OnBrowserControlsStateStateChanged(*last_state_);
+  const content::BrowserControlsState renderer_availability_state =
+      CalculateStateForReasonRendererAvailability();
+  if (renderer_availability_state != last_renderer_availability_state_) {
+    last_renderer_availability_state_ = renderer_availability_state;
+    delegate_->OnBrowserControlsStateStateChanged(
+        ControlsVisibilityReason::kRendererUnavailable,
+        last_renderer_availability_state_);
+  }
+
+  const content::BrowserControlsState other_state =
+      CalculateStateForReasonOther();
+  if (other_state != last_other_state_) {
+    last_other_state_ = other_state;
+    delegate_->OnBrowserControlsStateStateChanged(
+        ControlsVisibilityReason::kOther, last_other_state_);
+  }
+}
+
+content::BrowserControlsState BrowserControlsNavigationStateHandler::
+    CalculateStateForReasonRendererAvailability() {
+  if (!IsRendererControllingOffsets() || web_contents()->IsBeingDestroyed() ||
+      web_contents()->IsCrashed()) {
+    return content::BROWSER_CONTROLS_STATE_SHOWN;
+  }
+
+  return content::BROWSER_CONTROLS_STATE_BOTH;
 }
 
 content::BrowserControlsState
-BrowserControlsNavigationStateHandler::CalculateCurrentState() {
+BrowserControlsNavigationStateHandler::CalculateStateForReasonOther() {
   // TODO(sky): this needs to force SHOWN if a11y enabled, see
   // AccessibilityUtil.isAccessibilityEnabled().
 
-  if (!IsRendererControllingOffsets())
-    return content::BROWSER_CONTROLS_STATE_SHOWN;
-
   if (force_show_during_load_ || web_contents()->IsFullscreen() ||
-      web_contents()->IsFocusedElementEditable() ||
-      web_contents()->IsBeingDestroyed() || web_contents()->IsCrashed()) {
+      web_contents()->IsFocusedElementEditable()) {
     return content::BROWSER_CONTROLS_STATE_SHOWN;
   }
 

weblayer/browser/browser_controls_navigation_state_handler.h

@@ -65,8 +65,12 @@ class BrowserControlsNavigationStateHandler
   // Checks the current state, and if it has changed notifies the delegate.
   void UpdateState();
 
-  // Calcultes the current browser controls state.
-  content::BrowserControlsState CalculateCurrentState();
+  // Calculates whether the renderer is available to control the browser
+  // controls.
+  content::BrowserControlsState CalculateStateForReasonRendererAvailability();
+
+  // Calculates the value of the ControlsVisibilityReason::kOther state.
+  content::BrowserControlsState CalculateStateForReasonOther();
 
   bool IsRendererHungOrCrashed();
 
@@ -79,8 +83,11 @@ class BrowserControlsNavigationStateHandler
   // Timer used to set |force_show_during_load_| to false.
   base::OneShotTimer forced_show_during_load_timer_;
 
-  // Last value supplied to the delegate.
-  base::Optional<content::BrowserControlsState> last_state_;
+  // Last values supplied to the delegate.
+  content::BrowserControlsState last_renderer_availability_state_ =
+      content::BROWSER_CONTROLS_STATE_BOTH;
+  content::BrowserControlsState last_other_state_ =
+      content::BROWSER_CONTROLS_STATE_BOTH;
 
   // This is cached as WebContents::IsCrashed() does not always return the
   // right thing.

weblayer/browser/browser_controls_navigation_state_handler_delegate.h

@@ -9,11 +9,14 @@
 
 namespace weblayer {
 
+enum class ControlsVisibilityReason;
+
 // Called to propagate changse to BrowserControlsState.
 class BrowserControlsNavigationStateHandlerDelegate {
  public:
   // Called when the state changes.
   virtual void OnBrowserControlsStateStateChanged(
+      ControlsVisibilityReason reason,
       content::BrowserControlsState state) = 0;
 
   // Called when UpdateBrowserControlsState() should be called because a new
@@ -22,11 +25,6 @@ class BrowserControlsNavigationStateHandlerDelegate {
   virtual void OnUpdateBrowserControlsStateBecauseOfProcessSwitch(
       bool did_commit) = 0;
 
-  // Called if the browser controls need to be shown and the renderer is in a
-  // hung/crashed state. This needs to be handled specially as the renderer
-  // normally drives the offsets, but in this situation it won't.
-  virtual void OnForceBrowserControlsShown() = 0;
-
  protected:
   virtual ~BrowserControlsNavigationStateHandlerDelegate() = default;
 };

weblayer/browser/controls_visibility_reason.h

@@ -16,9 +16,6 @@ enum class ControlsVisibilityReason {
   // Browser controls are hidden when fullscreen is active.
   kFullscreen = 0,
 
-  // Browser controls are always shown for a few seconds after a navigation.
-  kPostNavigation,
-
   // Find in page forces browser controls to be visible.
   kFindInPage,
 
@@ -32,6 +29,17 @@ enum class ControlsVisibilityReason {
   // being set or cleared.
   kAnimation,
 
+  // If the renderer isn't able to update the controls position because it is
+  // being destroyed, crashed, or is unresponsive, show the controls.
+  kRendererUnavailable,
+
+  // Miscellaneous reasons for showing the controls, including:
+  //  * User entering text
+  //  * The URL being dangerous or having a warning
+  //  * An interstitial is showing
+  //  * chrome:// URL
+  kOther,
+
   kReasonCount,
 };
 

weblayer/browser/java/org/chromium/weblayer_private/BrowserControlsContainerView.java

@@ -315,6 +315,10 @@ class BrowserControlsContainerView extends FrameLayout {
         mOnlyExpandControlsAtPageTop = onlyExpandControlsAtPageTop;
     }
 
+    public boolean getOnlyExpandControlsAtPageTop() {
+        return mOnlyExpandControlsAtPageTop;
+    }
+
     /**
      * Called from ViewAndroidDelegate, see it for details.
      */

weblayer/browser/java/org/chromium/weblayer_private/BrowserViewController.java

@@ -215,6 +215,8 @@ public final class BrowserViewController
         if (mTab != null) {
             mTab.setBrowserControlsVisibilityConstraint(
                     ImplControlsVisibilityReason.ANIMATION, mBrowserControlsConstraint);
+            mTab.setOnlyExpandTopControlsAtPageTop(
+                    mTopControlsContainerView.getOnlyExpandControlsAtPageTop());
             mTab.onAttachedToViewController(mTopControlsContainerView.getNativeHandle(),
                     mBottomControlsContainerView.getNativeHandle());
             mContentView.requestFocus();
@@ -234,7 +236,13 @@ public final class BrowserViewController
     }
 
     public void setOnlyExpandTopControlsAtPageTop(boolean onlyExpandControlsAtPageTop) {
+        if (onlyExpandControlsAtPageTop
+                == mTopControlsContainerView.getOnlyExpandControlsAtPageTop()) {
+            return;
+        }
         mTopControlsContainerView.setOnlyExpandControlsAtPageTop(onlyExpandControlsAtPageTop);
+        if (mTab == null) return;
+        mTab.setOnlyExpandTopControlsAtPageTop(onlyExpandControlsAtPageTop);
     }
 
     public void setTopControlsAnimationsEnabled(boolean animationsEnabled) {

weblayer/browser/java/org/chromium/weblayer_private/TabImpl.java

@@ -115,7 +115,12 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
     // A list of browser control visibility constraints, indexed by ImplControlsVisibilityReason.
     private List<BrowserControlsVisibilityDelegate> mBrowserControlsDelegates;
     // Computes a net browser control visibility constraint from constituent constraints.
-    private ComposedBrowserControlsVisibilityDelegate mBrowserControlsVisibility;
+    private ComposedBrowserControlsVisibilityDelegate mComposedBrowserControlsVisibility;
+    // Which BrowserControlsVisibilityDelegate is currently controlling the visibility. The active
+    // delegate changes from mComposedBrowserControlsVisibility to the delegate for visibility
+    // reason RENDERER_UNAVAILABLE if onlyExpandControlsAtPageTop is enabled, in which case we don't
+    // want to ever force the controls to be visible unless the renderer isn't responsive.
+    private BrowserControlsVisibilityDelegate mActiveBrowserControlsVisibilityDelegate;
     // Invoked when the computed visibility constraint changes.
     private Callback<Integer> mConstraintsUpdatedCallback;
 
@@ -270,16 +275,17 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         mMediaStreamManager = new MediaStreamManager(this);
 
         mBrowserControlsDelegates = new ArrayList<BrowserControlsVisibilityDelegate>();
-        mBrowserControlsVisibility = new ComposedBrowserControlsVisibilityDelegate();
+        mComposedBrowserControlsVisibility = new ComposedBrowserControlsVisibilityDelegate();
         for (int i = 0; i < ImplControlsVisibilityReason.REASON_COUNT; ++i) {
             BrowserControlsVisibilityDelegate delegate =
                     new BrowserControlsVisibilityDelegate(BrowserControlsState.BOTH);
             mBrowserControlsDelegates.add(delegate);
-            mBrowserControlsVisibility.addDelegate(delegate);
+            mComposedBrowserControlsVisibility.addDelegate(delegate);
         }
         mConstraintsUpdatedCallback =
                 (constraint) -> onBrowserControlsConstraintUpdated(constraint);
-        mBrowserControlsVisibility.addObserver(mConstraintsUpdatedCallback);
+        mActiveBrowserControlsVisibilityDelegate = mComposedBrowserControlsVisibility;
+        mActiveBrowserControlsVisibilityDelegate.addObserver(mConstraintsUpdatedCallback);
 
         mInterceptNavigationDelegateClient = new InterceptNavigationDelegateClientImpl(this);
         mInterceptNavigationDelegate =
@@ -636,7 +642,9 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         if (controller == null) return false;
 
         // Refuse to start a find session when the browser controls are forced hidden.
-        if (mBrowserControlsVisibility.get() == BrowserControlsState.HIDDEN) return false;
+        if (mActiveBrowserControlsVisibilityDelegate.get() == BrowserControlsState.HIDDEN) {
+            return false;
+        }
 
         setBrowserControlsVisibilityConstraint(
                 ImplControlsVisibilityReason.FIND_IN_PAGE, BrowserControlsState.SHOWN);
@@ -950,7 +958,7 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         // ObservableSupplierImpl.addObserver() posts a task to notify the observer, ensure the
         // callback isn't run after destroy() is called (otherwise we'll get crashes as the native
         // tab has been deleted).
-        mBrowserControlsVisibility.removeObserver(mConstraintsUpdatedCallback);
+        mActiveBrowserControlsVisibilityDelegate.removeObserver(mConstraintsUpdatedCallback);
         hideFindInPageUiAndNotifyClient();
         mFindInPageCallbackClient = null;
         mNavigationController = null;
@@ -979,6 +987,17 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         return mBrowserControlsDelegates.get(reason).get();
     }
 
+    public void setOnlyExpandTopControlsAtPageTop(boolean onlyExpandControlsAtPageTop) {
+        BrowserControlsVisibilityDelegate activeDelegate = onlyExpandControlsAtPageTop
+                ? mBrowserControlsDelegates.get(ImplControlsVisibilityReason.RENDERER_UNAVAILABLE)
+                : mComposedBrowserControlsVisibility;
+        if (activeDelegate == mActiveBrowserControlsVisibilityDelegate) return;
+
+        mActiveBrowserControlsVisibilityDelegate.removeObserver(mConstraintsUpdatedCallback);
+        mActiveBrowserControlsVisibilityDelegate = activeDelegate;
+        mActiveBrowserControlsVisibilityDelegate.addObserver(mConstraintsUpdatedCallback);
+    }
+
     @CalledByNative
     public void showRepostFormWarningDialog() {
         BrowserViewController viewController = getViewController();
@@ -1003,20 +1022,9 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
                 ObjectWrapper.wrap(nonEmptyOrNull(params.getSrcUrl())));
     }
 
-    @CalledByNative
-    private void onForceBrowserControlsShown() {
-        // At this time, the only place HIDDEN is used is fullscreen, in which case we don't show
-        // the controls.
-        if (mBrowserControlsVisibility.get() == BrowserControlsState.HIDDEN) return;
-
-        if (mBrowser.getActiveTab() != this) return;
-
-        onBrowserControlsConstraintUpdated(mBrowserControlsVisibility.get());
-    }
-
     @VisibleForTesting
     public boolean canBrowserControlsScrollForTesting() {
-        return mBrowserControlsVisibility.get() == BrowserControlsState.BOTH;
+        return mActiveBrowserControlsVisibilityDelegate.get() == BrowserControlsState.BOTH;
     }
 
     @VisibleForTesting
@@ -1032,9 +1040,9 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
             hideFindInPageUiAndNotifyClient();
         }
 
-        BrowserViewController viewController = getViewController();
         // Don't animate when hiding the controls unless an animation was requested by
         // BrowserControlsContainerView.
+        BrowserViewController viewController = getViewController();
         boolean animate = constraint != BrowserControlsState.HIDDEN
                 || (viewController != null
                         && viewController.shouldAnimateBrowserControlsHeightChanges());

weblayer/browser/tab_impl.cc

@@ -1172,9 +1172,9 @@ void TabImpl::OnFindResultAvailable(content::WebContents* web_contents) {
 
 #if defined(OS_ANDROID)
 void TabImpl::OnBrowserControlsStateStateChanged(
+    ControlsVisibilityReason reason,
     content::BrowserControlsState state) {
-  SetBrowserControlsConstraint(ControlsVisibilityReason::kPostNavigation,
-                               state);
+  SetBrowserControlsConstraint(reason, state);
 }
 
 void TabImpl::OnUpdateBrowserControlsStateBecauseOfProcessSwitch(
@@ -1197,10 +1197,6 @@ void TabImpl::OnUpdateBrowserControlsStateBecauseOfProcessSwitch(
   }
 }
 
-void TabImpl::OnForceBrowserControlsShown() {
-  Java_TabImpl_onForceBrowserControlsShown(AttachCurrentThread(), java_impl_);
-}
-
 #endif
 
 void TabImpl::DidChangeVisibleSecurityState() {

weblayer/browser/tab_impl.h

@@ -338,10 +338,10 @@ class TabImpl : public Tab,
 #if defined(OS_ANDROID)
   // BrowserControlsNavigationStateHandlerDelegate:
   void OnBrowserControlsStateStateChanged(
+      ControlsVisibilityReason reason,
       content::BrowserControlsState state) override;
   void OnUpdateBrowserControlsStateBecauseOfProcessSwitch(
       bool did_commit) override;
-  void OnForceBrowserControlsShown() override;
 #endif
 
   // Called from closure supplied to delegate to exit fullscreen.