Skip to content

Commit

Permalink
Revert "[Start] Reland: Make Start not scrollable when the content is…
Browse files Browse the repository at this point in the history
… shorter than screen."

This reverts commit cc20d61.

Reason for revert: crbug.com/4021417

Original change's description:
> [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}

(cherry picked from commit 95614a1)

Bug: 1208486, 1383105, 1385600
Change-Id: I88946a4fed6c3eab712c714fddf05671d2203b22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4117592
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1086080}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4121615
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#51}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Xi Han authored and Chromium LUCI CQ committed Dec 22, 2022
1 parent 6113398 commit bad50f0
Show file tree
Hide file tree
Showing 13 changed files with 59 additions and 255 deletions.
Expand Up @@ -4,15 +4,11 @@

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;
Expand All @@ -22,7 +18,6 @@
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;
Expand Down Expand Up @@ -157,7 +152,6 @@ 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.
Expand Down Expand Up @@ -299,28 +293,26 @@ 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 =
mHasPrimaryTasksSurfaceCreated ? mTasksSurface::initializeMVTiles : null;
View logoContainerView = mHasPrimaryTasksSurfaceCreated
? mTasksSurface.getView().findViewById(R.id.logo_container)
: null;
mTasksSurface == null ? null : mTasksSurface::initializeMVTiles;
View logoContainerView = mTasksSurface == null
? null
: mTasksSurface.getView().findViewById(R.id.logo_container);
ViewGroup feedPlaceholderParentView =
mHasPrimaryTasksSurfaceCreated ? mTasksSurface.getBodyViewContainer() : null;
mTasksSurface == null ? null : mTasksSurface.getBodyViewContainer();
mStartSurfaceMediator = new StartSurfaceMediator(controller, containerView,
mTabModelSelector, mPropertyModel,
mHasPrimaryTasksSurfaceCreated ? this::initializeSecondaryTasksSurface : null,
mIsStartSurfaceEnabled ? this::initializeSecondaryTasksSurface : null,
mIsStartSurfaceEnabled, mActivity, mBrowserControlsManager,
this::isActivityFinishingOrDestroyed, excludeQueryTiles,
startSurfaceOneshotSupplier, hadWarmStart, jankTracker, initializeMVTilesRunnable,
mParentTabSupplier, logoContainerView, backPressManager, feedPlaceholderParentView,
mActivityLifecycleDispatcher);

mayUpdateLayoutParams(ApiCompatibilityUtils.isInMultiWindowMode(mActivity));
startSurfaceOneshotSupplier.set(this);
}

Expand Down Expand Up @@ -726,9 +718,6 @@ 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,
Expand All @@ -737,29 +726,6 @@ 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;
Expand Down
Expand Up @@ -54,7 +54,6 @@
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;
Expand All @@ -67,9 +66,7 @@
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;
Expand All @@ -87,7 +84,6 @@
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;
Expand Down Expand Up @@ -468,20 +464,6 @@ 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);
Expand Down Expand Up @@ -1062,10 +1044,6 @@ public boolean shouldShowFeedPlaceholder() {
&& !mHasFeedPlaceholderShown;
}

boolean hasFeedPlaceholderShown() {
return mHasFeedPlaceholderShown;
}

void setSecondaryTasksSurfaceController(
TabSwitcher.Controller secondaryTasksSurfaceController) {
mSecondaryTasksSurfaceController = secondaryTasksSurfaceController;
Expand Down
Expand Up @@ -130,9 +130,4 @@ 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();
}
Expand Up @@ -335,11 +335,6 @@ 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,
Expand Down
Expand Up @@ -87,7 +87,7 @@ private TasksSurfaceProperties() {}
public static final PropertyModel.WritableIntPropertyKey TOP_TOOLBAR_PLACEHOLDER_HEIGHT =
new PropertyModel.WritableIntPropertyKey();
public static final PropertyModel
.WritableObjectPropertyKey<Boolean> RESET_TASK_SURFACE_HEADER_SCROLL_POSITION =
.WritableObjectPropertyKey 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,
Expand Down
Expand Up @@ -54,7 +54,6 @@ 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;
Expand All @@ -64,7 +63,7 @@ public class TasksView extends CoordinatorLayoutForPointer {
CookieControlsEnforcement.NO_ENFORCEMENT;
private View.OnClickListener mIncognitoCookieControlsIconClickListener;
private UiConfig mUiConfig;
private int mContentHeight;
private boolean mIsIncognito;

/** Default constructor needed to inflate via XML. */
public TasksView(Context context, AttributeSet attrs) {
Expand All @@ -90,7 +89,8 @@ protected void onFinishInflate() {
mSearchBoxCoordinator = new SearchBoxCoordinator(getContext(), this);

mHeaderView = (AppBarLayout) findViewById(R.id.task_surface_header);
mBodyViewContainer = findViewById(R.id.tasks_surface_body);

forceHeaderScrollable();

mUiConfig = new UiConfig(this);
setHeaderPadding();
Expand Down Expand Up @@ -122,15 +122,15 @@ ViewGroup getCarouselTabSwitcherContainer() {
}

ViewGroup getBodyViewContainer() {
return mBodyViewContainer;
return findViewById(R.id.tasks_surface_body);
}

/**
* Set the visibility of the tasks surface body.
* @param isVisible Whether it's visible.
*/
void setSurfaceBodyVisibility(boolean isVisible) {
mBodyViewContainer.setVisibility(isVisible ? View.VISIBLE : View.GONE);
getBodyViewContainer().setVisibility(isVisible ? View.VISIBLE : View.GONE);
}

/**
Expand Down Expand Up @@ -208,6 +208,7 @@ 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;
}

/**
Expand Down Expand Up @@ -320,7 +321,7 @@ void setIncognitoCookieControlsIconClickListener(OnClickListener listener) {
* @param topMargin The top margin to set.
*/
void setTasksSurfaceBodyTopMargin(int topMargin) {
MarginLayoutParams params = (MarginLayoutParams) mBodyViewContainer.getLayoutParams();
MarginLayoutParams params = (MarginLayoutParams) getBodyViewContainer().getLayoutParams();
params.topMargin = topMargin;
}

Expand Down Expand Up @@ -426,25 +427,15 @@ void updateFakeSearchBox(int height, int topMargin, int endPadding, float transl
mSearchBoxCoordinator.setLensButtonLeftMargin(lensButtonLeftMargin);
}

void initHeaderDragListener() {
mHeaderView.addOnLayoutChangeListener(
(view, left, top, right, bottom, oldLeft, oldTop, oldRight, oldBottom) -> {
mContentHeight = mHeaderView.getMeasuredHeight()
+ mBodyViewContainer.getMeasuredHeight();
});

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.
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;
}
});
Expand Down
Expand Up @@ -5,16 +5,22 @@
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;
Expand Down Expand Up @@ -70,6 +76,8 @@
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();

Expand Down Expand Up @@ -141,7 +149,7 @@ public void testCachedFeedVisibility() {

// Hide articles and verify that FEED_ARTICLES_LIST_VISIBLE and ARTICLES_LIST_VISIBLE are
// both false.
toggleFeedHeader(false);
toggleHeader(false);
CriteriaHelper.pollUiThread(() -> !ReturnToChromeUtil.getFeedArticlesVisibility());
TestThreadUtils.runOnUiThreadBlocking(
()
Expand All @@ -151,7 +159,7 @@ public void testCachedFeedVisibility() {

// Show articles and verify that FEED_ARTICLES_LIST_VISIBLE and ARTICLES_LIST_VISIBLE are
// both true.
toggleFeedHeader(true);
toggleHeader(true);
CriteriaHelper.pollUiThread(ReturnToChromeUtil::getFeedArticlesVisibility);
TestThreadUtils.runOnUiThreadBlocking(
()
Expand Down Expand Up @@ -190,6 +198,26 @@ 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;
Expand Down
Expand Up @@ -139,7 +139,7 @@ public void renderSingleAsHomepage_NoTab_scrollToolbarToTop() throws IOException
StartSurfaceTestUtils.waitForTabModel(cta);
TabUiTestHelper.verifyTabModelTabCount(cta, 0, 0);

StartSurfaceTestUtils.scrollToolbarAndVerify(cta);
StartSurfaceTestUtils.scrollToolbar(cta);

View surface = cta.findViewById(org.chromium.chrome.test.R.id.control_container);
ChromeRenderTestRule.sanitize(surface);
Expand Down
Expand Up @@ -27,7 +27,6 @@

import android.view.KeyEvent;
import android.view.View;
import android.view.ViewGroup;
import android.widget.TextView;

import androidx.recyclerview.widget.RecyclerView;
Expand Down Expand Up @@ -164,16 +163,8 @@ public void testShow_SingleAsTabSwitcher() {
}

ChromeTabbedActivity cta = mActivityTestRule.getActivity();

if (!ChromeFeatureList.sStartSurfaceRefactor.isEnabled()) {
onViewWaiting(withId(R.id.secondary_tasks_surface_view));

// Grid tab switcher surface's height shouldn't be wrap_content. See crbug.com/1368437.
ViewGroup gtsSurfaceView = cta.findViewById(R.id.secondary_tasks_surface_view);
int surfaceHeight = gtsSurfaceView.getMeasuredHeight();
int bodyHeight =
gtsSurfaceView.findViewById(R.id.tasks_surface_body).getMeasuredHeight();
Assert.assertEquals(surfaceHeight, bodyHeight);
}

onViewWaiting(allOf(withParent(withId(TabUiTestHelper.getTabSwitcherParentId(cta))),
Expand Down

0 comments on commit bad50f0

Please sign in to comment.