From 44145efeb3e9f8e1502e9d87ffb8cea921daa4bc Mon Sep 17 00:00:00 2001 From: Tibor Goldschwendt Date: Mon, 19 Dec 2022 15:43:44 +0000 Subject: [PATCH] [cc-side-panel] Replace default color with main color if it exists Adds a separate for the extracted / main color of the background image, which is shown in place of the default color if a main color for the background image exists. The main color circles is selected whenever the currently set theme matches the extracted color. To that end, this CL refactors the selection logic somewhat to better guarantee at most one selected color circle. + Makes the main color optional in the background image data to robustly identify the case where there is no main color. + Sets main color to null if extracted color feature is disabled so that we can disable the feature later on if need be. https://screenshot.googleplex.com/57TvkNaruwuuoTF Fixed: 1401018 Change-Id: I9cadd272146081c044e484bb0f024e08d1f23c62 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4112661 Commit-Queue: Tibor Goldschwendt Reviewed-by: Alex Gough Cr-Commit-Position: refs/heads/main@{#1084962} --- chrome/app/generated_resources.grd | 3 + ...DS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL.png.sha1 | 1 + .../side_panel/customize_chrome/colors.html | 49 ++++++---- .../side_panel/customize_chrome/colors.ts | 90 ++++++++++++++++--- .../search/background/ntp_background_data.h | 3 +- .../ntp_custom_background_service.cc | 5 +- .../ntp_custom_background_service_unittest.cc | 14 ++- .../customize_chrome/customize_chrome.mojom | 2 + .../customize_chrome_page_handler.cc | 4 + .../customize_chrome_page_handler_unittest.cc | 5 +- .../customize_chrome/customize_chrome_ui.cc | 1 + .../customize_chrome/colors_test.ts | 76 ++++++++++++++-- .../customize_chrome/test_support.ts | 1 + 13 files changed, 207 insertions(+), 47 deletions(-) create mode 100644 chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL.png.sha1 diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index da40975d68b06..178eef0ed011e 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -6773,6 +6773,9 @@ Keep your key file in a safe place. You will need it to create new versions of y Default color + + Color based on theme + Warm grey diff --git a/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL.png.sha1 b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL.png.sha1 new file mode 100644 index 0000000000000..d88be19770211 --- /dev/null +++ b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL.png.sha1 @@ -0,0 +1 @@ +2047f3cd7a6cc0b347ba3ac3bd59dfdd4f586780 \ No newline at end of file diff --git a/chrome/browser/resources/side_panel/customize_chrome/colors.html b/chrome/browser/resources/side_panel/customize_chrome/colors.html index 1f4de436258b6..9f5c993830cb5 100644 --- a/chrome/browser/resources/side_panel/customize_chrome/colors.html +++ b/chrome/browser/resources/side_panel/customize_chrome/colors.html @@ -40,20 +40,35 @@ - - - + + diff --git a/chrome/browser/resources/side_panel/customize_chrome/colors.ts b/chrome/browser/resources/side_panel/customize_chrome/colors.ts index ca1df1f88afc7..cba0bb8f649ac 100644 --- a/chrome/browser/resources/side_panel/customize_chrome/colors.ts +++ b/chrome/browser/resources/side_panel/customize_chrome/colors.ts @@ -29,15 +29,21 @@ export const DARK_DEFAULT_COLOR: Color = { foreground: {value: 0xff202124}, }; -function isChromeColorSelected( - theme: Theme|undefined, colors: ChromeColor[]|undefined, color: SkColor) { - return !!theme && !!colors && !!theme.foregroundColor && - theme.foregroundColor.value === color.value; +enum ColorType { + NONE, + DEFAULT, + MAIN, + CHROME, + CUSTOM, +} + +interface SelectedColor { + type: ColorType; + chromeColor?: SkColor; } export interface ColorsElement { $: { - defaultColor: ColorElement, chromeColors: DomRepeat, customColorContainer: HTMLElement, customColor: ColorElement, @@ -61,15 +67,27 @@ export class ColorsElement extends PolymerElement { type: Object, computed: 'computeDefaultColor_(theme_)', }, + mainColor_: { + type: Object, + computed: 'computeMainColor_(theme_)', + }, colors_: Array, theme_: Object, + selectedColor_: { + type: Object, + computed: 'computeSelectedColor_(theme_, colors_)', + }, isDefaultColorSelected_: { type: Object, - computed: 'computeIsDefaultColorSelected_(theme_)', + computed: 'computeIsDefaultColorSelected_(selectedColor_)', + }, + isMainColorSelected_: { + type: Object, + computed: 'computeIsMainColorSelected_(selectedColor_)', }, isCustomColorSelected_: { type: Object, - computed: 'computeIsCustomColorSelected_(theme_, color_)', + computed: 'computeIsCustomColorSelected_(selectedColor_)', }, customColor_: { type: Object, @@ -89,6 +107,7 @@ export class ColorsElement extends PolymerElement { private colors_: ChromeColor[]; private theme_: Theme; + private selectedColor_: SelectedColor; private isCustomColorSelected_: boolean; private customColor_: Color; private setThemeListenerId_: number|null = null; @@ -122,19 +141,49 @@ export class ColorsElement extends PolymerElement { LIGHT_DEFAULT_COLOR; } + private computeMainColor_(): SkColor|undefined { + return this.theme_ && this.theme_.backgroundImage && + this.theme_.backgroundImage.mainColor; + } + + private computeSelectedColor_(): SelectedColor { + if (!this.colors_ || !this.theme_) { + return {type: ColorType.NONE}; + } + if (!this.theme_.foregroundColor) { + return {type: ColorType.DEFAULT}; + } + if (this.theme_.backgroundImage && this.theme_.backgroundImage.mainColor && + this.theme_.backgroundImage.mainColor!.value === + this.theme_.foregroundColor.value) { + return {type: ColorType.MAIN}; + } + if (this.colors_.find( + (color: ChromeColor) => color.foreground.value === + this.theme_.foregroundColor!.value)) { + return { + type: ColorType.CHROME, + chromeColor: this.theme_.foregroundColor!, + }; + } + return {type: ColorType.CUSTOM}; + } + private computeIsDefaultColorSelected_(): boolean { - return this.theme_ && !this.theme_.foregroundColor; + return this.selectedColor_.type === ColorType.DEFAULT; + } + + private computeIsMainColorSelected_(): boolean { + return this.selectedColor_.type === ColorType.MAIN; } private computeIsCustomColorSelected_(): boolean { - return !!this.colors_ && !!this.theme_ && !!this.theme_.foregroundColor && - !this.colors_.find( - (color: ChromeColor) => - color.foreground.value === this.theme_.foregroundColor!.value); + return this.selectedColor_.type === ColorType.CUSTOM; } private isChromeColorSelected_(color: SkColor): boolean { - return isChromeColorSelected(this.theme_, this.colors_, color); + return this.selectedColor_.type === ColorType.CHROME && + this.selectedColor_.chromeColor!.value === color.value; } private tabIndex_(selected: boolean): string { @@ -142,17 +191,30 @@ export class ColorsElement extends PolymerElement { } private chromeColorTabIndex_(color: SkColor): string { - return isChromeColorSelected(this.theme_, this.colors_, color) ? '0' : '-1'; + return this.selectedColor_.type === ColorType.CHROME && + this.selectedColor_.chromeColor!.value === color.value ? + '0' : + '-1'; } private themeHasBackgroundImage_(): boolean { return !!this.theme_ && !!this.theme_.backgroundImage; } + private themeHasMainColor_(): boolean { + return !!this.theme_ && !!this.theme_.backgroundImage && + !!this.theme_.backgroundImage.mainColor; + } + private onDefaultColorClick_() { CustomizeChromeApiProxy.getInstance().handler.setDefaultColor(); } + private onMainColorClick_() { + CustomizeChromeApiProxy.getInstance().handler.setForegroundColor( + this.theme_!.backgroundImage!.mainColor!); + } + private onChromeColorClick_(e: Event) { CustomizeChromeApiProxy.getInstance().handler.setForegroundColor( this.$.chromeColors.itemForElement(e.target as HTMLElement).foreground); diff --git a/chrome/browser/search/background/ntp_background_data.h b/chrome/browser/search/background/ntp_background_data.h index 4cf64b87a8f97..b638aa04e3755 100644 --- a/chrome/browser/search/background/ntp_background_data.h +++ b/chrome/browser/search/background/ntp_background_data.h @@ -8,6 +8,7 @@ #include #include "chrome/browser/search/background/ntp_background.pb.h" +#include "third_party/abseil-cpp/absl/types/optional.h" #include "third_party/skia/include/core/SkColor.h" #include "url/gurl.h" @@ -135,7 +136,7 @@ struct CustomBackground { std::string collection_id; // Main color of the image. - SkColor custom_background_main_color; + absl::optional custom_background_main_color; }; #endif // CHROME_BROWSER_SEARCH_BACKGROUND_NTP_BACKGROUND_DATA_H_ diff --git a/chrome/browser/search/background/ntp_custom_background_service.cc b/chrome/browser/search/background/ntp_custom_background_service.cc index 299de464efebf..a4d7fa1d82276 100644 --- a/chrome/browser/search/background/ntp_custom_background_service.cc +++ b/chrome/browser/search/background/ntp_custom_background_service.cc @@ -430,7 +430,10 @@ NtpCustomBackgroundService::GetCustomBackground() { const base::Value* attribution_action_url = background_info.Find(kNtpCustomBackgroundAttributionActionURL); const base::Value* color = - background_info.Find(kNtpCustomBackgroundMainColor); + base::FeatureList::IsEnabled( + ntp_features::kCustomizeChromeColorExtraction) + ? background_info.Find(kNtpCustomBackgroundMainColor) + : nullptr; custom_background->custom_background_url = custom_background_url; custom_background->is_uploaded_image = false; custom_background->collection_id = collection_id; diff --git a/chrome/browser/search/background/ntp_custom_background_service_unittest.cc b/chrome/browser/search/background/ntp_custom_background_service_unittest.cc index e2bccb4ff2a63..c57f7a552dc9a 100644 --- a/chrome/browser/search/background/ntp_custom_background_service_unittest.cc +++ b/chrome/browser/search/background/ntp_custom_background_service_unittest.cc @@ -25,6 +25,7 @@ #include "content/public/test/test_utils.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/abseil-cpp/absl/types/optional.h" #include "third_party/skia/include/core/SkBitmap.h" #include "ui/gfx/image/image.h" #include "url/gurl.h" @@ -577,8 +578,9 @@ TEST_F(NtpCustomBackgroundServiceTest, TestUpdateCustomBackgroundColor) { task_environment_.RunUntilIdle(); auto custom_background = custom_background_service_->GetCustomBackground(); auto custom_background_main_color = - custom_background ? custom_background->custom_background_main_color : 0; - EXPECT_NE(SK_ColorRED, custom_background_main_color); + custom_background ? custom_background->custom_background_main_color + : SK_ColorWHITE; + EXPECT_NE(SK_ColorRED, custom_background_main_color.value_or(SK_ColorWHITE)); const GURL kUrl("https://www.foo.com"); const GURL kThumbnailUrl("https://www.thumbnail.com"); @@ -597,12 +599,16 @@ TEST_F(NtpCustomBackgroundServiceTest, TestUpdateCustomBackgroundColor) { GURL("different_url"), image, image_fetcher::RequestMetadata()); task_environment_.RunUntilIdle(); custom_background = custom_background_service_->GetCustomBackground(); - EXPECT_NE(SK_ColorRED, custom_background->custom_background_main_color); + EXPECT_NE( + SK_ColorRED, + custom_background->custom_background_main_color.value_or(SK_ColorWHITE)); // Background color should update. custom_background_service_->UpdateCustomBackgroundColorAsync( kUrl, image, image_fetcher::RequestMetadata()); task_environment_.RunUntilIdle(); custom_background = custom_background_service_->GetCustomBackground(); - EXPECT_EQ(SK_ColorRED, custom_background->custom_background_main_color); + EXPECT_EQ( + SK_ColorRED, + custom_background->custom_background_main_color.value_or(SK_ColorWHITE)); } diff --git a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome.mojom b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome.mojom index 00b9102341f2c..8da3053aa4f50 100644 --- a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome.mojom +++ b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome.mojom @@ -15,6 +15,8 @@ struct BackgroundImage { bool is_uploaded_image; // Title of the background image. string title; + // The main color of the background image. + skia.mojom.SkColor? main_color; }; // A generic theme. diff --git a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler.cc b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler.cc index e2b3f7a2363fb..edbcc2b87a31d 100644 --- a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler.cc +++ b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler.cc @@ -151,6 +151,10 @@ void CustomizeChromePageHandler::UpdateTheme() { background_image->is_uploaded_image = custom_background->is_uploaded_image; background_image->title = custom_background->custom_background_attribution_line_1; + if (custom_background->custom_background_main_color.has_value()) { + background_image->main_color = + *custom_background->custom_background_main_color; + } } else { background_image = nullptr; } diff --git a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler_unittest.cc b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler_unittest.cc index 4dc9345b01c0e..90d598e0832fa 100644 --- a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler_unittest.cc +++ b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_page_handler_unittest.cc @@ -370,6 +370,7 @@ TEST_P(CustomizeChromePageHandlerSetThemeTest, SetTheme) { custom_background.custom_background_url = GURL("https://foo.com/img.png"); custom_background.custom_background_attribution_line_1 = "foo line"; custom_background.is_uploaded_image = false; + custom_background.custom_background_main_color = SK_ColorGREEN; ON_CALL(mock_ntp_custom_background_service_, GetCustomBackground()) .WillByDefault(testing::Return(absl::make_optional(custom_background))); ON_CALL(mock_theme_service(), UsingDefaultTheme()) @@ -384,7 +385,9 @@ TEST_P(CustomizeChromePageHandlerSetThemeTest, SetTheme) { ASSERT_TRUE(theme); ASSERT_TRUE(theme->background_image); EXPECT_EQ("https://foo.com/img.png", theme->background_image->url); - ASSERT_FALSE(theme->background_image->is_uploaded_image); + EXPECT_FALSE(theme->background_image->is_uploaded_image); + EXPECT_EQ(SK_ColorGREEN, + theme->background_image->main_color.value_or(SK_ColorWHITE)); EXPECT_EQ("foo line", theme->background_image->title); EXPECT_TRUE(theme->system_dark_mode); EXPECT_EQ( diff --git a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_ui.cc b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_ui.cc index 94e918706ac87..01013ba768c12 100644 --- a/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_ui.cc +++ b/chrome/browser/ui/webui/side_panel/customize_chrome/customize_chrome_ui.cc @@ -36,6 +36,7 @@ CustomizeChromeUI::CustomizeChromeUI(content::WebUI* web_ui) {"customizeThisPage", IDS_NTP_CUSTOM_BG_CUSTOMIZE_NTP_LABEL}, {"appearanceHeader", IDS_NTP_CUSTOMIZE_APPEARANCE_LABEL}, {"defaultColorName", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL}, + {"mainColorName", IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL}, {"mostVisited", IDS_NTP_CUSTOMIZE_MOST_VISITED_LABEL}, {"myShortcuts", IDS_NTP_CUSTOMIZE_MY_SHORTCUTS_LABEL}, {"shortcutsCurated", IDS_NTP_CUSTOMIZE_MY_SHORTCUTS_DESC}, diff --git a/chrome/test/data/webui/side_panel/customize_chrome/colors_test.ts b/chrome/test/data/webui/side_panel/customize_chrome/colors_test.ts index 4894c9fb7ff8e..c2f7c35e3737c 100644 --- a/chrome/test/data/webui/side_panel/customize_chrome/colors_test.ts +++ b/chrome/test/data/webui/side_panel/customize_chrome/colors_test.ts @@ -13,7 +13,7 @@ import {assertDeepEquals, assertEquals, assertGE, assertTrue} from 'chrome://web import {waitAfterNextRender} from 'chrome://webui-test/polymer_test_util.js'; import {TestBrowserProxy} from 'chrome://webui-test/test_browser_proxy.js'; -import {assertStyle, capture, createBackgroundImage, createTheme, installMock} from './test_support.js'; +import {$$, assertStyle, capture, createBackgroundImage, createTheme, installMock} from './test_support.js'; suite('ColorsTest', () => { let colorsElement: ColorsElement; @@ -46,22 +46,59 @@ suite('ColorsTest', () => { callbackRouter.setTheme(theme); await callbackRouter.$.flushForTesting(); + await waitAfterNextRender(colorsElement); + const defaultColorElement = + $$(colorsElement, '#defaultColor')!; assertDeepEquals( - defaultColor.foreground, - colorsElement.$.defaultColor.foregroundColor); + defaultColor.foreground, defaultColorElement.foregroundColor); assertDeepEquals( - defaultColor.background, - colorsElement.$.defaultColor.backgroundColor); + defaultColor.background, defaultColorElement.backgroundColor); }); }); - test('sets default color', () => { - colorsElement.$.defaultColor.click(); + test('sets default color', async () => { + const theme = createTheme(); + theme.foregroundColor = undefined; + callbackRouter.setTheme(theme); + await callbackRouter.$.flushForTesting(); + await waitAfterNextRender(colorsElement); + + $$(colorsElement, '#defaultColor')!.click(); assertEquals(1, handler.getCallCount('setDefaultColor')); }); + test('renders main color', async () => { + const theme: Theme = createTheme(); + theme.foregroundColor = {value: 7}; + theme.backgroundImage = createBackgroundImage('https://foo.com'); + theme.backgroundImage.mainColor = {value: 7}; + + callbackRouter.setTheme(theme); + await callbackRouter.$.flushForTesting(); + await waitAfterNextRender(colorsElement); + + assertEquals( + 7, + $$(colorsElement, '#mainColor')!.foregroundColor.value); + }); + + test('sets main color', async () => { + const theme = createTheme(); + theme.foregroundColor = {value: 7}; + theme.backgroundImage = createBackgroundImage('https://foo.com'); + theme.backgroundImage.mainColor = {value: 7}; + callbackRouter.setTheme(theme); + await callbackRouter.$.flushForTesting(); + await waitAfterNextRender(colorsElement); + + $$(colorsElement, '#mainColor')!.click(); + + assertEquals(1, handler.getCallCount('setForegroundColor')); + assertEquals(7, handler.getArgs('setForegroundColor')[0].value); + }); + test('renders chrome colors', async () => { const colors = { colors: [ @@ -171,15 +208,36 @@ suite('ColorsTest', () => { theme.foregroundColor = undefined; callbackRouter.setTheme(theme); await callbackRouter.$.flushForTesting(); + await waitAfterNextRender(colorsElement); // Check default color selected. + const defaultColorElement = + $$(colorsElement, '#defaultColor')!; let checkedColors = colorsElement.shadowRoot!.querySelectorAll('[checked]'); assertEquals(1, checkedColors.length); - assertEquals(colorsElement.$.defaultColor, checkedColors[0]); + assertEquals(defaultColorElement, checkedColors[0]); let indexedColors = colorsElement.shadowRoot!.querySelectorAll('[tabindex="0"]'); assertEquals(1, indexedColors.length); - assertEquals(colorsElement.$.defaultColor, indexedColors[0]); + assertEquals(defaultColorElement, indexedColors[0]); + + // Set main color. + theme.foregroundColor = {value: 7}; + theme.backgroundImage = createBackgroundImage('https://foo.com'); + theme.backgroundImage.mainColor = {value: 7}; + callbackRouter.setTheme(theme); + await callbackRouter.$.flushForTesting(); + await waitAfterNextRender(colorsElement); + + // Check main color selected. + const mainColorElement = $$(colorsElement, '#mainColor')!; + checkedColors = colorsElement.shadowRoot!.querySelectorAll('[checked]'); + assertEquals(1, checkedColors.length); + assertEquals(mainColorElement, checkedColors[0]); + indexedColors = + colorsElement.shadowRoot!.querySelectorAll('[tabindex="0"]'); + assertEquals(1, indexedColors.length); + assertEquals(mainColorElement, indexedColors[0]); // Set Chrome color. theme.foregroundColor = {value: 2}; diff --git a/chrome/test/data/webui/side_panel/customize_chrome/test_support.ts b/chrome/test/data/webui/side_panel/customize_chrome/test_support.ts index 3107b072be9fb..1e8ea483f58ad 100644 --- a/chrome/test/data/webui/side_panel/customize_chrome/test_support.ts +++ b/chrome/test/data/webui/side_panel/customize_chrome/test_support.ts @@ -43,6 +43,7 @@ export function createBackgroundImage(url: string): BackgroundImage { url: {url}, isUploadedImage: false, title: '', + mainColor: undefined, }; }