Make SWA background and theme colors dynamic.
As part of project jelly apps will have a dynamic color palette which
they cannot predict ahead of time. This means a static color in a SWA
manifest file is likely to be incorrect at runtime and clash visually
with the web contents.
This cl fixes this by having SWAs use a background and theme color pulled from the dynamic palette itself. This updates live with color theme changes.
A SWA only uses this dynamic palette when a theme color or background color can not be inferred from the web contents. The SWA will however ignore a manifest theme/background color. I.e SWAs:
1. Attempt to get color from the web contents.
2. If no web contents color, checks if the jellybean flag is on.
2.1 If flag is on, returns colors from dynamic palette
2.2 If flag is off, attempts to get color from manifest
Screenshots of before/after are available on the bug: crbug.com/1372211.
BUG: 1372211
Change-Id: Id03d93772d9e95f6a521180c914dd80e77e25266
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3812729
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Dominic Schulz <dominicschulz@google.com>
Commit-Queue: Zain Afzal <zafzal@google.com>
Cr-Commit-Position: refs/heads/main@{#1059802}
diff --git a/chrome/browser/ash/system_web_apps/BUILD.gn b/chrome/browser/ash/system_web_apps/BUILD.gn
index 9146fe6..e39bddf 100644
--- a/chrome/browser/ash/system_web_apps/BUILD.gn
+++ b/chrome/browser/ash/system_web_apps/BUILD.gn
@@ -9,6 +9,8 @@
source_set("system_web_apps") {
sources = [
"chrome_system_web_app_ui_config.cc",
+ "color_helpers.cc",
+ "color_helpers.h",
"system_web_app_background_task.cc",
"system_web_app_background_task.h",
"system_web_app_manager.cc",
@@ -45,6 +47,7 @@
"//components/webapps/browser",
"//content/public/browser",
"//ui/base/idle",
+ "//ui/chromeos/styles:cros_tokens_color_mappings_generator",
]
if (!is_official_build) {
diff --git a/chrome/browser/ash/system_web_apps/color_helpers.cc b/chrome/browser/ash/system_web_apps/color_helpers.cc
new file mode 100644
index 0000000..992605fe
--- /dev/null
+++ b/chrome/browser/ash/system_web_apps/color_helpers.cc
@@ -0,0 +1,22 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ash/system_web_apps/color_helpers.h"
+#include "ui/chromeos/styles/cros_tokens_color_mappings.h"
+#include "ui/color/color_provider.h"
+#include "ui/native_theme/native_theme.h"
+
+SkColor ash::GetSystemThemeColor() {
+ auto* native_theme = ui::NativeTheme::GetInstanceForNativeUi();
+ auto* color_provider = ui::ColorProviderManager::Get().GetColorProviderFor(
+ native_theme->GetColorProviderKey(nullptr));
+ return color_provider->GetColor(cros_tokens::kCrosSysHeader);
+}
+
+SkColor ash::GetSystemBackgroundColor() {
+ auto* native_theme = ui::NativeTheme::GetInstanceForNativeUi();
+ auto* color_provider = ui::ColorProviderManager::Get().GetColorProviderFor(
+ native_theme->GetColorProviderKey(nullptr));
+ return color_provider->GetColor(cros_tokens::kCrosSysAppBase2);
+}
diff --git a/chrome/browser/ash/system_web_apps/color_helpers.h b/chrome/browser/ash/system_web_apps/color_helpers.h
new file mode 100644
index 0000000..ce28353
--- /dev/null
+++ b/chrome/browser/ash/system_web_apps/color_helpers.h
@@ -0,0 +1,20 @@
+// Copyright 2022 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_ASH_SYSTEM_WEB_APPS_COLOR_HELPERS_H_
+#define CHROME_BROWSER_ASH_SYSTEM_WEB_APPS_COLOR_HELPERS_H_
+
+#include "third_party/skia/include/core/SkColor.h"
+
+namespace ash {
+// Returns the theme color that system web apps should use, as reported by
+// the operating system theme.
+SkColor GetSystemThemeColor();
+
+// Returns the background color that system web apps should use, as reported
+// by the operating system theme.
+SkColor GetSystemBackgroundColor();
+} // namespace ash
+
+#endif // CHROME_BROWSER_ASH_SYSTEM_WEB_APPS_COLOR_HELPERS_H_
diff --git a/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.cc b/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.cc
index 8596b607..637434aa 100644
--- a/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.cc
+++ b/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.cc
@@ -191,6 +191,9 @@
bool UnittestingSystemAppDelegate::PreferManifestBackgroundColor() const {
return prefer_manifest_background_color_;
}
+bool UnittestingSystemAppDelegate::UseSystemThemeColor() const {
+ return use_system_theme_color_;
+}
#if BUILDFLAG(IS_CHROMEOS)
bool UnittestingSystemAppDelegate::ShouldAnimateThemeChanges() const {
return should_animate_theme_changes_;
@@ -273,6 +276,9 @@
bool value) {
prefer_manifest_background_color_ = value;
}
+void UnittestingSystemAppDelegate::SetUseSystemThemeColor(bool value) {
+ use_system_theme_color_ = value;
+}
#if BUILDFLAG(IS_CHROMEOS)
void UnittestingSystemAppDelegate::SetShouldAnimateThemeChanges(bool value) {
should_animate_theme_changes_ = value;
diff --git a/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.h b/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.h
index a8e0145..b70c5e92 100644
--- a/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.h
+++ b/chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.h
@@ -64,6 +64,7 @@
bool IsAppEnabled() const override;
bool IsUrlInSystemAppScope(const GURL& url) const override;
bool PreferManifestBackgroundColor() const override;
+ bool UseSystemThemeColor() const override;
#if BUILDFLAG(IS_CHROMEOS)
bool ShouldAnimateThemeChanges() const override;
#endif // BUILDFLAG(IS_CHROMEOS)
@@ -90,6 +91,7 @@
void SetIsAppEnabled(bool);
void SetUrlInSystemAppScope(const GURL& url);
void SetPreferManifestBackgroundColor(bool);
+ void SetUseSystemThemeColor(bool);
#if BUILDFLAG(IS_CHROMEOS)
void SetShouldAnimateThemeChanges(bool);
#endif // BUILDFLAG(IS_CHROMEOS)
@@ -115,6 +117,7 @@
bool is_app_enabled = true;
GURL url_in_system_app_scope_;
bool prefer_manifest_background_color_ = false;
+ bool use_system_theme_color_ = true;
#if BUILDFLAG(IS_CHROMEOS)
bool should_animate_theme_changes_ = false;
#endif // BUILDFLAG(IS_CHROMEOS)
diff --git a/chrome/browser/ui/web_applications/web_app_browser_controller.cc b/chrome/browser/ui/web_applications/web_app_browser_controller.cc
index b1057a4b..b6dc94e4 100644
--- a/chrome/browser/ui/web_applications/web_app_browser_controller.cc
+++ b/chrome/browser/ui/web_applications/web_app_browser_controller.cc
@@ -49,7 +49,9 @@
#endif
#if BUILDFLAG(IS_CHROMEOS_ASH)
+#include "ash/constants/ash_features.h"
#include "chrome/browser/ash/apps/apk_web_app_service.h"
+#include "chrome/browser/ash/system_web_apps/color_helpers.h"
#include "chrome/browser/ash/system_web_apps/types/system_web_app_delegate.h"
#endif
@@ -311,7 +313,7 @@
absl::optional<SkColor> WebAppBrowserController::GetThemeColor() const {
#if BUILDFLAG(IS_CHROMEOS_ASH)
// System App popups (settings pages) always use default theme.
- if (system_app_ && browser()->is_type_app_popup())
+ if (system_app() && browser()->is_type_app_popup())
return absl::nullopt;
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
@@ -332,6 +334,15 @@
}
#endif // BUILDFLAG(IS_CHROMEOS)
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+ // System Apps with dynamic color ignore manifest and pull theme color from
+ // the OS.
+ if (system_app() && system_app()->UseSystemThemeColor() &&
+ ash::features::IsJellyEnabled()) {
+ return ash::GetSystemThemeColor();
+ }
+#endif // BUILDFLAG(IS_CHROMEOS_ASH)
+
if (ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors()) {
absl::optional<SkColor> dark_mode_color =
registrar().GetAppDarkModeThemeColor(app_id());
@@ -347,19 +358,30 @@
absl::optional<SkColor> WebAppBrowserController::GetBackgroundColor() const {
auto web_contents_color = AppBrowserController::GetBackgroundColor();
auto manifest_color = GetResolvedManifestBackgroundColor();
+ // Prefer an available web contents color but when such a color is
+ // unavailable (i.e. in the time between when a window launches and it's web
+ // content loads) attempt to pull the background color from the manifest.
+ absl::optional<SkColor> result =
+ web_contents_color ? web_contents_color : manifest_color;
- bool prefer_manifest_background_color = false;
#if BUILDFLAG(IS_CHROMEOS_ASH)
- if (system_app())
- prefer_manifest_background_color =
- system_app()->PreferManifestBackgroundColor();
+ if (system_app()) {
+ if (ash::features::IsJellyEnabled()) {
+ // System Apps with dynamic color ignore the manifest and pull background
+ // color from the OS in situations where a background color can not be
+ // extracted from the web contents.
+ SkColor os_color = ash::GetSystemBackgroundColor();
+
+ result = web_contents_color ? web_contents_color : os_color;
+ } else if (system_app()->PreferManifestBackgroundColor()) {
+ // Some system web apps prefer their web content background color to be
+ // ignored in favour of their manifest background color.
+ result = manifest_color ? manifest_color : web_contents_color;
+ }
+ }
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
- auto [preferred_color, fallback_color] =
- prefer_manifest_background_color
- ? std::tie(manifest_color, web_contents_color)
- : std::tie(web_contents_color, manifest_color);
- return preferred_color ? preferred_color : fallback_color;
+ return result;
}
GURL WebAppBrowserController::GetAppStartUrl() const {
diff --git a/chrome/browser/ui/web_applications/web_app_browsertest.cc b/chrome/browser/ui/web_applications/web_app_browsertest.cc
index 85d2d1f4..1b3d0f4 100644
--- a/chrome/browser/ui/web_applications/web_app_browsertest.cc
+++ b/chrome/browser/ui/web_applications/web_app_browsertest.cc
@@ -99,6 +99,8 @@
#include "ui/gfx/geometry/size.h"
#if BUILDFLAG(IS_CHROMEOS_ASH)
+#include "ash/constants/ash_features.h"
+#include "chrome/browser/ash/system_web_apps/color_helpers.h"
#include "chrome/browser/ash/system_web_apps/test_support/test_system_web_app_installation.h"
#include "chrome/browser/ui/ash/shelf/chrome_shelf_controller.h"
#endif
@@ -406,27 +408,17 @@
#if BUILDFLAG(IS_CHROMEOS_ASH)
-class BackgroundColorChangeSystemWebAppBrowserTest
- : public WebAppBrowserTest,
- public testing::WithParamInterface<
- /*prefer_manifest_background_color=*/bool> {
+class ColorSystemWebAppBrowserTest : public WebAppBrowserTest {
public:
- BackgroundColorChangeSystemWebAppBrowserTest() {
+ ColorSystemWebAppBrowserTest() {
system_web_app_installation_ =
ash::TestSystemWebAppInstallation::SetUpAppWithColors(
/*theme_color=*/SK_ColorWHITE,
/*dark_mode_theme_color=*/SK_ColorBLACK,
/*background_color=*/SK_ColorWHITE,
/*dark_mode_background_color=*/SK_ColorBLACK);
- static_cast<ash::UnittestingSystemAppDelegate*>(
- system_web_app_installation_->GetDelegate())
- ->SetPreferManifestBackgroundColor(PreferManifestBackgroundColor());
}
- // Returns whether the web app under test prefers manifest background colors
- // over web contents background colors.
- bool PreferManifestBackgroundColor() const { return GetParam(); }
-
// Installs the web app under test, blocking until installation is complete,
// and returning the `AppId` for the installed web app.
AppId WaitForSwaInstall() {
@@ -434,11 +426,46 @@
return system_web_app_installation_->GetAppId();
}
- private:
+ protected:
std::unique_ptr<ash::TestSystemWebAppInstallation>
system_web_app_installation_;
};
+class BackgroundColorChangeSystemWebAppBrowserTest
+ : public ColorSystemWebAppBrowserTest,
+ public testing::WithParamInterface<
+ /*prefer_manifest_background_color=*/bool> {
+ public:
+ BackgroundColorChangeSystemWebAppBrowserTest() {
+ static_cast<ash::UnittestingSystemAppDelegate*>(
+ system_web_app_installation_->GetDelegate())
+ ->SetPreferManifestBackgroundColor(PreferManifestBackgroundColor());
+ }
+
+ // Returns whether the web app under test prefers manifest background colors
+ // over web contents background colors.
+ bool PreferManifestBackgroundColor() const { return GetParam(); }
+};
+
+class DynamicColorSystemWebAppBrowserTest
+ : public ColorSystemWebAppBrowserTest,
+ public testing::WithParamInterface</*use_system_theme_color=*/bool> {
+ public:
+ DynamicColorSystemWebAppBrowserTest() {
+ auto* delegate = static_cast<ash::UnittestingSystemAppDelegate*>(
+ system_web_app_installation_->GetDelegate());
+
+ delegate->SetUseSystemThemeColor(GetParam());
+ }
+
+ // Returns whether the web app under test wants to use a system sourced theme
+ // color.
+ bool UseSystemThemeColor() const { return GetParam(); }
+
+ private:
+ base::test::ScopedFeatureList scoped_feature_list_{ash::features::kJelly};
+};
+
INSTANTIATE_TEST_SUITE_P(All,
BackgroundColorChangeSystemWebAppBrowserTest,
/*prefer_manifest_background_color=*/testing::Bool(),
@@ -489,6 +516,40 @@
}
}
+INSTANTIATE_TEST_SUITE_P(All,
+ DynamicColorSystemWebAppBrowserTest,
+ /*use_system_theme_color=*/::testing::Bool(),
+ [](const testing::TestParamInfo<
+ /*use_system_theme_color=*/bool>& info) {
+ return info.param ? "WithUseSystemThemeColor"
+ : "WithoutUseSystemThemeColor";
+ });
+
+IN_PROC_BROWSER_TEST_P(DynamicColorSystemWebAppBrowserTest, BackgroundColor) {
+ const AppId app_id = WaitForSwaInstall();
+ Browser* const app_browser = LaunchWebAppBrowser(app_id);
+ auto* app_controller = app_browser->app_controller();
+
+ // Ensure app controller is pulling the color from the OS.
+ EXPECT_EQ(app_controller->GetBackgroundColor().value(),
+ ash::GetSystemBackgroundColor());
+}
+
+IN_PROC_BROWSER_TEST_P(DynamicColorSystemWebAppBrowserTest, ThemeColor) {
+ const AppId app_id = WaitForSwaInstall();
+ Browser* const app_browser = LaunchWebAppBrowser(app_id);
+ auto* app_controller = app_browser->app_controller();
+ auto theme_color = app_controller->GetThemeColor().value();
+ if (UseSystemThemeColor()) {
+ // Ensure app controller is pulling the color from the OS.
+ EXPECT_EQ(theme_color, ash::GetSystemThemeColor());
+ } else {
+ // If SWA has opted out, theme color should default to white or black
+ // depending on launch context.
+ EXPECT_TRUE(theme_color == SK_ColorWHITE || theme_color == SK_ColorBLACK);
+ }
+}
+
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
// This tests that we don't crash when launching a PWA window with an
