Fix tab colors for custom themes

This change:
* removes COLOR_TAB_TEXT_INACTIVE and instead simply alpha-blends
  COLOR_TAB_TEXT
* removes disabled unit test that uses COLOR_TAB_TEXT_INACTIVE
* uses COLOR_BACKGROUND_TAB_TEXT_INACTIVE directly if a theme
  provides it

Bug: 901610, 859243
Change-Id: I7db13a7719284ac3e27ec3bfc122d6381430df26
Reviewed-on: https://chromium-review.googlesource.com/c/1334419
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608831}

chrome/browser/themes/browser_theme_pack.cc

@@ -59,7 +59,7 @@ constexpr int kTallestFrameHeight = kTallestTabHeight + 19;
 // theme packs that aren't int-equal to this. Increment this number if you
 // change default theme assets or if you need themes to recreate their generated
 // images (which are cached).
-const int kThemePackVersion = 60;
+const int kThemePackVersion = 61;
 
 // 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
@@ -244,7 +244,6 @@ constexpr StringToIntTable kOverwritableColorTable[] = {
      TP::COLOR_BACKGROUND_TAB_INCOGNITO_INACTIVE},
     {"toolbar", TP::COLOR_TOOLBAR},
     {"tab_text", TP::COLOR_TAB_TEXT},
-    {"tab_text_inactive", TP::COLOR_TAB_TEXT_INACTIVE},
     {"tab_background_text", TP::COLOR_BACKGROUND_TAB_TEXT},
     {"tab_background_text_inactive", TP::COLOR_BACKGROUND_TAB_TEXT_INACTIVE},
     {"tab_background_text_incognito", TP::COLOR_BACKGROUND_TAB_TEXT_INCOGNITO},

chrome/browser/themes/theme_properties.cc

@@ -38,7 +38,6 @@ base::Optional<SkColor> GetDarkModeColor(int id) {
   switch (id) {
     case ThemeProperties::COLOR_BOOKMARK_TEXT:
     case ThemeProperties::COLOR_TAB_TEXT:
-    case ThemeProperties::COLOR_TAB_TEXT_INACTIVE:
     case ThemeProperties::COLOR_BACKGROUND_TAB_TEXT:
     case ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE:
     case ThemeProperties::COLOR_NTP_TEXT:
@@ -73,7 +72,6 @@ base::Optional<SkColor> GetIncognitoColor(int id) {
       return SkColorSetRGB(0x32, 0x36, 0x39);
     case ThemeProperties::COLOR_BOOKMARK_TEXT:
     case ThemeProperties::COLOR_TAB_TEXT:
-    case ThemeProperties::COLOR_TAB_TEXT_INACTIVE:
     case ThemeProperties::COLOR_TAB_CLOSE_BUTTON_ACTIVE:
     case ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON:
       return gfx::kGoogleGrey100;
@@ -227,7 +225,6 @@ SkColor ThemeProperties::GetDefaultColor(int id, bool incognito) {
       return SK_ColorWHITE;
     case COLOR_BOOKMARK_TEXT:
     case COLOR_TAB_TEXT:
-    case COLOR_TAB_TEXT_INACTIVE:
       return gfx::kGoogleGrey800;
     case COLOR_BACKGROUND_TAB_TEXT:
     case COLOR_BACKGROUND_TAB_TEXT_INACTIVE:

chrome/browser/themes/theme_properties.h

@@ -40,7 +40,6 @@ class ThemeProperties {
     COLOR_BACKGROUND_TAB_INCOGNITO_INACTIVE,
     COLOR_TOOLBAR,
     COLOR_TAB_TEXT,
-    COLOR_TAB_TEXT_INACTIVE,
     COLOR_BACKGROUND_TAB_TEXT,
     COLOR_BACKGROUND_TAB_TEXT_INACTIVE,
     COLOR_BACKGROUND_TAB_TEXT_INCOGNITO,

chrome/browser/ui/views/frame/browser_non_client_frame_view.cc

@@ -202,24 +202,29 @@ SkColor BrowserNonClientFrameView::GetTabForegroundColor(TabState state) const {
   SkColor background_color;
   SkColor default_color;
   if (state == TAB_ACTIVE) {
-    const int color_id = ShouldPaintAsActive()
-                             ? ThemeProperties::COLOR_TAB_TEXT
-                             : ThemeProperties::COLOR_TAB_TEXT_INACTIVE;
-
     background_color = GetTabBackgroundColor(TAB_ACTIVE);
-    default_color = GetThemeOrDefaultColor(color_id);
+    default_color = GetThemeOrDefaultColor(ThemeProperties::COLOR_TAB_TEXT);
   } else {
-    const int color_id =
-        ShouldPaintAsActive()
-            ? ThemeProperties::COLOR_BACKGROUND_TAB_TEXT
-            : ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE;
-    if (GetThemeProvider()->HasCustomColor(color_id))
-      return GetThemeOrDefaultColor(color_id);
+    // If there's a custom color for the background-tab inactive-frame case, use
+    // that instead of alpha blending.
+    if (!ShouldPaintAsActive() &&
+        GetThemeProvider()->HasCustomColor(
+            ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE)) {
+      return GetThemeOrDefaultColor(
+          ThemeProperties::COLOR_BACKGROUND_TAB_TEXT_INACTIVE);
+    }
 
     background_color = GetTabBackgroundColor(TAB_INACTIVE);
-    default_color = color_utils::IsDark(background_color) ? gfx::kGoogleGrey400
+
+    const int color_id = ThemeProperties::COLOR_BACKGROUND_TAB_TEXT;
+    if (GetThemeProvider()->HasCustomColor(color_id)) {
+      default_color = GetThemeOrDefaultColor(color_id);
+    } else {
+      default_color = color_utils::IsDark(background_color)
+                          ? gfx::kGoogleGrey400
                           : gfx::kGoogleGrey800;
     }
+  }
 
   if (!ShouldPaintAsActive()) {
     // For inactive frames, we draw text at 75%.

chrome/browser/ui/views/frame/browser_non_client_frame_view_unittest.cc

@@ -92,78 +92,3 @@ TEST_F(BrowserNonClientFrameViewTabbedTest, HitTestTabstrip) {
                     GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP) - 1,
                 100, 100)));
 }
-
-// Fixture for testing colors fetched from theme service.
-class BrowserNonClientFrameViewThemeTest
-    : public BrowserNonClientFrameViewTest {
- public:
-  BrowserNonClientFrameViewThemeTest()
-      : BrowserNonClientFrameViewTest(Browser::Type::TYPE_TABBED) {}
-
-  // We override this method to provide our StubThemeService.
-  TestingProfile::TestingFactories GetTestingFactories() override {
-    auto testing_factories =
-        BrowserNonClientFrameViewTest::GetTestingFactories();
-    testing_factories.emplace_back(ThemeServiceFactory::GetInstance(),
-                                   base::BindRepeating(&BuildStubThemeService));
-    return testing_factories;
-  }
-
- private:
-  class StubThemeService : public ThemeService {
-    SkColor GetDefaultColor(int id, bool incognito) const override {
-      switch (id) {
-        case ThemeProperties::COLOR_TAB_TEXT:
-          return SK_ColorBLACK;
-        case ThemeProperties::COLOR_TAB_TEXT_INACTIVE:
-          return SK_ColorWHITE;
-        default:
-          return ThemeService::GetDefaultColor(id, incognito);
-      }
-    }
-  };
-
-  static std::unique_ptr<KeyedService> BuildStubThemeService(
-      content::BrowserContext* context) {
-    auto theme_service = std::make_unique<StubThemeService>();
-    theme_service->Init(static_cast<Profile*>(context));
-    return theme_service;
-  }
-
-  DISALLOW_COPY_AND_ASSIGN(BrowserNonClientFrameViewThemeTest);
-};
-
-// TODO(ellyjones): widget activation doesn't work on Mac.
-// https://crbug.com/823543
-#if defined(OS_MACOSX)
-#define MAYBE_ActiveTabTextColor DISABLED_ActiveTabTextColor
-#else
-#define MAYBE_ActiveTabTextColor DISABLED_ActiveTabTextColor
-#endif
-
-TEST_F(BrowserNonClientFrameViewThemeTest, MAYBE_ActiveTabTextColor) {
-  frame_view_->frame()->Show();
-
-  // Get text color for active tab in active window.
-  frame_view_->frame()->Activate();
-  views::test::WidgetActivationWaiter active_waiter(frame_view_->frame(), true);
-  active_waiter.Wait();
-  SkColor active_color =
-      frame_view_->GetTabForegroundColor(TabState::TAB_ACTIVE);
-
-  // Get text color for active tab in inactive window.
-  frame_view_->frame()->Deactivate();
-  views::test::WidgetActivationWaiter inactive_waiter(frame_view_->frame(),
-                                                      false);
-  inactive_waiter.Wait();
-  SkColor inactive_color =
-      frame_view_->GetTabForegroundColor(TabState::TAB_ACTIVE);
-
-  // These colors should be different since our stub ThemeService returns black
-  // for active windows and white for inactive windows. We don't check the color
-  // against the ones StubThemeService provides because
-  // BrowserNonClientFrameView may adjust the colors for minimum contrast
-  // against the tab background. As long as the colors are different, we're
-  // good.
-  EXPECT_NE(active_color, inactive_color);
-}