Avoid overriding explicitly-specified theme colors.

We originally did this to try and ensure tabs had sufficient contrast to be
readable in all cases.  However, this caused problems if the background colors
were not correctly calculated, and also limited theme authors' ability to make
aesthetic choices (or override us if we got something wrong).

Instead, use the specified colors directly.  Generate unspecified inactive tab
colors by blending with the background, as we do for the default theme.  No
changes to how the default theme's colors are calculated.

Bug: 904372
Change-Id: I90d96afbf3a1e9b095b0860a64aa044fecdad6d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1755142
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687315}

chrome/browser/themes/browser_theme_pack.cc

@@ -62,7 +62,7 @@ constexpr int kTallestFrameHeight = kTallestTabHeight + 19;
 // change default theme assets, if you need themes to recreate their generated
 // images (which are cached), or if you changed how missing values are
 // generated.
-const int kThemePackVersion = 68;
+const int kThemePackVersion = 69;
 
 // IDs that are in the DataPack won't clash with the positive integer
 // uint16_t. kHeaderID should always have the maximum value because we want the
@@ -1010,11 +1010,6 @@ void BrowserThemePack::AdjustThemePack() {
   // creating these.
   CreateTabBackgroundImagesAndColors(&images_);
 
-  // Generate any missing text colors.  This must be done after generating frame
-  // and tab colors, as generated text colors will try to appropriately contrast
-  // with the frame/tab behind them.
-  GenerateMissingTextColors();
-
   // Make sure the |images_on_file_thread_| has bitmaps for supported
   // scale factors before passing to FILE thread.
   images_on_file_thread_ = images_;
@@ -1681,93 +1676,6 @@ void BrowserThemePack::CreateTabBackgroundImagesAndColors(ImageCache* images) {
   MergeImageCaches(temp_output, images);
 }
 
-void BrowserThemePack::GenerateMissingTextColors() {
-  constexpr int kDefaultSourceTextColorId = TP::COLOR_BACKGROUND_TAB_TEXT;
-
-  // Background Tab
-  GenerateMissingTextColorForID(TP::COLOR_BACKGROUND_TAB_TEXT,
-                                TP::COLOR_BACKGROUND_TAB, TP::COLOR_FRAME,
-                                kDefaultSourceTextColorId);
-
-  // Background Tab - Inactive
-  GenerateMissingTextColorForID(
-      TP::COLOR_BACKGROUND_TAB_TEXT_INACTIVE, TP::COLOR_BACKGROUND_TAB_INACTIVE,
-      TP::COLOR_FRAME_INACTIVE, kDefaultSourceTextColorId);
-
-  // Incognito
-  GenerateMissingTextColorForID(TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO,
-                                TP::COLOR_BACKGROUND_TAB_INCOGNITO,
-                                TP::COLOR_FRAME_INCOGNITO,
-                                kDefaultSourceTextColorId);
-
-  // Incognito - Inactive
-  GenerateMissingTextColorForID(
-      TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO_INACTIVE,
-      TP::COLOR_BACKGROUND_TAB_INCOGNITO_INACTIVE,
-      TP::COLOR_FRAME_INCOGNITO_INACTIVE,
-      TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO);
-}
-
-void BrowserThemePack::GenerateMissingTextColorForID(int text_color_id,
-                                                     int tab_color_id,
-                                                     int frame_color_id,
-                                                     int source_color_id) {
-  SkColor text_color, tab_color, frame_color;
-  color_utils::HSL tab_tint;
-
-  const bool has_text_color = GetColor(text_color_id, &text_color);
-  const bool has_tab_color = GetColor(tab_color_id, &tab_color);
-  const bool has_frame_color = GetColor(frame_color_id, &frame_color);
-
-  const bool has_tab_tint = GetTint(TP::TINT_BACKGROUND_TAB, &tab_tint);
-  const bool has_meaningful_tab_tint =
-      has_tab_tint && color_utils::IsHSLShiftMeaningful(tab_tint);
-
-  // If there is no tab color specified (also meaning there is no image), fall
-  // back to the frame color.
-  SkColor bg_color = (has_tab_color ? tab_color : frame_color);
-  const bool has_bg_color =
-      has_tab_color || has_frame_color || has_meaningful_tab_tint;
-
-  // If no bg color is set, we have nothing to blend against, so there's no way
-  // to do this calculation.
-  if (!has_bg_color)
-    return;
-
-  if (has_meaningful_tab_tint && !has_tab_color) {
-    // We need to tint the frame color, so if the theme didn't specify it, grab
-    // the default.
-    if (!has_frame_color) {
-      frame_color = TP::GetDefaultColor(TP::GetLookupID(frame_color_id));
-    }
-    bg_color = color_utils::HSLShift(frame_color, tab_tint);
-  }
-
-  // Determine the text color to start with, in order of preference:
-  // 1) The color specified by the theme (if it exists)
-  // 2) The color passed in to use as a source function (if it exists)
-  // 3) The default color for the text property
-  SkColor blend_source_color;
-  if (has_text_color) {
-    blend_source_color = text_color;
-  } else {
-    SkColor source_text_color;
-    if (GetColor(source_color_id, &source_text_color)) {
-      blend_source_color = source_text_color;
-    } else {
-      // GetDefaultColor() requires incognito-aware lookup, so we first have to
-      // get the appropriate lookup ID information.
-      TP::PropertyLookupPair lookup_pair = TP::GetLookupID(text_color_id);
-
-      blend_source_color = TP::GetDefaultColor(lookup_pair);
-    }
-  }
-
-  SetColor(
-      text_color_id,
-      color_utils::BlendForMinContrast(blend_source_color, bg_color).color);
-}
-
 void BrowserThemePack::GenerateMissingNtpColors() {
   // Calculate NTP text color based on NTP background.
   SkColor ntp_background_color;

chrome/browser/themes/browser_theme_pack.h

@@ -225,17 +225,6 @@ class BrowserThemePack : public CustomThemeSupplier {
   // color has been specified.  Must be called after GenerateFrameImages().
   void CreateTabBackgroundImagesAndColors(ImageCache* images);
 
-  // Generates any text colors which have not already been set.
-  void GenerateMissingTextColors();
-
-  // Generates text color for the specified id |text_color_id|, based on the
-  // background color of the tab |tab_color_id|, and using the color already
-  // defined for |source_color_id| as a starting point (if it exists).
-  void GenerateMissingTextColorForID(int text_color_id,
-                                     int tab_color_id,
-                                     int frame_color_id,
-                                     int source_color_id);
-
   // Generates missing NTP related colors.
   void GenerateMissingNtpColors();
 

chrome/browser/themes/browser_theme_pack_unittest.cc

@@ -777,159 +777,6 @@ TEST_F(BrowserThemePackTest, HiDpiThemeTest) {
   }
 }
 
-// Ensure that, given a theme that specifies background tab/text colors which
-// are too similar, the importing process modifies the text color so that it
-// maintains a minimum readable contrast ratio with the background.
-TEST_F(BrowserThemePackTest, TestBackgroundTabTextMinimumContrast) {
-  scoped_refptr<BrowserThemePack> pack(
-      new BrowserThemePack(CustomThemeSupplier::ThemeType::EXTENSION));
-  BuildTestExtensionTheme("theme_tabcontrast", pack.get());
-
-  // Check the contrast ratio of text/tab color pairs to make sure that they
-  // meet the minimum criteria for readable contrast ratio.
-  struct TabColorPair {
-    TP::OverwritableByUserThemeProperty tab_color_id;
-    TP::OverwritableByUserThemeProperty text_color_id;
-  };
-  const TabColorPair color_pairs_to_check[] = {
-      {TP::COLOR_BACKGROUND_TAB, TP::COLOR_BACKGROUND_TAB_TEXT},
-      {TP::COLOR_BACKGROUND_TAB_INACTIVE,
-       TP::COLOR_BACKGROUND_TAB_TEXT_INACTIVE},
-      {TP::COLOR_BACKGROUND_TAB_INCOGNITO,
-       TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO},
-      {TP::COLOR_BACKGROUND_TAB_INCOGNITO_INACTIVE,
-       TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO_INACTIVE},
-  };
-
-  for (const TabColorPair& current_pair : color_pairs_to_check) {
-    SkColor cur_tab_color;
-    SkColor cur_text_color;
-
-    pack->GetColor(current_pair.tab_color_id, &cur_tab_color);
-    pack->GetColor(current_pair.text_color_id, &cur_text_color);
-
-    float contrast_ratio =
-        color_utils::GetContrastRatio(cur_tab_color, cur_text_color);
-
-    EXPECT_GE(contrast_ratio, color_utils::kMinimumReadableContrastRatio);
-  }
-}
-
-// Ensure that, given a theme that specifies a frame color and a background text
-// color, but NO discrete tab background colors or images, the importing process
-// properly modified the text color so that it maintains a minimum readable
-// contrast ratio with the background.
-TEST_F(BrowserThemePackTest, TestBackgroundTabTextMinimumContrast_NoTabColor) {
-  // Build a theme from test file
-  // (theme_test_bgtabtext_notabcolor_singletextcolor).
-  base::FilePath theme_path = GetTestExtensionThemePath(
-      "theme_test_bgtabtext_notabcolor_singletextcolor");
-  scoped_refptr<BrowserThemePack> pack(
-      new BrowserThemePack(CustomThemeSupplier::ThemeType::EXTENSION));
-  BuildFromUnpackedExtension(theme_path, pack.get());
-
-  SkColor frame_color;
-  SkColor text_color;
-
-  pack->GetColor(TP::COLOR_FRAME, &frame_color);
-  pack->GetColor(TP::COLOR_BACKGROUND_TAB_TEXT, &text_color);
-
-  float contrast_ratio = color_utils::GetContrastRatio(frame_color, text_color);
-
-  EXPECT_GE(contrast_ratio, color_utils::kMinimumReadableContrastRatio);
-}
-
-// Ensure that, given a theme which only specifies a color for
-// COLOR_BACKGROUND_TAB_TEXT, that color is used for the other variants of
-// background tab text (inactive, incognito, and incognito+inactive).
-TEST_F(BrowserThemePackTest, TestBGTabTextColorAutoAssign) {
-  scoped_refptr<BrowserThemePack> pack(
-      new BrowserThemePack(CustomThemeSupplier::ThemeType::EXTENSION));
-  BuildTestExtensionTheme("theme_testinherittextcolor", pack.get());
-
-  // Verify that all background tab text colors match the color for background
-  // tab text.
-  VerifyColorsMatch(pack.get(), TP::COLOR_BACKGROUND_TAB_TEXT_INACTIVE,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-  VerifyColorsMatch(pack.get(), TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-  VerifyColorsMatch(pack.get(),
-                    TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO_INACTIVE,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-}
-
-// Ensure that, given a theme which only specifies colors for
-// COLOR_BACKGROUND_TAB_TEXT and COLOR_BACKGROUND_TAB_TEXT_INCOGNITO, those
-// colors are also used for their respective inactive variants.
-TEST_F(BrowserThemePackTest, TestBGTabTextColorAutoAssign_WithIncognito) {
-  scoped_refptr<BrowserThemePack> pack(
-      new BrowserThemePack(CustomThemeSupplier::ThemeType::EXTENSION));
-  BuildTestExtensionTheme("theme_testinherittextcolor_withincog", pack.get());
-
-  // Verify that background_inactive is getting its color from background, and
-  // background_incognito_inactive is getting its color from
-  // background_incognito.
-  VerifyColorsMatch(pack.get(), TP::COLOR_BACKGROUND_TAB_TEXT_INACTIVE,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-  VerifyColorsMatch(pack.get(),
-                    TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO_INACTIVE,
-                    TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO);
-}
-
-// Ensure that, given a theme which only specifies a color for
-// COLOR_BACKGROUND_TAB_TEXT and FRAME_COLOR (no discrete tab colors or images),
-// that the background tab text color is properly used for the other variants of
-// background tab text (inactive, incognito, and incognito+inactive).
-TEST_F(BrowserThemePackTest, TestBGTabTextColorAutoAssign_NoTabColor) {
-  // Build a theme from test file
-  // (theme_test_bgtabtext_notabcolor_singletextcolor_propagate).
-  // This theme specifies a color for frame_color, and background_tab_text, but
-  // no tab background colors, and no variants of background_tab_text.
-  base::FilePath theme_path = GetTestExtensionThemePath(
-      "theme_test_bgtabtext_notabcolor_singletextcolor_autoassign");
-  scoped_refptr<BrowserThemePack> pack(
-      new BrowserThemePack(CustomThemeSupplier::ThemeType::EXTENSION));
-  BuildFromUnpackedExtension(theme_path, pack.get());
-
-  // Verify that all background tab text colors match the color for background
-  // tab text.
-  VerifyColorsMatch(pack.get(), TP::COLOR_BACKGROUND_TAB_TEXT_INACTIVE,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-  VerifyColorsMatch(pack.get(), TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-  VerifyColorsMatch(pack.get(),
-                    TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO_INACTIVE,
-                    TP::COLOR_BACKGROUND_TAB_TEXT);
-}
-
-// Ensure that, given a theme which specifies a background tab tint, but no
-// background tab color, tab text is correctly calculated to ensure contrast
-// against the (tinted) background tab color.
-TEST_F(BrowserThemePackTest, TestBGTabTextColorContrast_TabTint) {
-  // This theme specifies a color for frame (white) and background_tab_text
-  // (black), in addition to a background_tab tint that reduces the color to
-  // nearly zero.
-  base::FilePath theme_path =
-      GetTestExtensionThemePath("theme_test_bgtabtext_tintonly");
-  scoped_refptr<BrowserThemePack> pack(
-      new BrowserThemePack(CustomThemeSupplier::ThemeType::EXTENSION));
-  BuildFromUnpackedExtension(theme_path, pack.get());
-
-  SkColor frame_color;
-  SkColor text_color;
-  color_utils::HSL tab_tint;
-
-  pack->GetColor(TP::COLOR_FRAME, &frame_color);
-  pack->GetColor(TP::COLOR_BACKGROUND_TAB_TEXT, &text_color);
-  pack->GetTint(TP::TINT_BACKGROUND_TAB, &tab_tint);
-
-  SkColor tinted_bg_tab_color = color_utils::HSLShift(frame_color, tab_tint);
-  float contrast_ratio =
-      color_utils::GetContrastRatio(tinted_bg_tab_color, text_color);
-
-  EXPECT_GE(contrast_ratio, color_utils::kMinimumReadableContrastRatio);
-}
-
 // Ensure that, given a theme which only specifies a frame color, the calculated
 // caption button background colors appropriately match the frame color.
 TEST_F(BrowserThemePackTest, TestWindowControlButtonBGColor_FrameColor) {

chrome/browser/themes/theme_properties.cc

@@ -288,30 +288,3 @@ SkColor ThemeProperties::GetDefaultColor(int id, bool incognito) {
       return gfx::kPlaceholderColor;
   }
 }
-
-// static
-SkColor ThemeProperties::GetDefaultColor(PropertyLookupPair lookup_pair) {
-  return GetDefaultColor(lookup_pair.property_id, lookup_pair.is_incognito);
-}
-
-// static
-ThemeProperties::PropertyLookupPair ThemeProperties::GetLookupID(int input_id) {
-  // Mapping of incognito property ids to their corresponding non-incognito
-  // property ids.
-  base::flat_map<int, int> incognito_property_map({
-      {COLOR_FRAME_INCOGNITO, COLOR_FRAME},
-      {COLOR_FRAME_INCOGNITO_INACTIVE, COLOR_FRAME_INACTIVE},
-      {COLOR_BACKGROUND_TAB_INCOGNITO, COLOR_BACKGROUND_TAB},
-      {COLOR_BACKGROUND_TAB_INCOGNITO_INACTIVE, COLOR_BACKGROUND_TAB_INACTIVE},
-      {COLOR_BACKGROUND_TAB_TEXT_INCOGNITO, COLOR_BACKGROUND_TAB_TEXT},
-      {COLOR_BACKGROUND_TAB_TEXT_INCOGNITO_INACTIVE,
-       COLOR_BACKGROUND_TAB_TEXT_INACTIVE},
-      {TINT_FRAME_INCOGNITO, TINT_FRAME},
-      {TINT_FRAME_INCOGNITO_INACTIVE, TINT_FRAME_INACTIVE},
-  });
-
-  const auto found_entry = incognito_property_map.find(input_id);
-  if (found_entry != incognito_property_map.end())
-    return {found_entry->second, true};
-  return {input_id, false};
-}

chrome/browser/themes/theme_properties.h

@@ -159,14 +159,6 @@ class ThemeProperties {
     COLOR_FEATURE_PROMO_BUBBLE_BACKGROUND,
   };
 
-  // Represents the lookup values for a theme property.
-  struct PropertyLookupPair {
-    int property_id;    // ID of the property to lookup (should never be an
-                        // incognito variant)
-    bool is_incognito;  // Whether the lookup should use the incognito value
-                        // of this property or not
-  };
-
   // Themes are hardcoded to draw frame images as if they start this many DIPs
   // above the top of the tabstrip, no matter how much space actually exists.
   // This aids with backwards compatibility (for some themes; Chrome's behavior
@@ -200,16 +192,6 @@ class ThemeProperties {
   // Returns gfx::kPlaceholderColor if |id| is invalid.
   static SkColor GetDefaultColor(int id, bool incognito);
 
-  // Returns the default color for the color represented by |lookup_pair|
-  // Returns gfx::kPlaceholderColor if |id| is invalid.
-  static SkColor GetDefaultColor(PropertyLookupPair lookup_pair);
-
-  // Get the PropertyLookupPair  necessary to look up a property for |input_id|
-  // in an incognito-aware context.  Returns a pair with the id to lookup
-  // (always a non-incognito variant), and a boolean representing whether
-  // |input_id| was an incognito variant of the id to lookup
-  static PropertyLookupPair GetLookupID(int input_id);
-
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(ThemeProperties);
 };

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -1721,33 +1721,30 @@ SkColor TabStrip::GetTabForegroundColor(TabState tab_state,
   const bool is_active_frame = ShouldPaintAsActiveFrame();
 
   // This color varies based on the tab and frame active states.
-  SkColor default_color;
-  if (tab_state == TAB_ACTIVE) {
-    default_color = tp->GetColor(ThemeProperties::COLOR_TAB_TEXT);
-  } else {
-    // If there's a custom color for the background-tab inactive-frame case, use
-    // that instead of alpha blending.
-    if (!is_active_frame &&
-        tp->HasCustomColor(
-            ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE)) {
-      return tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE);
-    }
-
-    const int color_id = ThemeProperties::COLOR_BACKGROUND_TAB_TEXT;
-    default_color =
-        tp->HasCustomColor(color_id)
-            ? tp->GetColor(color_id)
-            : color_utils::PickContrastingColor(
-                  gfx::kGoogleGrey400, gfx::kGoogleGrey800, background_color);
-  }
-
-  if (!is_active_frame) {
-    default_color =
-        color_utils::AlphaBlend(default_color, background_color, 0.75f);
-  }
-
-  return color_utils::BlendForMinContrast(default_color, background_color)
-      .color;
+  int color_id = ThemeProperties::COLOR_TAB_TEXT;
+  if (tab_state != TAB_ACTIVE) {
+    color_id = is_active_frame
+                   ? ThemeProperties::COLOR_BACKGROUND_TAB_TEXT
+                   : ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE;
+  }
+  SkColor color = tp->GetColor(color_id);
+  if (tp->HasCustomColor(color_id))
+    return color;
+  if ((color_id == ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE) &&
+      tp->HasCustomColor(ThemeProperties::COLOR_BACKGROUND_TAB_TEXT)) {
+    // If a custom theme sets a background tab text color for active but not
+    // inactive windows, generate the inactive color by blending the active one
+    // at 75% as we do in the default theme.
+    color = tp->GetColor(ThemeProperties::COLOR_BACKGROUND_TAB_TEXT);
+  } else if (tab_state != TAB_ACTIVE) {
+    color = color_utils::PickContrastingColor(
+        gfx::kGoogleGrey400, gfx::kGoogleGrey800, background_color);
+  }
+
+  if (!is_active_frame)
+    color = color_utils::AlphaBlend(color, background_color, 0.75f);
+
+  return color_utils::BlendForMinContrast(color, background_color).color;
 }
 
 // Returns the accessible tab name for the tab.

chrome/browser/ui/views/tabs/tab_style_views.cc

@@ -400,7 +400,6 @@ TabStyle::TabColors GM2TabStyle::CalculateColors() const {
 
   SkColor title_color = tab_->controller()->GetTabForegroundColor(
       expected_opacity > 0.5f ? TAB_ACTIVE : TAB_INACTIVE, bg_color);
-  title_color = color_utils::BlendForMinContrast(title_color, bg_color).color;
 
   const SkColor base_hovered_color = theme_provider->GetColor(
       ThemeProperties::COLOR_TAB_CLOSE_BUTTON_BACKGROUND_HOVER);