Skip to content

Commit

Permalink
[StartTablet] Update the interval padding of carousel MVT on tablets.
Browse files Browse the repository at this point in the history
This is a follow up of CL: https://crrev.com/c/4542600.
This CL changes another version of calculating the interval padding of the carousel MVT on tablets. With the current algorithm, there will be half of the tile element at the end of MVT only when we can't fit all the tile elements in the MV Tiles.
See the updated screenshots: https://docs.google.com/document/d/1h1LcikSwJtlQyWt2vMHMXq0tpcG9UqUkpUnRmBe6F10/edit?usp=sharing&resourcekey=0-dlELgLy0WrCIZJpRr6XYRg

(cherry picked from commit b35015c)

Low-Coverage-Reason: Some of the changes are tested in instrument test on tablet.
Bug: 1416536
Change-Id: I36677a340fd0740e31ea971e0e1daddacb034bca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4567123
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: Peter Williamson <petewil@chromium.org>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Commit-Queue: Xinyi Ji <xinyiji@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1151318}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4582477
Cr-Commit-Position: refs/branch-heads/5790@{#241}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Xinyi Ji authored and Chromium LUCI CQ committed Jun 1, 2023
1 parent 1a61c8c commit 2af9202
Show file tree
Hide file tree
Showing 5 changed files with 182 additions and 85 deletions.
Expand Up @@ -19,7 +19,6 @@

import org.hamcrest.Matchers;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -457,20 +456,18 @@ public void test1RowMvtMarginWithMultiColumnFeedsOnNtpHomePage() throws IOExcept
int expectedContainerTwoSideMarginLandscape =
res.getDimensionPixelSize(org.chromium.chrome.R.dimen.ntp_search_box_start_margin)
* 2
+ res.getDimensionPixelSize(
org.chromium.chrome.R.dimen
.mvt_container_to_ntp_right_extra_margin_two_feed_tablet)
+ res.getDimensionPixelSize(org.chromium.chrome.R.dimen.tile_grid_layout_bleed) / 2
* 2;
int expectedContainerTwoSideMarginPortrait =
res.getDimensionPixelSize(org.chromium.chrome.R.dimen.tile_grid_layout_bleed) / 2
* 2
+ res.getDimensionPixelSize(
org.chromium.chrome.R.dimen
.mvt_container_to_ntp_right_extra_margin_two_feed_tablet);
* 2;
int expectedContainerRightExtraMargin = res.getDimensionPixelSize(
org.chromium.chrome.R.dimen
.mvt_container_to_ntp_right_extra_margin_two_feed_tablet);
// Verifies the margins of the module most visited tiles and its inner view are correct.
verifyMostVisitedTileMargin(expectedContainerTwoSideMarginLandscape,
expectedContainerTwoSideMarginPortrait, 0, 0, /*isScrollable=*/true, ntp);
expectedContainerTwoSideMarginPortrait, expectedContainerRightExtraMargin, 0, 0,
/*isScrollable=*/true, ntp);

int expectedMvtBottomMargin = res.getDimensionPixelSize(
org.chromium.chrome.R.dimen.mvt_container_bottom_margin_tablet);
Expand Down Expand Up @@ -541,7 +538,7 @@ public void test2RowMvtMarginWithMultiColumnFeedsOnNtpHomePage() throws IOExcept
org.chromium.chrome.R.dimen.tile_grid_layout_portrait_edge_margin_tablet);
// Verifies the margins of the module most visited tiles and its inner view are correct.
verifyMostVisitedTileMargin(expectedContainerTwoSideMargin, expectedContainerTwoSideMargin,
expectedLandScapeEdgeMargin, expectedPortraitEdgeMargin, /*isScrollable=*/false,
0, expectedLandScapeEdgeMargin, expectedPortraitEdgeMargin, /*isScrollable=*/false,
ntp);

int expectedMvtBottomMargin = res.getDimensionPixelSize(
Expand Down Expand Up @@ -596,6 +593,9 @@ public void testClickSingleTabCardCloseNtpHomeSurface() throws IOException {
* most visited tiles container when the tablet is in landscape.
* @param expectedContainerTwoSideMarginPortrait The expected sum of two side margins of the
* most visited tiles container when the tablet is in portrait.
* @param expectedContainerRightExtraMargin The extra value might be added to the right margin
* of the most visited tiles container when there is a half-tile element at the end of
* the scrollable most visited tiles.
* @param expectedEdgeMarginLandScape The expected edge margin of the most visited tiles element
* to the MV tiles layout when the tablet is in landscape.
* @param expectedEdgeMarginPortrait The expected edge margin of the most visited tiles element
Expand All @@ -604,10 +604,10 @@ public void testClickSingleTabCardCloseNtpHomeSurface() throws IOException {
* @param ntp The current {@link NewTabPage}.
*/
private void verifyMostVisitedTileMargin(int expectedContainerTwoSideMarginLandScape,
int expectedContainerTwoSideMarginPortrait, int expectedEdgeMarginLandScape,
int expectedEdgeMarginPortrait, boolean isScrollable, NewTabPage ntp) {
int expectedContainerTwoSideMarginPortrait, int expectedContainerRightExtraMargin,
int expectedEdgeMarginLandScape, int expectedEdgeMarginPortrait, boolean isScrollable,
NewTabPage ntp) {
NewTabPageLayout ntpLayout = ntp.getNewTabPageLayout();
Assume.assumeTrue(ntpLayout.getChildCount() >= 4);
View mvTilesContainer =
ntpLayout.findViewById(org.chromium.chrome.test.R.id.mv_tiles_container);
View mvTilesLayout = ntpLayout.findViewById(org.chromium.chrome.test.R.id.mv_tiles_layout);
Expand All @@ -621,17 +621,19 @@ private void verifyMostVisitedTileMargin(int expectedContainerTwoSideMarginLandS
waitForScreenOrientation("\"landscape\"");
// Verifies the margins added for the most visited tiles are correct.
verifyMostVisitedTileMarginImpl(ntpLayout, mvTilesContainer, mvTilesLayout, mvTileItem1,
mvTileItem2, expectedContainerTwoSideMarginLandScape, expectedEdgeMarginLandScape,
mvTilesItemWidth, isScrollable);
mvTileItem2, expectedContainerTwoSideMarginLandScape,
expectedContainerRightExtraMargin, expectedEdgeMarginLandScape, mvTilesItemWidth,
isScrollable);

// Start off in portrait screen orientation.
mActivityTestRule.getActivity().setRequestedOrientation(
ActivityInfo.SCREEN_ORIENTATION_PORTRAIT);
waitForScreenOrientation("\"portrait\"");
// Verifies the margins added for the most visited tiles are correct.
verifyMostVisitedTileMarginImpl(ntpLayout, mvTilesContainer, mvTilesLayout, mvTileItem1,
mvTileItem2, expectedContainerTwoSideMarginPortrait, expectedEdgeMarginPortrait,
mvTilesItemWidth, isScrollable);
mvTileItem2, expectedContainerTwoSideMarginPortrait,
expectedContainerRightExtraMargin, expectedEdgeMarginPortrait, mvTilesItemWidth,
isScrollable);
}

/**
Expand All @@ -644,35 +646,64 @@ private void verifyMostVisitedTileMargin(int expectedContainerTwoSideMarginLandS
* @param mvTileItem2 The second element of the most visited tile.
* @param expectedContainerTwoSideMargin The expected sum of two side margins of the
* most visited tiles container.
* @param expectedContainerRightExtraMargin The extra value might be added to the right margin
* of the most visited tiles container when there is
* a half-tile element at the end of the scrollable
* most visited tiles.
* @param expectedEdgeMargin The expected edge margin of the most visited tiles element
* to the MV tiles layout.
* @param mvTilesItemWidth The width of the elements in the most visited tile.
* @param isScrollable Whether the most visited tiles is scrollable.
*/
private void verifyMostVisitedTileMarginImpl(View ntpLayout, View mvTilesContainer,
View mvTilesLayout, View mvTileItem1, View mvTileItem2,
int expectedContainerTwoSideMargin, int expectedEdgeMargin, int mvTilesItemWidth,
boolean isScrollable) {
int expectedContainerTwoSideMargin, int expectedContainerRightExtraMargin,
int expectedEdgeMargin, int mvTilesItemWidth, boolean isScrollable) {
int mvtContainerWidth = mvTilesContainer.getWidth();
int mvTilesLayoutWidth = mvTilesLayout.getWidth();
int mvt1LeftMargin = ((MarginLayoutParams) mvTileItem1.getLayoutParams()).leftMargin;
int mvt2LeftMargin = ((MarginLayoutParams) mvTileItem2.getLayoutParams()).leftMargin;

Assert.assertEquals("The container's margin with respect to the layout of the new tab "
+ "page is incorrect.",
expectedContainerTwoSideMargin, ntpLayout.getWidth() - mvtContainerWidth, 3);

if (isScrollable) {
Assert.assertTrue("The width of the most visited tiles layout is wrong.",
mvtContainerWidth < mvTilesLayoutWidth);
int mvtWithPadding = mvTilesItemWidth + mvt2LeftMargin;
int visibleMvtNum = mvtContainerWidth / mvtWithPadding;
Assert.assertEquals("It fails to meet the requirement that half of "
+ "the most visited tiles element should be at the end of the MV tiles "
+ "when the new tab page is initially loaded.",
mvtContainerWidth - visibleMvtNum * mvtWithPadding, mvTilesItemWidth / 2,
mvTilesItemWidth / 20);
mvtContainerWidth <= mvTilesLayoutWidth);
int tileNum =
((ViewGroup) ntpLayout.findViewById(org.chromium.chrome.R.id.mv_tiles_layout))
.getChildCount();
int minIntervalMargin = ntpLayout.getResources().getDimensionPixelOffset(
org.chromium.chrome.R.dimen.tile_carousel_layout_min_interval_margin_tablet);
boolean isHalfMvt = tileNum * mvTilesItemWidth + (tileNum - 1) * minIntervalMargin
> mvtContainerWidth;
if (isHalfMvt) {
Assert.assertEquals(
"The container's margin with respect to the layout of the new tab "
+ "page is incorrect.",
expectedContainerTwoSideMargin + expectedContainerRightExtraMargin,
ntpLayout.getWidth() - mvtContainerWidth, 3);
int mvtWithPadding = mvTilesItemWidth + mvt2LeftMargin;
int visibleMvtNum = mvtContainerWidth / mvtWithPadding;
Assert.assertEquals("It fails to meet the requirement that half of "
+ "the most visited tiles element should be at the end of the MV "
+ "tiles when the new tab page is initially loaded with too many "
+ "tile elements.",
mvtContainerWidth - visibleMvtNum * mvtWithPadding, mvTilesItemWidth / 2,
mvTilesItemWidth / 20);
} else {
Assert.assertTrue(
"The container's margin with respect to the layout of the new tab "
+ "page is incorrect.",
expectedContainerTwoSideMargin <= ntpLayout.getWidth() - mvtContainerWidth);
Assert.assertEquals("It fails to meet the requirement that all of "
+ "the most visited tiles element should be fitted in the MV tiles "
+ "when the new tab page is initially loaded without too many tile "
+ "elements.",
mvtContainerWidth,
tileNum * mvTilesItemWidth + (tileNum - 1) * mvt2LeftMargin);
}
} else {
Assert.assertEquals("The container's margin with respect to the "
+ "layout of the new tab page is incorrect.",
expectedContainerTwoSideMargin, ntpLayout.getWidth() - mvtContainerWidth, 3);
Assert.assertTrue("The width of the most visited tiles layout is wrong.",
mvtContainerWidth == mvTilesLayoutWidth);
int minHorizontalSpacing = ((MostVisitedTilesGridLayout) mvTilesLayout)
Expand All @@ -685,7 +716,7 @@ private void verifyMostVisitedTileMarginImpl(View ntpLayout, View mvTilesContain
int expectedIntervalPadding =
Math.round((float) (mvTilesLayoutWidth - mvTilesItemWidth * numColumns
- expectedEdgeMargin * 2)
/ (numColumns - 1));
/ Math.max(1, numColumns - 1));
if (expectedIntervalPadding >= minHorizontalSpacing
&& expectedIntervalPadding <= maxHorizontalSpacing) {
Assert.assertEquals("The edge margin of the most visited tiles element to "
Expand Down
3 changes: 2 additions & 1 deletion chrome/android/java/res/values/dimens.xml
Expand Up @@ -165,7 +165,8 @@ found in the LICENSE file.
<dimen name="mvt_container_bottom_margin_tablet">48dp</dimen>
<dimen name="tile_grid_layout_landscape_edge_margin_tablet">100dp</dimen>
<dimen name="tile_grid_layout_portrait_edge_margin_tablet">84dp</dimen>
<dimen name="tile_carousel_layout_max_interval_margin_tablet">64dp</dimen>
<dimen name="tile_carousel_layout_max_interval_margin_tablet">48dp</dimen>
<dimen name="tile_carousel_layout_min_interval_margin_tablet">16dp</dimen>
<dimen name="single_tab_card_top_margin_tablet">48dp</dimen>
<dimen name="single_tab_card_bottom_margin_tablet">40dp</dimen>
<dimen name="ntp_iph_searchbox_y_inset">6dp</dimen>
Expand Down
Expand Up @@ -82,9 +82,8 @@ public class NewTabPageLayout extends LinearLayout {
private final Context mContext;
private int mSearchBoxEndPadding = UNSET_RESOURCE_FLAG;

private final int mMvtLandscapeLeftMarginTablet;
private final int mMvtLandscapeRightMarginTablet;
private final int mMvtPortraitRightMarginTablet;
private final int mMvtLandscapeLateralMarginTablet;
private final int mMvtExtraRightMarginTablet;

private View mMiddleSpacer; // Spacer between toolbar and Most Likely.

Expand Down Expand Up @@ -142,6 +141,12 @@ public class NewTabPageLayout extends LinearLayout {

private NewTabPageUma mNewTabPageUma;

private int mTileViewWidth;
private int mTileViewMinIntervalPaddingTablet;
private Integer mInitialTileNum;
private Boolean mIsHalfMvtLandscape;
private Boolean mIsHalfMvtPortrait;

/**
* Constructor for inflating from XML.
*/
Expand All @@ -150,12 +155,14 @@ public NewTabPageLayout(Context context, AttributeSet attrs) {
mContext = context;
Resources res = getResources();
mTileGridLayoutBleed = res.getDimensionPixelSize(R.dimen.tile_grid_layout_bleed);
mMvtLandscapeLeftMarginTablet =
mMvtLandscapeLateralMarginTablet =
res.getDimensionPixelSize(R.dimen.ntp_search_box_start_margin);
mMvtPortraitRightMarginTablet = res.getDimensionPixelSize(
mMvtExtraRightMarginTablet = res.getDimensionPixelSize(
R.dimen.mvt_container_to_ntp_right_extra_margin_two_feed_tablet);
mMvtLandscapeRightMarginTablet =
mMvtLandscapeLeftMarginTablet + mMvtPortraitRightMarginTablet;
mTileViewWidth =
getResources().getDimensionPixelOffset(org.chromium.chrome.R.dimen.tile_view_width);
mTileViewMinIntervalPaddingTablet = getResources().getDimensionPixelOffset(
org.chromium.chrome.R.dimen.tile_carousel_layout_min_interval_margin_tablet);
}

@Override
Expand Down Expand Up @@ -451,10 +458,45 @@ public View getSearchBoxView() {

@Override
protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
if (mIsNtpAsHomeSurfaceEnabled && mIsMultiColumnFeedEnabled && isScrollableMvtEnabled()) {
calculateTabletMvtMargin(MeasureSpec.getSize(widthMeasureSpec));
}
super.onMeasure(widthMeasureSpec, heightMeasureSpec);
unifyElementWidths();
}

/**
* Update the right margin for the MV tiles container if needed to have half tile element
* in the end of the MV tiles when used in NTP on the tablet.
*/
private void calculateTabletMvtMargin(int widthMeasureSpec) {
if (mMvTilesContainerLayout.getVisibility() == GONE) return;

if (mInitialTileNum == null) {
mInitialTileNum = ((ViewGroup) findViewById(R.id.mv_tiles_layout)).getChildCount();
}

int currentOrientation = getResources().getConfiguration().orientation;
if ((currentOrientation == Configuration.ORIENTATION_LANDSCAPE
&& mIsHalfMvtLandscape == null)
|| (currentOrientation == Configuration.ORIENTATION_PORTRAIT
&& mIsHalfMvtPortrait == null)) {
MarginLayoutParams marginLayoutParams =
(MarginLayoutParams) mMvTilesContainerLayout.getLayoutParams();
int mvtContainerWidth = widthMeasureSpec - marginLayoutParams.leftMargin
- marginLayoutParams.rightMargin;
boolean isHalfMvt = mInitialTileNum * mTileViewWidth
+ (mInitialTileNum - 1) * mTileViewMinIntervalPaddingTablet
> mvtContainerWidth;
if (currentOrientation == Configuration.ORIENTATION_LANDSCAPE) {
mIsHalfMvtLandscape = isHalfMvt;
} else {
mIsHalfMvtPortrait = isHalfMvt;
}
updateTilesLayoutLeftAndRightMarginsOnTablet(marginLayoutParams);
}
}

public void onSwitchToForeground() {
if (mMostVisitedTilesCoordinator != null) {
mMostVisitedTilesCoordinator.onSwitchToForeground();
Expand Down Expand Up @@ -524,7 +566,6 @@ private void updateTilesLayoutMargins() {
// Let mMvTilesContainerLayout attached to the edge of the screen.
setClipToPadding(false);
if (mIsNtpAsHomeSurfaceEnabled && mIsMultiColumnFeedEnabled) {
((LayoutParams) marginLayoutParams).gravity = Gravity.START;
updateTilesLayoutLeftAndRightMarginsOnTablet(marginLayoutParams);
} else {
int lateralPaddingsForNTP = -getResources().getDimensionPixelSize(
Expand Down Expand Up @@ -955,13 +996,19 @@ public void onConfigurationChanged(Configuration newConfig) {
*/
private void updateTilesLayoutLeftAndRightMarginsOnTablet(
MarginLayoutParams marginLayoutParams) {
((LayoutParams) marginLayoutParams).gravity = Gravity.CENTER_HORIZONTAL;
int leftMarginForNtp = mTileGridLayoutBleed / 2;
int rightMarginForNtp = leftMarginForNtp;
if (getResources().getConfiguration().orientation == Configuration.ORIENTATION_LANDSCAPE) {
leftMarginForNtp = leftMarginForNtp + mMvtLandscapeLeftMarginTablet;
rightMarginForNtp = rightMarginForNtp + mMvtLandscapeRightMarginTablet;
} else {
rightMarginForNtp = rightMarginForNtp + mMvtPortraitRightMarginTablet;
leftMarginForNtp = leftMarginForNtp + mMvtLandscapeLateralMarginTablet;
rightMarginForNtp = rightMarginForNtp + mMvtLandscapeLateralMarginTablet;
if (mIsHalfMvtLandscape != null && mIsHalfMvtLandscape) {
((LayoutParams) marginLayoutParams).gravity = Gravity.START;
rightMarginForNtp = rightMarginForNtp + mMvtExtraRightMarginTablet;
}
} else if (mIsHalfMvtPortrait != null && mIsHalfMvtPortrait) {
((LayoutParams) marginLayoutParams).gravity = Gravity.START;
rightMarginForNtp = rightMarginForNtp + mMvtExtraRightMarginTablet;
}
marginLayoutParams.leftMargin = leftMarginForNtp;
marginLayoutParams.rightMargin = rightMarginForNtp;
Expand Down

0 comments on commit 2af9202

Please sign in to comment.