[Start] Reland: Make Start not scrollable when the content is shorter…

… than screen. The original CL got reverted because it breaks the scrolling on Start when splitting screen happens. This CL fixes the issue by querying the displayMatrics() in real time rather than using the cached value. The CL also fixes another multiple window issue: when Feeds is disabled, we have to change the height of the tasks_surface_body back to "match_parent" to prevent the Feeds's header scrolling above the toolbar and can't be scrolled back. The original CL description is: 1. Resets Start surface scroll when Feed is turned off. 2. Sets tasks surface body's height WRAP_CONTENT and check whether tasks surface view is shorter than the screen size. If it is, makes it not scrollable by setting AppBarLayout#canDrag false. Video: http://shortn/_u5D8TNdiuN Video for small phone with signin promo: https://drive.google.com/file/d/1VnUAR4xPbLchgfTVu-6Ti-UCKWpdAQkj/view?usp=share_link Video for multiple window: https://drive.google.com/file/d/1VDWuXMmKl1BjzaNek8gLA55lQGauleyr/view?usp=share_link Bug: 1208486, 1383105 Change-Id: Ie151f834063d1aa82c47081380114f33d9bec935 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4021417 Reviewed-by: Wei-Yin Chen <wychen@chromium.org> Reviewed-by: Fred Mello <fredmello@chromium.org> Commit-Queue: Xi Han <hanxi@chromium.org> Cr-Commit-Position: refs/heads/main@{#1071891}
chromium · Nov 15, 2022 · cc20d61 · cc20d61
1 parent 76d0045
commit cc20d61
Show file tree

Hide file tree

Showing 13 changed files with 254 additions and 61 deletions.
diff --git a/..._surface/java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java b/..._surface/java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java
@@ -4,11 +4,15 @@
 
 package org.chromium.chrome.features.start_surface;
 
+import static android.view.ViewGroup.LayoutParams.MATCH_PARENT;
+import static android.view.ViewGroup.LayoutParams.WRAP_CONTENT;
+
 import android.app.Activity;
 import android.os.SystemClock;
 import android.text.TextUtils;
 import android.view.View;
 import android.view.ViewGroup;
+import android.view.ViewGroup.LayoutParams;
 import android.widget.FrameLayout;
 
 import androidx.annotation.NonNull;
@@ -18,6 +22,7 @@
 import com.google.android.material.appbar.AppBarLayout;
 
 import org.chromium.base.ActivityState;
+import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.ApplicationStatus;
 import org.chromium.base.Log;
 import org.chromium.base.MathUtils;
@@ -152,6 +157,7 @@ public class StartSurfaceCoordinator implements StartSurface {
     private boolean mIsInitPending;
 
     private boolean mIsSecondaryTaskInitPending;
+    private boolean mHasPrimaryTasksSurfaceCreated;
 
     // Listeners used by the contained surfaces (e.g., Explore) to listen to the scroll changes on
     // the main scrollable container of the start surface.
@@ -292,25 +298,27 @@ public StartSurfaceCoordinator(@NonNull Activity activity,
             // createSwipeRefreshLayout has to be called before creating any surface.
             createSwipeRefreshLayout();
             createAndSetStartSurface(excludeQueryTiles);
+            mHasPrimaryTasksSurfaceCreated = true;
         }
 
         TabSwitcher.Controller controller =
                 mTabSwitcher != null ? mTabSwitcher.getController() : mTasksSurface.getController();
         Runnable initializeMVTilesRunnable =
-                mTasksSurface == null ? null : mTasksSurface::initializeMVTiles;
-        View logoContainerView = mTasksSurface == null
-                ? null
-                : mTasksSurface.getView().findViewById(R.id.logo_container);
+                mHasPrimaryTasksSurfaceCreated ? mTasksSurface::initializeMVTiles : null;
+        View logoContainerView = mHasPrimaryTasksSurfaceCreated
+                ? mTasksSurface.getView().findViewById(R.id.logo_container)
+                : null;
         ViewGroup feedPlaceholderParentView =
-                mTasksSurface == null ? null : mTasksSurface.getBodyViewContainer();
+                mHasPrimaryTasksSurfaceCreated ? mTasksSurface.getBodyViewContainer() : null;
         mStartSurfaceMediator = new StartSurfaceMediator(controller, containerView,
                 mTabModelSelector, mPropertyModel,
-                mIsStartSurfaceEnabled ? this::initializeSecondaryTasksSurface : null,
+                mHasPrimaryTasksSurfaceCreated ? this::initializeSecondaryTasksSurface : null,
                 mIsStartSurfaceEnabled, mActivity, mBrowserControlsManager,
                 this::isActivityFinishingOrDestroyed, excludeQueryTiles,
                 startSurfaceOneshotSupplier, hadWarmStart, jankTracker, initializeMVTilesRunnable,
                 mParentTabSupplier, logoContainerView, backPressManager, feedPlaceholderParentView);
 
+        mayUpdateLayoutParams(ApiCompatibilityUtils.isInMultiWindowMode(mActivity));
         startSurfaceOneshotSupplier.set(this);
     }
 
@@ -705,6 +713,9 @@ private void createAndSetStartSurface(boolean excludeQueryTiles) {
         mTasksSurface.getView().setId(R.id.primary_tasks_surface_view);
         initializeOffsetChangedListener();
         addHeaderOffsetChangeListener(mOffsetChangedListenerToGenerateScrollEvents);
+        mTasksSurface.initHeaderDragListener();
+        mMultiWindowModeStateDispatcher.addObserver(
+                isInMultiWindowMode -> mayUpdateLayoutParams(isInMultiWindowMode));
 
         mTasksSurfacePropertyModelChangeProcessor =
                 PropertyModelChangeProcessor.create(mPropertyModel,
@@ -713,6 +724,29 @@ private void createAndSetStartSurface(boolean excludeQueryTiles) {
                         TasksSurfaceViewBinder::bind);
     }
 
+    private void mayUpdateLayoutParams(boolean isInMultiWindowMode) {
+        // Set the height of Start surface homepage's tasks_surface_body as wrap_content. We only
+        // update the tasks_surface_body in mTasksSurface since it's Start surface homepage while we
+        // don't change mSecondaryTasksSurface (grid tab switcher).
+        // If Feed placeholder is shown and we set body's height wrap_content, feed articles aren't
+        // rendered on emulator. Thus we need to add the check of whether the place holder is
+        // showing. See https://crbug.com/1208486.
+        if (!mHasPrimaryTasksSurfaceCreated || mStartSurfaceMediator.hasFeedPlaceholderShown()) {
+            return;
+        }
+
+        FrameLayout bodyViewLayout = (FrameLayout) mTasksSurface.getBodyViewContainer();
+        LayoutParams params = bodyViewLayout.getLayoutParams();
+        // Keeps the height to be "match_parent" in multiple window mode. When Feeds is disabled, it
+        // prevents the tasks_surface_body from scrolling above the toolbar and can't be scrolling
+        // down.
+        int heightToSet = isInMultiWindowMode ? MATCH_PARENT : WRAP_CONTENT;
+        if (params.height == heightToSet) return;
+
+        params.height = heightToSet;
+        bodyViewLayout.setLayoutParams(params);
+    }
+
     private TabSwitcher.Controller initializeSecondaryTasksSurface() {
         assert mIsStartSurfaceEnabled;
         assert mSecondaryTasksSurface == null;

diff --git a/...art_surface/java/src/org/chromium/chrome/features/start_surface/StartSurfaceMediator.java b/...art_surface/java/src/org/chromium/chrome/features/start_surface/StartSurfaceMediator.java
@@ -54,6 +54,7 @@
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.back_press.BackPressManager;
 import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
+import org.chromium.chrome.browser.feed.FeedFeatures;
 import org.chromium.chrome.browser.feed.FeedReliabilityLogger;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.lens.LensEntryPoint;
@@ -64,7 +65,9 @@
 import org.chromium.chrome.browser.omnibox.UrlFocusChangeListener;
 import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
 import org.chromium.chrome.browser.preferences.Pref;
+import org.chromium.chrome.browser.preferences.PrefChangeRegistrar;
 import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
+import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.TabLaunchType;
 import org.chromium.chrome.browser.tab.TabSelectionType;
@@ -82,6 +85,7 @@
 import org.chromium.components.browser_ui.widget.gesture.BackPressHandler;
 import org.chromium.components.embedder_support.util.UrlUtilities;
 import org.chromium.components.prefs.PrefService;
+import org.chromium.components.user_prefs.UserPrefs;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.ui.modelutil.PropertyModel;
 import org.chromium.ui.util.ColorUtils;
@@ -452,6 +456,20 @@ void initWithNative(@Nullable OmniboxStub omniboxStub,
                     if (mLogoCoordinator != null) mLogoCoordinator.initWithNative();
                 }
             }
+
+            // If feed is disabled from user toggling off the header, Start surface will be set not
+            // scrollable, so we reset the scroll position to make it visible in the entire screen
+            // again.
+            PrefChangeRegistrar prefChangeRegistrar = new PrefChangeRegistrar();
+            prefChangeRegistrar.addObserver(Pref.ARTICLES_LIST_VISIBLE, () -> {
+                boolean isFeedVisible = FeedFeatures.isFeedEnabled()
+                        && UserPrefs.get(Profile.getLastUsedRegularProfile())
+                                   .getBoolean(Pref.ARTICLES_LIST_VISIBLE);
+                if (!isFeedVisible) {
+                    mPropertyModel.set(RESET_TASK_SURFACE_HEADER_SCROLL_POSITION, true);
+                    maybeDestroyFeedPlaceholder();
+                }
+            });
         }
 
         mFeedVisibilityPrefOnStartUp = prefService.getBoolean(Pref.ARTICLES_LIST_VISIBLE);
@@ -1003,6 +1021,10 @@ public boolean shouldShowFeedPlaceholder() {
                 && !mHasFeedPlaceholderShown;
     }
 
+    boolean hasFeedPlaceholderShown() {
+        return mHasFeedPlaceholderShown;
+    }
+
     void setSecondaryTasksSurfaceController(
             TabSwitcher.Controller secondaryTasksSurfaceController) {
         mSecondaryTasksSurfaceController = secondaryTasksSurfaceController;

diff --git a/...roid/features/start_surface/java/src/org/chromium/chrome/features/tasks/TasksSurface.java b/...roid/features/start_surface/java/src/org/chromium/chrome/features/tasks/TasksSurface.java
@@ -130,4 +130,9 @@ void updateFakeSearchBox(int height, int topMargin, int endPadding, float transl
      */
     @Nullable
     TabSwitcherCustomViewManager getTabSwitcherCustomViewManager();
+
+    /**
+     * Initialize the listener to decide whether the tasks view can be draggable or scrollable.
+     */
+    void initHeaderDragListener();
 }
diff --git a/...es/start_surface/java/src/org/chromium/chrome/features/tasks/TasksSurfaceCoordinator.java b/...es/start_surface/java/src/org/chromium/chrome/features/tasks/TasksSurfaceCoordinator.java
@@ -335,6 +335,11 @@ public boolean isMVTilesInitialized() {
         return (mTabSwitcher != null) ? mTabSwitcher.getTabSwitcherCustomViewManager() : null;
     }
 
+    @Override
+    public void initHeaderDragListener() {
+        mView.initHeaderDragListener();
+    }
+
     /** Suggestions UI Delegate for constructing the TileGroup. */
     private class MostVisitedSuggestionsUiDelegate extends SuggestionsUiDelegateImpl {
         public MostVisitedSuggestionsUiDelegate(SuggestionsNavigationDelegate navigationDelegate,

diff --git a/...res/start_surface/java/src/org/chromium/chrome/features/tasks/TasksSurfaceProperties.java b/...res/start_surface/java/src/org/chromium/chrome/features/tasks/TasksSurfaceProperties.java
@@ -87,7 +87,7 @@ private TasksSurfaceProperties() {}
     public static final PropertyModel.WritableIntPropertyKey TOP_TOOLBAR_PLACEHOLDER_HEIGHT =
             new PropertyModel.WritableIntPropertyKey();
     public static final PropertyModel
-            .WritableObjectPropertyKey RESET_TASK_SURFACE_HEADER_SCROLL_POSITION =
+            .WritableObjectPropertyKey<Boolean> RESET_TASK_SURFACE_HEADER_SCROLL_POSITION =
             new PropertyModel.WritableObjectPropertyKey<>(true /* skipEquality */);
     public static final PropertyKey[] ALL_KEYS = new PropertyKey[] {IS_FAKE_SEARCH_BOX_VISIBLE,
             IS_INCOGNITO, IS_INCOGNITO_DESCRIPTION_INITIALIZED, IS_INCOGNITO_DESCRIPTION_VISIBLE,

diff --git a/...android/features/start_surface/java/src/org/chromium/chrome/features/tasks/TasksView.java b/...android/features/start_surface/java/src/org/chromium/chrome/features/tasks/TasksView.java
@@ -52,6 +52,7 @@ public class TasksView extends CoordinatorLayoutForPointer {
     private FrameLayout mCarouselTabSwitcherContainer;
     private AppBarLayout mHeaderView;
     private ViewGroup mMvTilesContainerLayout;
+    private ViewGroup mBodyViewContainer;
     private SearchBoxCoordinator mSearchBoxCoordinator;
     private IncognitoDescriptionView mIncognitoDescriptionView;
     private View.OnClickListener mIncognitoDescriptionLearnMoreListener;
@@ -61,7 +62,7 @@ public class TasksView extends CoordinatorLayoutForPointer {
             CookieControlsEnforcement.NO_ENFORCEMENT;
     private View.OnClickListener mIncognitoCookieControlsIconClickListener;
     private UiConfig mUiConfig;
-    private boolean mIsIncognito;
+    private int mContentHeight;
 
     /** Default constructor needed to inflate via XML. */
     public TasksView(Context context, AttributeSet attrs) {
@@ -87,8 +88,7 @@ protected void onFinishInflate() {
         mSearchBoxCoordinator = new SearchBoxCoordinator(getContext(), this);
 
         mHeaderView = (AppBarLayout) findViewById(R.id.task_surface_header);
-
-        forceHeaderScrollable();
+        mBodyViewContainer = findViewById(R.id.tasks_surface_body);
 
         mUiConfig = new UiConfig(this);
         setHeaderPadding();
@@ -120,15 +120,15 @@ ViewGroup getCarouselTabSwitcherContainer() {
     }
 
     ViewGroup getBodyViewContainer() {
-        return findViewById(R.id.tasks_surface_body);
+        return mBodyViewContainer;
     }
 
     /**
      * Set the visibility of the tasks surface body.
      * @param isVisible Whether it's visible.
      */
     void setSurfaceBodyVisibility(boolean isVisible) {
-        getBodyViewContainer().setVisibility(isVisible ? View.VISIBLE : View.GONE);
+        mBodyViewContainer.setVisibility(isVisible ? View.VISIBLE : View.GONE);
     }
 
     /**
@@ -206,7 +206,6 @@ void setIncognitoMode(boolean isIncognito) {
         int hintTextColor = mContext.getColor(isIncognito ? R.color.locationbar_light_hint_text
                                                           : R.color.locationbar_dark_hint_text);
         mSearchBoxCoordinator.setSearchBoxHintColor(hintTextColor);
-        mIsIncognito = isIncognito;
     }
 
     /**
@@ -320,7 +319,7 @@ void setIncognitoCookieControlsIconClickListener(OnClickListener listener) {
      * @param topMargin The top margin to set.
      */
     void setTasksSurfaceBodyTopMargin(int topMargin) {
-        MarginLayoutParams params = (MarginLayoutParams) getBodyViewContainer().getLayoutParams();
+        MarginLayoutParams params = (MarginLayoutParams) mBodyViewContainer.getLayoutParams();
         params.topMargin = topMargin;
     }
 
@@ -426,15 +425,25 @@ void updateFakeSearchBox(int height, int topMargin, int endPadding, float transl
         mSearchBoxCoordinator.setLensButtonLeftMargin(lensButtonLeftMargin);
     }
 
-    private void forceHeaderScrollable() {
-        // TODO(https://crbug.com/1251632): Find out why scrolling was broken after
-        // crrev.com/c/3025127. Force the header view to be draggable as a workaround.
+    void initHeaderDragListener() {
+        mHeaderView.addOnLayoutChangeListener(
+                (view, left, top, right, bottom, oldLeft, oldTop, oldRight, oldBottom) -> {
+                    mContentHeight = mHeaderView.getMeasuredHeight()
+                            + mBodyViewContainer.getMeasuredHeight();
+                });
+
         CoordinatorLayout.LayoutParams params =
                 (CoordinatorLayout.LayoutParams) mHeaderView.getLayoutParams();
         AppBarLayout.Behavior behavior = new AppBarLayout.Behavior();
         behavior.setDragCallback(new AppBarLayout.Behavior.DragCallback() {
             @Override
             public boolean canDrag(AppBarLayout appBarLayout) {
+                if (mContentHeight <= mContext.getResources().getDisplayMetrics().heightPixels) {
+                    // Needs to reset the offset of the Start surface to prevent it stay in the
+                    // middle of the screen but can't scroll any more.
+                    resetScrollPosition();
+                    return false;
+                }
                 return true;
             }
         });

diff --git a/...urface/javatests/src/org/chromium/chrome/features/start_surface/InstantStartFeedTest.java b/...urface/javatests/src/org/chromium/chrome/features/start_surface/InstantStartFeedTest.java
@@ -5,22 +5,16 @@
 package org.chromium.chrome.features.start_surface;
 
 import static androidx.test.espresso.Espresso.onView;
-import static androidx.test.espresso.action.ViewActions.click;
 import static androidx.test.espresso.assertion.ViewAssertions.doesNotExist;
 import static androidx.test.espresso.assertion.ViewAssertions.matches;
 import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed;
 import static androidx.test.espresso.matcher.ViewMatchers.withId;
-import static androidx.test.espresso.matcher.ViewMatchers.withText;
-
-import static org.hamcrest.CoreMatchers.allOf;
-import static org.hamcrest.Matchers.instanceOf;
 
 import static org.chromium.chrome.features.start_surface.StartSurfaceTestUtils.INSTANT_START_TEST_BASE_PARAMS;
+import static org.chromium.chrome.features.start_surface.StartSurfaceTestUtils.toggleFeedHeader;
 
 import android.view.View;
 
-import androidx.recyclerview.widget.RecyclerView;
-import androidx.test.espresso.contrib.RecyclerViewActions;
 import androidx.test.espresso.matcher.BoundedMatcher;
 import androidx.test.filters.LargeTest;
 import androidx.test.filters.SmallTest;
@@ -76,8 +70,6 @@
     UiRestriction.RESTRICTION_TYPE_PHONE})
 public class InstantStartFeedTest {
     // clang-format on
-    private static final int ARTICLE_SECTION_HEADER_POSITION = 0;
-
     @Rule
     public JniMocker mJniMocker = new JniMocker();
 
@@ -149,7 +141,7 @@ public void testCachedFeedVisibility() {
 
         // Hide articles and verify that FEED_ARTICLES_LIST_VISIBLE and ARTICLES_LIST_VISIBLE are
         // both false.
-        toggleHeader(false);
+        toggleFeedHeader(false);
         CriteriaHelper.pollUiThread(() -> !ReturnToChromeUtil.getFeedArticlesVisibility());
         TestThreadUtils.runOnUiThreadBlocking(
                 ()
@@ -159,7 +151,7 @@ public void testCachedFeedVisibility() {
 
         // Show articles and verify that FEED_ARTICLES_LIST_VISIBLE and ARTICLES_LIST_VISIBLE are
         // both true.
-        toggleHeader(true);
+        toggleFeedHeader(true);
         CriteriaHelper.pollUiThread(ReturnToChromeUtil::getFeedArticlesVisibility);
         TestThreadUtils.runOnUiThreadBlocking(
                 ()
@@ -198,26 +190,6 @@ public void testShowFeedPlaceholder() {
                 .check(matches(isDisplayed()));
     }
 
-    /**
-     * Toggles the header and checks whether the header has the right status.
-     *
-     * @param expanded Whether the header should be expanded.
-     */
-    private void toggleHeader(boolean expanded) {
-        onView(allOf(instanceOf(RecyclerView.class),
-                       withId(org.chromium.chrome.test.R.id.feed_stream_recycler_view)))
-                .perform(RecyclerViewActions.scrollToPosition(ARTICLE_SECTION_HEADER_POSITION));
-        onView(withId(org.chromium.chrome.test.R.id.header_menu)).perform(click());
-
-        onView(withText(expanded ? org.chromium.chrome.test.R.string.ntp_turn_on_feed
-                                 : org.chromium.chrome.test.R.string.ntp_turn_off_feed))
-                .perform(click());
-
-        onView(withText(expanded ? org.chromium.chrome.test.R.string.ntp_discover_on
-                                 : org.chromium.chrome.test.R.string.ntp_discover_off))
-                .check(matches(isDisplayed()));
-    }
-
     private static Matcher<View> matchesBackgroundAlpha(final int expectedAlpha) {
         return new BoundedMatcher<View, View>(View.class) {
             String mMessage;

diff --git a/...ace/javatests/src/org/chromium/chrome/features/start_surface/InstantStartToolbarTest.java b/...ace/javatests/src/org/chromium/chrome/features/start_surface/InstantStartToolbarTest.java
@@ -140,7 +140,7 @@ public void renderSingleAsHomepage_NoTab_scrollToolbarToTop() throws IOException
         StartSurfaceTestUtils.waitForTabModel(cta);
         TabUiTestHelper.verifyTabModelTabCount(cta, 0, 0);
 
-        StartSurfaceTestUtils.scrollToolbar(cta);
+        StartSurfaceTestUtils.scrollToolbarAndVerify(cta);
 
         View surface = cta.findViewById(org.chromium.chrome.test.R.id.control_container);
         ChromeRenderTestRule.sanitize(surface);

diff --git a/...javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceTabSwitcherTest.java b/...javatests/src/org/chromium/chrome/features/start_surface/StartSurfaceTabSwitcherTest.java
@@ -162,13 +162,19 @@ public void testShow_SingleAsTabSwitcher() {
             TabUiTestHelper.enterTabSwitcher(mActivityTestRule.getActivity());
         }
 
+        ChromeTabbedActivity cta = mActivityTestRule.getActivity();
         onViewWaiting(withId(R.id.secondary_tasks_surface_view));
 
+        // Grid tab switcher surface's height shouldn't be wrap_content. See crbug.com/1368437.
+        ViewGroup gts_surface_view = cta.findViewById(R.id.secondary_tasks_surface_view);
+        int surfaceHeight = gts_surface_view.getMeasuredHeight();
+        int bodyHeight = gts_surface_view.findViewById(R.id.tasks_surface_body).getMeasuredHeight();
+        Assert.assertEquals(surfaceHeight, bodyHeight);
+
         onViewWaiting(
                 allOf(withParent(withId(R.id.tasks_surface_body)), withId(R.id.tab_list_view)))
                 .perform(RecyclerViewActions.actionOnItemAtPosition(0, click()));
-        LayoutTestUtils.waitForLayout(
-                mActivityTestRule.getActivity().getLayoutManager(), LayoutType.BROWSING);
+        LayoutTestUtils.waitForLayout(cta.getLayoutManager(), LayoutType.BROWSING);
     }
 
     @Test