Commit b8d5ab25 authored by Peter Kasting's avatar Peter Kasting Committed by Commit Bot

Apply "incognito" tint to dark mode toolbar buttons.

This is necessary because ThemeProperties::GetDefaultColor() is never actually
reached for COLOR_TOOLBAR_BUTTON_ICON (so the kGoogleGrey100 return in
GetIncognitoColor() was dead code).  ThemeService::GetDefaultColor()
unconditionally uses the tint, which was {-1, -1, -1} in dark mode after
http://crrev.com/688233 .  This approximately restores the behavior from before
that CL, when dark-mode was special-cased for TINT_BUTTONS to just return white.

Another way to solve this would be to remove this code from ThemeService, add
a "return kChromeIconGrey" to GetLightModeColor() for COLOR_TOOLBAR_BUTTON_ICON,
and add processing to browser_theme_pack.cc to compute an override color if the
theme specifies TINT_BUTTONS.  This would change HasCustomColor() to return true
"more accurately" -- i.e. only if the theme customizes the color, instead of
what it does now, which is to return true any time the tint is not a no-op.
Looking at callers of HasCustomColor(COLOR_TOOLBAR_BUTTON_ICON), they seem to
want it to return true in the dark/incognito case, so I've avoided this route
for now, although perhaps these callers could be updated (e.g. to check if the
returned color was non-default or just unconditionally use the returned color as
if it's "custom").

This does not apply the dark tinting to TINT_FRAME*, since doing so will cause
dark mode to darken the frame images of every theme that uses them without
explicitly setting a manual tint (i.e. almost all themes that use custom frame
images).  This is sort of an interesting effect, but would almost certainly be
considered "breaking" these themes.

Bug: 995681
Change-Id: I4dd8a7c24323b5a4caf24b3ad08188f73de028c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1762943
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: default avatarEvan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#689157}
parent 602ff292
......@@ -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 = 70;
const int kThemePackVersion = 71;
// 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
......
......@@ -154,7 +154,6 @@ base::Optional<SkColor> GetIncognitoColor(int id) {
return gfx::kGoogleGrey900;
case ThemeProperties::COLOR_BOOKMARK_TEXT:
case ThemeProperties::COLOR_TAB_TEXT:
case ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON:
return gfx::kGoogleGrey100;
case ThemeProperties::COLOR_NTP_TEXT:
return gfx::kGoogleGrey200;
......@@ -255,16 +254,25 @@ color_utils::HSL ThemeProperties::GetDefaultTint(int id, bool incognito) {
"equivalents and an appropriate |incognito| value.";
// If you change these defaults, you must increment the version number in
// browser_theme_pack.cc.
if (incognito) {
if (id == TINT_FRAME)
return {-1, 0.7, 0.075}; // #DEE1E6 -> kGoogleGrey900
if (id == TINT_FRAME_INACTIVE)
// TINT_BUTTONS is used by ThemeService::GetDefaultColor() for both incognito
// and dark mode, and so must be applied to both.
if ((id == TINT_BUTTONS) &&
(incognito ||
ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors()))
return {-1, 0.57, 0.9605}; // kChromeIconGrey -> kGoogleGrey100
// The frame tints are used only when parsing browser themes, and should not
// take dark mode into account, lest themes with custom frame images get those
// images unexpectedly modified just because the user is in dark mode.
if ((id == TINT_FRAME) && incognito)
return {-1, 0.7, 0.075}; // #DEE1E6 -> kGoogleGrey900
if (id == TINT_FRAME_INACTIVE) {
if (incognito)
return {0.57, 0.65, 0.1405}; // #DEE1E6 -> kGoogleGrey800
if (id == TINT_BUTTONS)
return {-1, 0.57, 0.9605}; // kChromeIconGrey -> kGoogleGrey100
} else if (id == TINT_FRAME_INACTIVE) {
return {-1, -1, 0.642}; // #DEE1E6 -> #E7EAED
return {-1, -1, 0.642}; // #DEE1E6 -> #E7EAED
}
return {-1, -1, -1};
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment