Merge pull request #13124 from brave/fixed_dark_mode_omnibox_colors

Fixed wrong location bar/omnibox colors are used
brave · Apr 22, 2022 · cb752c3 · cb752c3
2 parents d210ca9 + c13dbe0
commit cb752c3
Show file tree

Hide file tree

Showing 5 changed files with 130 additions and 50 deletions.
diff --git a/browser/themes/brave_theme_helper.cc b/browser/themes/brave_theme_helper.cc
@@ -7,6 +7,7 @@
 
 #include "base/numerics/safe_conversions.h"
 #include "brave/browser/themes/brave_dark_mode_utils.h"
+#include "brave/browser/themes/brave_theme_helper_utils.h"
 #include "brave/browser/themes/theme_properties.h"
 #include "brave/components/brave_vpn/buildflags/buildflags.h"
 #include "brave/components/sidebar/buildflags/buildflags.h"
@@ -15,7 +16,6 @@
 #include "chrome/browser/ui/omnibox/omnibox_theme.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/color_utils.h"
-#include "ui/native_theme/native_theme.h"
 
 #if BUILDFLAG(IS_LINUX)
 #include "chrome/browser/themes/custom_theme_supplier.h"
@@ -28,55 +28,6 @@ namespace {
 const SkColor kDarkOmniboxText = SkColorSetRGB(0xff, 0xff, 0xff);
 const SkColor kLightOmniboxText = SkColorSetRGB(0x42, 0x42, 0x42);
 
-// Location bar colors
-const SkColor kPrivateLocationBarBgBase = SkColorSetRGB(0x0B, 0x07, 0x24);
-const SkColor kDarkLocationBarBgBase = SkColorSetRGB(0x18, 0x1A, 0x21);
-const SkColor kDarkLocationBarHoverBg = SkColorSetRGB(0x23, 0x25, 0x2F);
-
-SkColor GetLocationBarBackground(bool dark, bool priv, bool hover) {
-  if (priv) {
-    return hover ? color_utils::HSLShift(kPrivateLocationBarBgBase,
-                                         {-1, -1, 0.54})
-                 : kPrivateLocationBarBgBase;
-  }
-
-  if (dark) {
-    return hover ? kDarkLocationBarHoverBg : kDarkLocationBarBgBase;
-  }
-
-  return hover ? color_utils::AlphaBlend(SK_ColorWHITE,
-                                         SkColorSetRGB(0xf3, 0xf3, 0xf3), 0.7f)
-               : SK_ColorWHITE;
-}
-
-// Omnibox result bg colors
-SkColor GetOmniboxResultBackground(int id, bool dark, bool priv) {
-  // For high contrast, selected rows use inverted colors to stand out more.
-  ui::NativeTheme* native_theme = ui::NativeTheme::GetInstanceForNativeUi();
-  bool high_contrast =
-      native_theme && native_theme->UserHasContrastPreference();
-  OmniboxPartState state = OmniboxPartState::NORMAL;
-  if (id == ThemeProperties::COLOR_OMNIBOX_RESULTS_BG_HOVERED) {
-    state = OmniboxPartState::HOVERED;
-  } else if (id == ThemeProperties::COLOR_OMNIBOX_RESULTS_BG_SELECTED) {
-    state = OmniboxPartState::SELECTED;
-  }
-
-  SkColor color;
-  if (priv) {
-    color = high_contrast ? color_utils::HSLShift(kPrivateLocationBarBgBase,
-                                                  {-1, -1, 0.45})
-                          : kPrivateLocationBarBgBase;
-  } else if (dark) {
-    color = high_contrast ? gfx::kGoogleGrey900 : kDarkLocationBarBgBase;
-  } else {
-    color = SK_ColorWHITE;
-  }
-  return color_utils::BlendTowardMaxContrast(
-      color,
-      base::ClampRound(GetOmniboxStateOpacity(state) * 0xff));
-}
-
 #if BUILDFLAG(IS_LINUX)
 bool IsUsingSystemTheme(const CustomThemeSupplier* theme_supplier) {
   return theme_supplier &&
@@ -169,6 +120,18 @@ SkColor BraveThemeHelper::GetDefaultColor(
   if (braveColor) {
     return braveColor.value();
   }
+
+  // Handles omnibox colors before upstream handles.
+  // We shares most of dark mode colors with upstream's incognito color.
+  // We set |incognito| to true below for dark mode before fetching upstream's
+  // default color because we share upstream's incognito colors for our dark
+  // mode. So, we should handle here for using our omnibox colors before setting
+  // |incognito|.
+  const absl::optional<SkColor> omnibox_color =
+      GetOmniboxColor(id, incognito, theme_supplier);
+  if (omnibox_color.has_value())
+    return omnibox_color.value();
+
   // Make sure we fallback to Chrome's dark theme (incognito) for our dark theme
   if (type == dark_mode::BraveDarkModeType::BRAVE_DARK_MODE_TYPE_DARK) {
     incognito = true;

diff --git a/browser/themes/brave_theme_helper_utils.cc b/browser/themes/brave_theme_helper_utils.cc
@@ -0,0 +1,65 @@
+/* Copyright (c) 2022 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "brave/browser/themes/brave_theme_helper_utils.h"
+
+#include "base/numerics/safe_conversions.h"
+#include "chrome/browser/themes/theme_properties.h"
+#include "chrome/browser/ui/omnibox/omnibox_theme.h"
+#include "ui/gfx/color_palette.h"
+#include "ui/gfx/color_utils.h"
+#include "ui/native_theme/native_theme.h"
+
+namespace {
+
+// Location bar colors
+const SkColor kPrivateLocationBarBgBase = SkColorSetRGB(0x0B, 0x07, 0x24);
+const SkColor kDarkLocationBarBgBase = SkColorSetRGB(0x18, 0x1A, 0x21);
+const SkColor kDarkLocationBarHoverBg = SkColorSetRGB(0x23, 0x25, 0x2F);
+
+}  // namespace
+
+SkColor GetLocationBarBackground(bool dark, bool priv, bool hover) {
+  if (priv) {
+    return hover ? color_utils::HSLShift(kPrivateLocationBarBgBase,
+                                         {-1, -1, 0.54})
+                 : kPrivateLocationBarBgBase;
+  }
+
+  if (dark) {
+    return hover ? kDarkLocationBarHoverBg : kDarkLocationBarBgBase;
+  }
+
+  return hover ? color_utils::AlphaBlend(SK_ColorWHITE,
+                                         SkColorSetRGB(0xf3, 0xf3, 0xf3), 0.7f)
+               : SK_ColorWHITE;
+}
+
+// Omnibox result bg colors
+SkColor GetOmniboxResultBackground(int id, bool dark, bool priv) {
+  // For high contrast, selected rows use inverted colors to stand out more.
+  ui::NativeTheme* native_theme = ui::NativeTheme::GetInstanceForNativeUi();
+  bool high_contrast =
+      native_theme && native_theme->UserHasContrastPreference();
+  OmniboxPartState state = OmniboxPartState::NORMAL;
+  if (id == ThemeProperties::COLOR_OMNIBOX_RESULTS_BG_HOVERED) {
+    state = OmniboxPartState::HOVERED;
+  } else if (id == ThemeProperties::COLOR_OMNIBOX_RESULTS_BG_SELECTED) {
+    state = OmniboxPartState::SELECTED;
+  }
+
+  SkColor color;
+  if (priv) {
+    color = high_contrast ? color_utils::HSLShift(kPrivateLocationBarBgBase,
+                                                  {-1, -1, 0.45})
+                          : kPrivateLocationBarBgBase;
+  } else if (dark) {
+    color = high_contrast ? gfx::kGoogleGrey900 : kDarkLocationBarBgBase;
+  } else {
+    color = SK_ColorWHITE;
+  }
+  return color_utils::BlendTowardMaxContrast(
+      color, base::ClampRound(GetOmniboxStateOpacity(state) * 0xff));
+}
diff --git a/browser/themes/brave_theme_helper_utils.h b/browser/themes/brave_theme_helper_utils.h
@@ -0,0 +1,14 @@
+/* Copyright (c) 2022 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef BRAVE_BROWSER_THEMES_BRAVE_THEME_HELPER_UTILS_H_
+#define BRAVE_BROWSER_THEMES_BRAVE_THEME_HELPER_UTILS_H_
+
+#include "third_party/skia/include/core/SkColor.h"
+
+SkColor GetLocationBarBackground(bool dark, bool priv, bool hover);
+SkColor GetOmniboxResultBackground(int id, bool dark, bool priv);
+
+#endif  // BRAVE_BROWSER_THEMES_BRAVE_THEME_HELPER_UTILS_H_
diff --git a/browser/themes/brave_theme_service_browsertest.cc b/browser/themes/brave_theme_service_browsertest.cc
@@ -4,6 +4,7 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "brave/browser/themes/brave_dark_mode_utils.h"
+#include "brave/browser/themes/brave_theme_helper_utils.h"
 #include "brave/browser/themes/theme_properties.h"
 #include "brave/common/pref_names.h"
 #include "build/build_config.h"
@@ -12,6 +13,8 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/omnibox/omnibox_theme.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/test/browser_test.h"
@@ -152,6 +155,39 @@ IN_PROC_BROWSER_TEST_F(BraveThemeServiceTest, SystemThemeChangeTest) {
   }
 }
 
+IN_PROC_BROWSER_TEST_F(BraveThemeServiceTest, OmniboxColorTest) {
+  auto* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
+  auto* tp = browser_view->GetThemeProvider();
+  const int hovered = false;
+
+  // Change to light.
+  dark_mode::SetBraveDarkModeType(
+      dark_mode::BraveDarkModeType::BRAVE_DARK_MODE_TYPE_LIGHT);
+  bool dark = false;
+  EXPECT_EQ(GetLocationBarBackground(dark, false /* incognito */, hovered),
+            GetOmniboxColor(tp, OmniboxPart::LOCATION_BAR_BACKGROUND));
+  EXPECT_EQ(
+      GetOmniboxResultBackground(ThemeProperties::COLOR_OMNIBOX_RESULTS_BG,
+                                 dark, false /* incognito */),
+      tp->GetColor(ThemeProperties::COLOR_OMNIBOX_RESULTS_BG));
+
+  // Change to dark.
+  dark_mode::SetBraveDarkModeType(
+      dark_mode::BraveDarkModeType::BRAVE_DARK_MODE_TYPE_DARK);
+  dark = true;
+
+  EXPECT_EQ(GetLocationBarBackground(dark, false /* incognito */, hovered),
+            GetOmniboxColor(tp, OmniboxPart::LOCATION_BAR_BACKGROUND));
+  // Check color is different on dark mode and incognito mode.
+  EXPECT_NE(GetLocationBarBackground(dark, true /* incognito */, hovered),
+            GetOmniboxColor(tp, OmniboxPart::LOCATION_BAR_BACKGROUND));
+
+  EXPECT_EQ(
+      GetOmniboxResultBackground(ThemeProperties::COLOR_OMNIBOX_RESULTS_BG,
+                                 dark, false /* incognito */),
+      tp->GetColor(ThemeProperties::COLOR_OMNIBOX_RESULTS_BG));
+}
+
 #if BUILDFLAG(IS_WIN)
 // Test native theme notification is called properly by changing reg value.
 // This simulates dark mode setting from Windows settings.

diff --git a/browser/themes/sources.gni b/browser/themes/sources.gni
@@ -12,6 +12,8 @@ if (!is_android) {
     "//brave/browser/themes/brave_dark_mode_utils.cc",
     "//brave/browser/themes/brave_theme_helper.cc",
     "//brave/browser/themes/brave_theme_helper.h",
+    "//brave/browser/themes/brave_theme_helper_utils.cc",
+    "//brave/browser/themes/brave_theme_helper_utils.h",
     "//brave/browser/themes/brave_theme_service.cc",
     "//brave/browser/themes/brave_theme_service.h",
   ]