Skip to content

Commit

Permalink
[Tab Strip Redesign] Add param to disable button style
Browse files Browse the repository at this point in the history
Param(enable remove button style) currently includes remove button bg color and disable btn anchor. Alternatively we can make disable btn anchor as a new feature flag if we want to experiment with these two things individually.

Enabled Detached Remove Button Style: https://drive.google.com/file/d/1-buSoWLsYeWZSG7VW6xhUgqGrrXQ78GW/view?usp=sharing
Enabled Folio Remove Button Style: https://drive.google.com/file/d/1-_-o-HyhsxYdevuKeLZmIvlm8JZ438hf/view?usp=sharing

(cherry picked from commit cc2ae25)

Bug: 1447109
Change-Id: I062beec1486c2f6b29086beabe78b7180d4f15ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4545166
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Neil Coronado <nemco@google.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1148744}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4571180
Commit-Queue: Neil Coronado <nemco@google.com>
Auto-Submit: Theresa Sullivan <twellington@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#74}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Zhe Li authored and Chromium LUCI CQ committed May 26, 2023
1 parent db97cc8 commit 0fddfbf
Show file tree
Hide file tree
Showing 11 changed files with 164 additions and 53 deletions.
Expand Up @@ -71,6 +71,13 @@ public class TabUiFeatureUtilities {
new BooleanCachedFieldTrialParameter(ChromeFeatureList.TAB_STRIP_REDESIGN,
TAB_STRIP_REDESIGN_DISABLE_NTB_ANCHOR_PARAM, false);

// Field trial parameter for disabling button style for tab strip redesign. This includes
// disabling NTB anchor and button bg style.
private static final String TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE_PARAM = "disable_btn_style";
public static final BooleanCachedFieldTrialParameter TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE =
new BooleanCachedFieldTrialParameter(ChromeFeatureList.TAB_STRIP_REDESIGN,
TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE_PARAM, false);

private static boolean sTabSelectionEditorLongPressEntryEnabled;

/**
Expand All @@ -87,6 +94,13 @@ public static boolean isTabStripNtbAnchorDisabled() {
return TAB_STRIP_REDESIGN_DISABLE_NTB_ANCHOR.getValue();
}

/**
* @return Whether button style for tab strip redesign is disabled.
*/
public static boolean isTabStripButtonStyleDisabled() {
return TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE.getValue();
}

/**
* Whether the longpress entry for TabSelectionEditor is enabled. Currently only in tests.
*/
Expand Down
Expand Up @@ -110,6 +110,7 @@ public void cacheNativeFlags() {
TabUiFeatureUtilities.ZOOMING_MIN_MEMORY, TabUiFeatureUtilities.SKIP_SLOW_ZOOMING,
TabUiFeatureUtilities.THUMBNAIL_ASPECT_RATIO,
TabUiFeatureUtilities.TAB_STRIP_REDESIGN_DISABLE_NTB_ANCHOR,
TabUiFeatureUtilities.TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE,
TabManagementFieldTrial.DELAY_TEMP_STRIP_TIMEOUT_MS,
TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_FOLIO,
TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_DETACHED,
Expand Down
Expand Up @@ -1707,14 +1707,20 @@ private void createRenderList() {
}
}

private boolean isNewTabButtonAnchorDisabled() {
return !ChromeFeatureList.sTabStripRedesign.isEnabled()
|| TabUiFeatureUtilities.isTabStripNtbAnchorDisabled()
|| TabUiFeatureUtilities.isTabStripButtonStyleDisabled();
}

// @Todo (crbugs.com/1448590) Update NTB and incognito position for button style disabled param.
private CompositorAnimator updateNewTabButtonState(boolean animate) {
// 1. The NTB is faded out upon entering reorder mode and hidden when the model is empty.
boolean isEmpty = mStripTabs.length == 0;
mNewTabButton.setVisible(!isEmpty);
if (mInReorderMode || isEmpty) return null;

if (!ChromeFeatureList.sTabStripRedesign.isEnabled()
|| TabUiFeatureUtilities.isTabStripNtbAnchorDisabled()) {
if (isNewTabButtonAnchorDisabled()) {
// 2. Get offset from strip stacker.
float offset = mStripStacker.computeNewTabButtonOffset(mStripTabs, mTabOverlapWidth,
mLeftMargin, mRightMargin, mWidth, mNewTabButtonWidth,
Expand Down
Expand Up @@ -13,6 +13,7 @@

import androidx.annotation.ColorInt;
import androidx.annotation.VisibleForTesting;
import androidx.appcompat.content.res.AppCompatResources;

import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.supplier.Supplier;
Expand Down Expand Up @@ -272,12 +273,6 @@ public void onClick(long time) {
createDetachedModelSelectorButton(context, selectorClickHandler);
}

// Model selector button icon color
int iconDefaultColor =
context.getResources().getColor(R.color.model_selector_button_icon_color);
int iconIncognitoColor =
context.getResources().getColor(R.color.default_icon_color_secondary_light);

// Model selector button background color.
// Default bg color is surface inverse.
int backgroundDefaultColor =
Expand All @@ -294,9 +289,21 @@ public void onClick(long time) {
? context.getResources().getColor(defaultBgColorDarkElev1)
: context.getResources().getColor(defaultBgColorDarkElev2);

// Model selector button icon color
// @Todo(zheliooo crbugs.com/1447285) may need to update color using GM3 and update MSB
// icon per UX suggestion. Temporarily set MSB color match NTB color in normal mode
int iconDefaultColor = TabUiFeatureUtilities.isTabStripButtonStyleDisabled()
? AppCompatResources
.getColorStateList(context, R.color.default_icon_color_tint_list)
.getDefaultColor()
: context.getResources().getColor(R.color.model_selector_button_icon_color);
int iconIncognitoColor =
context.getResources().getColor(R.color.default_icon_color_secondary_light);

((TintedCompositorButton) mModelSelectorButton)
.setTint(iconDefaultColor, iconDefaultColor, iconIncognitoColor,
iconIncognitoColor);

((TintedCompositorButton) mModelSelectorButton)
.setBackgroundTint(backgroundDefaultColor, backgroundDefaultColor,
backgroundIncognitoColor, backgroundIncognitoColor);
Expand Down
Expand Up @@ -19,6 +19,7 @@
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.layouts.scene_layer.SceneLayer;
import org.chromium.chrome.browser.layouts.scene_layer.SceneOverlayLayer;
import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities;
import org.chromium.ui.base.LocalizationUtils;
import org.chromium.ui.resources.ResourceManager;

Expand All @@ -44,8 +45,9 @@ public static void setTestFlag(boolean testFlag) {
@Override
protected void initializeNative() {
if (mNativePtr == 0) {
mNativePtr = TabStripSceneLayerJni.get().init(
TabStripSceneLayer.this, ChromeFeatureList.sTabStripRedesign.isEnabled());
mNativePtr = TabStripSceneLayerJni.get().init(TabStripSceneLayer.this,
ChromeFeatureList.sTabStripRedesign.isEnabled(),
TabUiFeatureUtilities.isTabStripButtonStyleDisabled());
}
// Set flag for testing
if (!sTestFlag) {
Expand Down Expand Up @@ -177,7 +179,8 @@ public void destroy() {

@NativeMethods
public interface Natives {
long init(TabStripSceneLayer caller, boolean isTabStripRedesignEnabled);
long init(TabStripSceneLayer caller, boolean isTabStripRedesignEnabled,
boolean isTsrButtonStyleDisabled);
void beginBuildingFrame(
long nativeTabStripSceneLayer, TabStripSceneLayer caller, boolean visible);
void finishBuildingFrame(long nativeTabStripSceneLayer, TabStripSceneLayer caller);
Expand Down
Expand Up @@ -9,6 +9,7 @@
import android.content.Context;
import android.view.ContextThemeWrapper;

import androidx.appcompat.content.res.AppCompatResources;
import androidx.test.core.app.ApplicationProvider;

import org.junit.After;
Expand All @@ -31,13 +32,16 @@
import org.chromium.chrome.browser.compositor.layouts.LayoutManagerHost;
import org.chromium.chrome.browser.compositor.layouts.LayoutRenderHost;
import org.chromium.chrome.browser.compositor.layouts.LayoutUpdateHost;
import org.chromium.chrome.browser.compositor.layouts.components.TintedCompositorButton;
import org.chromium.chrome.browser.compositor.overlays.strip.StripLayoutHelperManager.TabModelStartupInfo;
import org.chromium.chrome.browser.compositor.scene_layer.TabStripSceneLayer;
import org.chromium.chrome.browser.compositor.scene_layer.TabStripSceneLayerJni;
import org.chromium.chrome.browser.flags.BooleanCachedFieldTrialParameter;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.lifecycle.ActivityLifecycleDispatcher;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tasks.tab_management.TabManagementFieldTrial;
import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.components.browser_ui.styles.ChromeColors;
import org.chromium.ui.base.LocalizationUtils;
Expand Down Expand Up @@ -97,17 +101,11 @@ private void initializeTest() {
mLifecycleDispatcher);
}

private void initializeFolioTest() {
// Since we check TSR arm and determine model selector button width inside constructor, so
// need to set TSR arm before initialize test.
TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_FOLIO.setForTesting(true);
initializeTest();
}

private void initializeDetachedTest() {
// Since we check TSR arm and determine model selector button width inside constructor, so
// need to set TSR arm before initialize test.
TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_DETACHED.setForTesting(true);
private void initializeTestWithTsrArm(BooleanCachedFieldTrialParameter param) {
// Since we check TSR arm and determine model selector button properties(eg. color/bg color,
// width, etc) inside constructor, so need to set TSR arm before initialize test each time
// we switch arm.
param.setForTesting(true);
initializeTest();
}

Expand All @@ -133,7 +131,7 @@ public void testGetBackgroundColorFolio() {
@Feature("Tab Strip Redesign")
public void testModelSelectorButtonPosition_Folio() {
// setup
initializeFolioTest();
initializeTestWithTsrArm(TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_FOLIO);

// Set model selector button position.
mStripLayoutHelperManager.onSizeChanged(
Expand All @@ -149,7 +147,7 @@ public void testModelSelectorButtonPosition_Folio() {
@Feature("Tab Strip Redesign")
public void testModelSelectorButtonPosition_RTL_Folio() {
// setup
initializeFolioTest();
initializeTestWithTsrArm(TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_FOLIO);

// Set model selector button position.
LocalizationUtils.setRtlForTesting(true);
Expand All @@ -165,7 +163,7 @@ public void testModelSelectorButtonPosition_RTL_Folio() {
@Feature("Tab Strip Redesign")
public void testModelSelectorButtonPosition_Detached() {
// setup
initializeDetachedTest();
initializeTestWithTsrArm(TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_DETACHED);

// Set model selector button position.
mStripLayoutHelperManager.onSizeChanged(
Expand All @@ -181,7 +179,7 @@ public void testModelSelectorButtonPosition_Detached() {
@Feature("Tab Strip Redesign")
public void testModelSelectorButtonPosition_RTL_Detached() {
// setup
initializeDetachedTest();
initializeTestWithTsrArm(TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_DETACHED);

// Set model selector button position.
LocalizationUtils.setRtlForTesting(true);
Expand Down Expand Up @@ -292,6 +290,30 @@ public void testFadeDrawable_Right_RTL_TSR() {
mStripLayoutHelperManager.getRightFadeDrawable());
}

@Test
@Feature("Tab Strip Redesign")
public void testButtonIconColor() {
// Verify TSR button icon color.
assertEquals("Unexpected incognito button color.",
mContext.getResources().getColor(R.color.model_selector_button_icon_color),
((TintedCompositorButton) mStripLayoutHelperManager.getModelSelectorButton())
.getTint());
}

@Test
@Feature("Tab Strip Redesign")
public void testButtonIconColor_DisableButtonStyle() {
// setup
initializeTestWithTsrArm(TabUiFeatureUtilities.TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE);

// Verify TSR button icon color after disabling button style.
assertEquals("Unexpected incognito button color.",
AppCompatResources.getColorStateList(mContext, R.color.default_icon_color_tint_list)
.getDefaultColor(),
((TintedCompositorButton) mStripLayoutHelperManager.getModelSelectorButton())
.getTint());
}

@Test
@Feature("TabStripPerformance")
public void testSetTabModelStartupInfo() {
Expand Down
Expand Up @@ -27,10 +27,14 @@
import android.content.res.Resources;
import android.text.TextUtils;
import android.util.DisplayMetrics;
import android.view.ContextThemeWrapper;
import android.view.HapticFeedbackConstants;
import android.view.View;
import android.view.ViewGroup.MarginLayoutParams;

import androidx.appcompat.content.res.AppCompatResources;
import androidx.test.core.app.ApplicationProvider;

import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -94,6 +98,7 @@ public class StripLayoutHelperTest {
private StripLayoutHelperManager mStripLayoutHelperManager;

private Activity mActivity;
private Context mContext;
private TestTabModel mModel = new TestTabModel();
private StripLayoutHelper mStripLayoutHelper;
private boolean mIncognito;
Expand Down Expand Up @@ -136,6 +141,8 @@ public void beforeTest() {
MockitoAnnotations.initMocks(this);
when(mModelSelectorBtn.isVisible()).thenReturn(true);
when(mTabGroupModelFilter.hasOtherRelatedTabs(any())).thenReturn(false);
mContext = new ContextThemeWrapper(
ApplicationProvider.getApplicationContext(), R.style.Theme_BrowserUI_DayNight);

mActivity = Robolectric.setupActivity(Activity.class);
mActivity.setTheme(org.chromium.chrome.R.style.Theme_BrowserUI);
Expand Down Expand Up @@ -770,6 +777,30 @@ public void testNewTabButtonPosition_NotAnchored_Rtl() {
mStripLayoutHelper.getNewTabButton().getX(), EPSILON);
}

@Test
@Feature("Tab Strip Redesign")
public void testNewTabButtonStyle_ButtonStyleDisabled() {
// Setup
TabManagementFieldTrial.TAB_STRIP_REDESIGN_ENABLE_FOLIO.setForTesting(true);
TabUiFeatureUtilities.TAB_STRIP_REDESIGN_DISABLE_BUTTON_STYLE.setForTesting(true);
int tabCount = 1;
initializeTest(false, false, false, 0, tabCount);
mStripLayoutHelper.onSizeChanged(SCREEN_WIDTH, SCREEN_HEIGHT, false, TIMESTAMP);
mStripLayoutHelper.updateLayout(TIMESTAMP);

// Verify new tab button position.
// tabWidth(237) + tabOverLapWidth(28) + folioXOffset(6) = 271
assertEquals("New tab button position is not as expected", 271.f,
mStripLayoutHelper.getNewTabButton().getX(), EPSILON);

assertEquals("Unexpected incognito button color.",
AppCompatResources.getColorStateList(mContext, R.color.default_icon_color_tint_list)
.getDefaultColor(),
((org.chromium.chrome.browser.compositor.layouts.components.TintedCompositorButton)
mStripLayoutHelper.getNewTabButton())
.getTint());
}

@Test
public void testScrollOffset_OnResume_StartOnLeft_SelectedRightmostTab() {
// Arrange: Initialize tabs with last tab selected.
Expand Down
Expand Up @@ -103,7 +103,7 @@ public void tearDown() {
private void initializeTest() {
mTabStripSceneLayer = new TabStripSceneLayer(mContext);
when(mTabStripSceneMock.init(
mTabStripSceneLayer, ChromeFeatureList.sTabStripRedesign.isEnabled()))
mTabStripSceneLayer, ChromeFeatureList.sTabStripRedesign.isEnabled(), false))
.thenReturn(1L);
mModelSelectorButton = new TintedCompositorButton(
mContext, 36.f, 36.f, mCompositorOnClickHandler, R.drawable.ic_new_tab_button);
Expand Down
12 changes: 11 additions & 1 deletion chrome/browser/about_flags.cc
Expand Up @@ -3154,6 +3154,11 @@ const FeatureEntry::FeatureParam kTabStripRedesignDisableNtbAnchorFolio[] = {
const FeatureEntry::FeatureParam kTabStripRedesignDisableNtbAnchorDetached[] = {
{"disable_ntb_anchor", "true"},
{"enable_detached", "true"}};
const FeatureEntry::FeatureParam kTabStripRedesignDisableButtonStyleFolio[] = {
{"disable_btn_style", "true"},
{"enable_folio", "true"}};
const FeatureEntry::FeatureParam kTabStripRedesignDisableButtonStyleDetached[] =
{{"disable_btn_style", "true"}, {"enable_detached", "true"}};

const FeatureEntry::FeatureVariation kTabStripRedesignVariations[] = {
{"Folio", kTabStripRedesignFolio, std::size(kTabStripRedesignFolio),
Expand All @@ -3163,7 +3168,12 @@ const FeatureEntry::FeatureVariation kTabStripRedesignVariations[] = {
{"Folio NTB Unanchored ", kTabStripRedesignDisableNtbAnchorFolio,
std::size(kTabStripRedesignDisableNtbAnchorFolio), nullptr},
{"Detached NTB Unanchored", kTabStripRedesignDisableNtbAnchorDetached,
std::size(kTabStripRedesignDisableNtbAnchorDetached), nullptr}};
std::size(kTabStripRedesignDisableNtbAnchorDetached), nullptr},
{"Folio Remove Button Style", kTabStripRedesignDisableButtonStyleFolio,
std::size(kTabStripRedesignDisableButtonStyleFolio), nullptr},
{"Detached Remove Button Style",
kTabStripRedesignDisableButtonStyleDetached,
std::size(kTabStripRedesignDisableButtonStyleDetached), nullptr}};
#endif // BUILDFLAG(IS_ANDROID)

#if !BUILDFLAG(IS_ANDROID) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
Expand Down

0 comments on commit 0fddfbf

Please sign in to comment.