Customize Chrome: Replace native color picker with custom hue slider

This CL replaces the native color picker with a hue slider for CR2023 as saturation and brightness do not impact themeing in CR2023. The hue slider uses cr-action-menu to position itself since cr-action-menu already handles anchored and off-screen positioning, but is styled like a dialog. Follow-up CLs will clean up positioning, focus states, and update how and when the dialog closes. Bug: 1449969 Change-Id: I93db77fd9eb0f9d72cbe4bc1f8b00d2e18690adf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4705429 Reviewed-by: Thomas Lukaszewicz <tluk@chromium.org> Reviewed-by: Will Harris <wfh@chromium.org> Reviewed-by: Riley Tatum <rtatum@google.com> Commit-Queue: John Lee <johntlee@chromium.org> Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#1184766}
chromium · Aug 17, 2023 · 8b23153 · 8b23153
1 parent a1f62c9
commit 8b23153
Show file tree

Hide file tree

Showing 21 changed files with 378 additions and 14 deletions.
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
@@ -7543,6 +7543,9 @@ Keep your key file in a safe place. You will need it to create new versions of y
       <message name="IDS_NTP_CUSTOMIZE_COLOR_PICKER_LABEL" desc="The label for the color picker in the customization menu on the New Tab Page">
         Custom color
       </message>
+      <message name="IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE" desc="The label for the hue slider in the customization menu on the New Tab Page that allows a user to pick a color for their theme">
+        Color picker
+      </message>
       <message name="IDS_NTP_THEME_MANAGED_DIALOG_TITLE" desc="Title text for the dialog informing users that their theme is managed when they try selecting a theme on the New Tab Page.">
         Theme is set by your Organization
       </message>

diff --git a/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE.png.sha1 b/chrome/app/generated_resources_grd/IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE.png.sha1
@@ -0,0 +1 @@
+f46ec53121d483c0aa87c2991b9b8bb907f326a6
diff --git a/chrome/browser/ui/webui/cr_components/theme_color_picker/theme_color_picker_handler.cc b/chrome/browser/ui/webui/cr_components/theme_color_picker/theme_color_picker_handler.cc
@@ -136,6 +136,12 @@ void ThemeColorPickerHandler::SetSeedColor(
   }
 }
 
+void ThemeColorPickerHandler::SetSeedColorFromHue(float hue) {
+  SetSeedColor(color_utils::HSLToSkColor(
+                   {std::clamp(hue / 360, 0.0f, 1.0f), 1, .5}, 255),
+               ui::mojom::BrowserColorVariant::kTonalSpot);
+}
+
 void ThemeColorPickerHandler::GetChromeColors(
     bool is_dark_mode,
     bool extended_list,
@@ -213,6 +219,9 @@ void ThemeColorPickerHandler::UpdateTheme() {
     theme->color_picker_icon_color =
         web_contents_->GetColorProvider().GetColor(kColorNewTabPageText);
   }
+  color_utils::HSL hsl;
+  color_utils::SkColorToHSL(theme->seed_color, &hsl);
+  theme->seed_color_hue = hsl.h * 360;
   theme->is_grey_baseline = theme_service_->GetIsGrayscale();
   theme->browser_color_variant = theme_service_->GetBrowserColorVariant();
 

diff --git a/chrome/browser/ui/webui/cr_components/theme_color_picker/theme_color_picker_handler.h b/chrome/browser/ui/webui/cr_components/theme_color_picker/theme_color_picker_handler.h
@@ -51,6 +51,7 @@ class ThemeColorPickerHandler
   void SetGreyDefaultColor() override;
   void SetSeedColor(SkColor seed_color,
                     ui::mojom::BrowserColorVariant variant) override;
+  void SetSeedColorFromHue(float hue) override;
   void GetChromeColors(bool is_dark_mode,
                        bool extended_list,
                        GetChromeColorsCallback callback) override;

diff --git a/chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc b/chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.cc
@@ -283,6 +283,7 @@ content::WebUIDataSource* CreateAndAddNewTabPageUiHtmlSource(Profile* profile) {
       {"backgroundsMenuItem", IDS_NTP_CUSTOMIZE_MENU_BACKGROUND_LABEL},
       {"cancelButton", IDS_CANCEL},
       {"colorPickerLabel", IDS_NTP_CUSTOMIZE_COLOR_PICKER_LABEL},
+      {"hueSliderTitle", IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE},
       {"customBackgroundDisabled",
        IDS_NTP_CUSTOMIZE_MENU_BACKGROUND_DISABLED_LABEL},
       {"customizeButton", IDS_NTP_CUSTOMIZE_BUTTON_LABEL},

diff --git a/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc b/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
@@ -1710,6 +1710,7 @@ void AddPeopleStrings(content::WebUIDataSource* html_source, Profile* profile) {
     {"defaultColorName", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL},
     {"defaultThemeLabel", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL},
     {"greyDefaultColorName", IDS_NTP_CUSTOMIZE_GREY_DEFAULT_LABEL},
+    {"hueSliderTitle", IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE},
     {"mainColorName", IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL},
     {"managedColorsBody", IDS_NTP_THEME_MANAGED_DIALOG_BODY},
     {"managedColorsTitle", IDS_NTP_THEME_MANAGED_DIALOG_TITLE},

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
@@ -75,6 +75,7 @@ CustomizeChromeUI::CustomizeChromeUI(content::WebUI* web_ui)
       {"currentTheme", IDS_NTP_CUSTOMIZE_CHROME_CURRENT_THEME_LABEL},
       {"defaultColorName", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL},
       {"greyDefaultColorName", IDS_NTP_CUSTOMIZE_GREY_DEFAULT_LABEL},
+      {"hueSliderTitle", IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE},
       {"mainColorName", IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL},
       {"managedColorsTitle", IDS_NTP_THEME_MANAGED_DIALOG_TITLE},
       {"managedColorsBody", IDS_NTP_THEME_MANAGED_DIALOG_BODY},

diff --git a/chrome/browser/ui/webui/signin/profile_customization_ui.cc b/chrome/browser/ui/webui/signin/profile_customization_ui.cc
@@ -85,11 +85,13 @@ ProfileCustomizationUI::ProfileCustomizationUI(content::WebUI* web_ui)
        IDS_PROFILE_CUSTOMIZATION_AVATAR_SELECTION_BACK_BUTTON_LABEL},
 
       // Color picker strings:
+      {"close", IDS_CLOSE},
       {"colorPickerLabel", IDS_NTP_CUSTOMIZE_COLOR_PICKER_LABEL},
       {"colorsContainerLabel", IDS_NTP_THEMES_CONTAINER_LABEL},
       {"currentTheme", IDS_NTP_CUSTOMIZE_CHROME_CURRENT_THEME_LABEL},
       {"defaultColorName", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL},
       {"defaultThemeLabel", IDS_NTP_CUSTOMIZE_DEFAULT_LABEL},
+      {"hueSliderTitle", IDS_NTP_CUSTOMIZE_COLOR_HUE_SLIDER_TITLE},
       {"greyDefaultColorName", IDS_NTP_CUSTOMIZE_GREY_DEFAULT_LABEL},
       {"mainColorName", IDS_NTP_CUSTOMIZE_MAIN_COLOR_LABEL},
       {"managedColorsBody", IDS_NTP_THEME_MANAGED_DIALOG_BODY},

diff --git a/chrome/test/data/webui/cr_components/BUILD.gn b/chrome/test/data/webui/cr_components/BUILD.gn
@@ -21,6 +21,7 @@ build_webui_tests("build") {
     "theme_color_picker_focus_test.ts",
     "theme_color_test.ts",
     "theme_color_picker_test.ts",
+    "theme_hue_slider_dialog_test.ts",
     "customize_themes_test.ts",
     "help_bubble_mixin_test.ts",
     "help_bubble_test.ts",

diff --git a/chrome/test/data/webui/cr_components/cr_components_browsertest.cc b/chrome/test/data/webui/cr_components/cr_components_browsertest.cc
@@ -102,6 +102,11 @@ IN_PROC_BROWSER_TEST_F(CrComponentsTest, ThemeColorPicker) {
   RunTest("cr_components/theme_color_picker_test.js", "mocha.run()");
 }
 
+IN_PROC_BROWSER_TEST_F(CrComponentsTest, ThemeHueSliderDialog) {
+  set_test_loader_host(chrome::kChromeUICustomizeChromeSidePanelHost);
+  RunTest("cr_components/theme_hue_slider_dialog_test.js", "mocha.run()");
+}
+
 typedef WebUIMochaBrowserTest CrComponentsAppManagementTest;
 IN_PROC_BROWSER_TEST_F(CrComponentsAppManagementTest, PermissionItem) {
   RunTest("cr_components/app_management/permission_item_test.js",

diff --git a/chrome/test/data/webui/cr_components/theme_color_picker_test.ts b/chrome/test/data/webui/cr_components/theme_color_picker_test.ts
@@ -10,7 +10,6 @@ import {Color, DARK_BASELINE_BLUE_COLOR, DARK_BASELINE_GREY_COLOR, DARK_DEFAULT_
 import {ThemeColorElement} from 'chrome://resources/cr_components/theme_color_picker/theme_color.js';
 import {ThemeColorPickerElement} from 'chrome://resources/cr_components/theme_color_picker/theme_color_picker.js';
 import {ChromeColor, Theme, ThemeColorPickerClientCallbackRouter, ThemeColorPickerClientRemote, ThemeColorPickerHandlerRemote} from 'chrome://resources/cr_components/theme_color_picker/theme_color_picker.mojom-webui.js';
-import {skColorToHexColor} from 'chrome://resources/js/color_utils.js';
 import {PromiseResolver} from 'chrome://resources/js/promise_resolver.js';
 import {BrowserColorVariant} from 'chrome://resources/mojo/ui/base/mojom/themes.mojom-webui.js';
 import {assertDeepEquals, assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
@@ -26,6 +25,7 @@ function createTheme(isDarkMode = false): Theme {
     backgroundImageMainColor: undefined,
     isDarkMode,
     seedColor: {value: 0xff0000ff},
+    seedColorHue: 0,
     backgroundColor: {value: 0xffff0000},
     foregroundColor: undefined,
     colorPickerIconColor: {value: 0xffff0000},
@@ -285,7 +285,7 @@ suite('CrComponentsThemeColorPickerTest', () => {
     assertEquals(BrowserColorVariant.kNeutral, args[1]);
   });
 
-  test('sets custom color', () => {
+  test('sets custom color from color input for non-CR2023', async () => {
     initializeElement();
     colorsElement.$.colorPicker.value = '#ff0000';
     colorsElement.$.colorPicker.dispatchEvent(new Event('change'));
@@ -296,6 +296,22 @@ suite('CrComponentsThemeColorPickerTest', () => {
     assertEquals(BrowserColorVariant.kTonalSpot, args[1]);
   });
 
+  test('sets custom color from hue slider for CR2023', async () => {
+    document.documentElement.toggleAttribute('chrome-refresh-2023', true);
+    initializeElement();
+    callbackRouter.setTheme(Object.assign(createTheme(), {seedColorHue: 10}));
+    await callbackRouter.$.flushForTesting();
+
+    colorsElement.$.hueSlider.selectedHue = 10;
+    colorsElement.$.hueSlider.dispatchEvent(new Event('selected-hue-changed'));
+    assertEquals(0, handler.getCallCount('setSeedColorFromHue'));
+
+    colorsElement.$.hueSlider.selectedHue = 30;
+    colorsElement.$.hueSlider.dispatchEvent(new Event('selected-hue-changed'));
+    assertEquals(1, handler.getCallCount('setSeedColorFromHue'));
+    assertEquals(30, handler.getArgs('setSeedColorFromHue')[0]);
+  });
+
   test('updates custom color for theme', async () => {
     initializeElement();
     const colors = {
@@ -364,31 +380,28 @@ suite('CrComponentsThemeColorPickerTest', () => {
 
     // Set a custom color theme.
     const customColortheme = createTheme();
-    customColortheme.seedColor = {value: 0xffffff00};
+    customColortheme.seedColorHue = 22;
     customColortheme.backgroundColor = {value: 0xffff0000};
     customColortheme.foregroundColor = {value: 0xff00ff00};
     customColortheme.colorPickerIconColor = {value: 0xff0000ff};
     callbackRouter.setTheme(customColortheme);
     await callbackRouter.$.flushForTesting();
 
-    // Custom colorpicker value should be updated.
+    // Custom hue slider value should be updated.
     assertEquals(
-        colorsElement.$.colorPicker.value,
-        skColorToHexColor(customColortheme.seedColor));
+        colorsElement.$.hueSlider.selectedHue, customColortheme.seedColorHue);
 
     // Set a theme that is not a custom color theme.
     const otherTheme = createTheme();
-    otherTheme.seedColor = {value: 0xff00ff00};
     otherTheme.backgroundColor = {value: 0xffffffff};
     otherTheme.foregroundColor = undefined;  // Makes a default theme.
     otherTheme.colorPickerIconColor = {value: 0xffffffff};
     callbackRouter.setTheme(otherTheme);
     await callbackRouter.$.flushForTesting();
 
-    // Custom colorpicker should still be the same.
+    // Custom hue slider value should still be the same.
     assertEquals(
-        colorsElement.$.colorPicker.value,
-        skColorToHexColor(customColortheme.seedColor));
+        colorsElement.$.hueSlider.selectedHue, customColortheme.seedColorHue);
   });
 
   test('checks selected color', async () => {

diff --git a/chrome/test/data/webui/cr_components/theme_hue_slider_dialog_test.ts b/chrome/test/data/webui/cr_components/theme_hue_slider_dialog_test.ts
@@ -0,0 +1,82 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'chrome://customize-chrome-side-panel.top-chrome/app.js';
+
+import {ThemeHueSliderDialogElement} from 'chrome://resources/cr_components/theme_color_picker/theme_hue_slider_dialog.js';
+import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
+import {eventToPromise} from 'chrome://webui-test/test_util.js';
+
+suite('CrComponentsThemeHueSliderDialogTest', () => {
+  let element: ThemeHueSliderDialogElement;
+
+  setup(() => {
+    document.body.innerHTML = window.trustedTypes!.emptyHTML;
+    element = document.createElement('cr-theme-hue-slider-dialog');
+    document.body.appendChild(element);
+  });
+
+  test('SetsUpCrSliderValues', () => {
+    assertEquals(0, element.$.slider.min);
+    assertEquals(359, element.$.slider.max);
+
+    element.selectedHue = 200;
+    assertEquals(200, element.$.slider.value);
+  });
+
+  test('UpdatesCrSliderUi', () => {
+    const knobStyle = window.getComputedStyle(element.$.slider.$.knob);
+    element.$.slider.value = 200;
+    element.$.slider.dispatchEvent(new CustomEvent('cr-slider-value-changed'));
+
+    // window.getComputedStyle only returns color values in rgb format, so
+    // rgb(0, 170, 255) is manually converted from hsl(200, 100%, 50%).
+    assertEquals('rgb(0, 170, 255)', knobStyle.backgroundColor);
+
+    element.$.slider.value = 300;
+    element.$.slider.dispatchEvent(new CustomEvent('cr-slider-value-changed'));
+    // rgb(255, 0, 255) is manually converted from hsl(300, 100%, 50%).
+    assertEquals('rgb(255, 0, 255)', knobStyle.backgroundColor);
+  });
+
+  test('UpdatesSelectedHue', () => {
+    element.selectedHue = 0;
+
+    // Changing the slider itself should not update selected hue.
+    element.$.slider.value = 100;
+    element.$.slider.dispatchEvent(new CustomEvent('cr-slider-value-changed'));
+    assertEquals(0, element.selectedHue);
+
+    // Only on pointerup should the selectedHue change.
+    element.$.slider.dispatchEvent(new PointerEvent('pointerup'));
+    assertEquals(100, element.selectedHue);
+
+    // Changing the slider itself should not update selected hue.
+    element.$.slider.value = 250;
+    element.$.slider.dispatchEvent(new CustomEvent('cr-slider-value-changed'));
+    assertEquals(100, element.selectedHue);
+
+    // Only on keyup should the selectedHue change.
+    element.$.slider.dispatchEvent(new KeyboardEvent('keyup'));
+    assertEquals(250, element.selectedHue);
+  });
+
+  test('DispatchesSelectedHueChangedEvent', async () => {
+    const selectedHueChangedEvent =
+        eventToPromise('selected-hue-changed', element);
+    element.$.slider.value = 100;
+    element.$.slider.dispatchEvent(new PointerEvent('pointerup'));
+    const e = await selectedHueChangedEvent;
+    assertEquals(100, e.detail.selectedHue);
+  });
+
+  test('ShowsAndHides', () => {
+    const anchor = document.createElement('div');
+    document.body.appendChild(anchor);
+    element.showAt(anchor);
+    assertTrue(element.$.crActionMenu.getDialog().open);
+    element.hide();
+    assertFalse(element.$.crActionMenu.getDialog().open);
+  });
+});
diff --git a/chrome/test/data/webui/side_panel/customize_chrome/chrome_colors_test.ts b/chrome/test/data/webui/side_panel/customize_chrome/chrome_colors_test.ts
@@ -23,6 +23,7 @@ function createTheme(): Theme {
     backgroundImageMainColor: undefined,
     isDarkMode: false,
     seedColor: {value: 0xff0000ff},
+    seedColorHue: 0,
     backgroundColor: {value: 0xffff0000},
     foregroundColor: undefined,
     colorPickerIconColor: {value: 0xffff0000},

diff --git a/ui/color/color_id.h b/ui/color/color_id.h
@@ -468,6 +468,10 @@
   E_CPONLY(kColorThemeColorPickerCheckmarkBackground) \
   E_CPONLY(kColorThemeColorPickerCheckmarkForeground) \
   E_CPONLY(kColorThemeColorPickerCustomColorIconBackground) \
+  E_CPONLY(kColorThemeColorPickerHueSliderDialogBackground) \
+  E_CPONLY(kColorThemeColorPickerHueSliderDialogForeground) \
+  E_CPONLY(kColorThemeColorPickerHueSliderDialogIcon) \
+  E_CPONLY(kColorThemeColorPickerHueSliderHandle) \
   E_CPONLY(kColorThemeColorPickerOptionBackground) \
   E_CPONLY(kColorThrobber) \
   E_CPONLY(kColorThrobberPreconnect) \

diff --git a/ui/color/material_ui_color_mixer.cc b/ui/color/material_ui_color_mixer.cc
@@ -151,6 +151,10 @@ void AddMaterialUiColorMixer(ColorProvider* provider,
       kColorSysInverseOnSurface};
   mixer[kColorThemeColorPickerCustomColorIconBackground] = {
       kColorSysOnSurfaceSubtle};
+  mixer[kColorThemeColorPickerHueSliderDialogBackground] = {kColorSysSurface};
+  mixer[kColorThemeColorPickerHueSliderDialogForeground] = {kColorSysOnSurface};
+  mixer[kColorThemeColorPickerHueSliderDialogIcon] = {kColorSysOnSurfaceSubtle};
+  mixer[kColorThemeColorPickerHueSliderHandle] = {kColorSysWhite};
   mixer[kColorThemeColorPickerOptionBackground] = {kColorSysNeutralContainer};
   mixer[kColorToastBackground] = {kColorSysInverseSurface};
   mixer[kColorToastButton] = {kColorSysInversePrimary};

diff --git a/ui/webui/resources/cr_components/theme_color_picker/BUILD.gn b/ui/webui/resources/cr_components/theme_color_picker/BUILD.gn
@@ -27,6 +27,7 @@ build_webui("build") {
     "check_mark_wrapper.ts",
     "theme_color.ts",
     "theme_color_picker.ts",
+    "theme_hue_slider_dialog.ts",
   ]
 
   non_web_component_files = [

diff --git a/ui/webui/resources/cr_components/theme_color_picker/theme_color_picker.html b/ui/webui/resources/cr_components/theme_color_picker/theme_color_picker.html
@@ -165,6 +165,11 @@
         on-change="onCustomColorChange_">
   </div>
 </cr-grid>
+
+<cr-theme-hue-slider-dialog id="hueSlider"
+    on-selected-hue-changed="onSelectedHueChanged_">
+</cr-theme-hue-slider-dialog>
+
 <template is="dom-if" if="[[showManagedDialog_]]" restamp>
   <managed-dialog on-close="onManagedDialogClosed_"
       title="[[i18n('managedColorsTitle')]]"

diff --git a/ui/webui/resources/cr_components/theme_color_picker/theme_color_picker.mojom b/ui/webui/resources/cr_components/theme_color_picker/theme_color_picker.mojom
@@ -19,6 +19,8 @@ struct Theme {
   bool is_dark_mode;
   // The color this theme was calculated from.
   skia.mojom.SkColor seed_color;
+  // The hue of the color this theme was calculated from.
+  float seed_color_hue;
   // The current theme background color.
   skia.mojom.SkColor background_color;
   // The current theme foreground color. If not set, we use the default theme.
@@ -77,6 +79,9 @@ interface ThemeColorPickerHandler {
   // Sets a Chrome theme generated from |seed_color|.
   SetSeedColor(skia.mojom.SkColor seed_color, ui.mojom.BrowserColorVariant variant);
 
+  // Sets a Chrome theme generated from |hue|.
+  SetSeedColorFromHue(float hue);
+
   // Removes background image.
   RemoveBackgroundImage();
 };