Implement color changing theme settings for Olivero

@andy-blum posted an initial patch for this at https://www.drupal.org/project/drupal/issues/3086514#comment-14347541 within the parent issue.

Went through the code without actually trying it out. Left comments in the MR. This is looking great so far!

Updating the summary to reflect @andy-blum's approach.

Added some minor requests.

We also need the ability to revert back to the default, and with that, it'd be nice to have some options for default (accessible) colors.

Can you create an option to have some default colors (you can pick them out, and we'll have @jwitkowski79 and @jponch change or approve), and add that?

Thanks!

On tugboat in the standard profile, I'm getting the following PHP errors. Not sure if this is your code (don't have time to look deep into it)

root@php:/var/lib/tugboat# drush --uri=standard cr
 [success] Cache rebuild complete.
root@php:/var/lib/tugboat# drush --uri=standard watchdog:show
 ---- -------------- -------- ---------- -------------------------------------------------- 
  ID   Date           Type     Severity   Message                                           
 ---- -------------- -------- ---------- -------------------------------------------------- 
  57   09/Jan 15:30   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  56   09/Jan 15:28   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  55   09/Jan 15:28   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  54   09/Jan 15:25   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  53   09/Jan 15:24   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  52   09/Jan 15:01   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  51   09/Jan 15:01   php      Error      Error: Call to undefined function bcdiv() in      
                                          _hexToHsl() (line 651 of                          
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me) #0                                            
                                          /var/lib/tugboat/core/themes/olivero/olivero.the  
                                          me(27): _hexToH                                   
  50   09/Jan 15:00   system   Info       claro theme installed.                            
  49   09/Jan 15:00   system   Info       olivero theme installed.                          
  48   09/Jan 15:00   user     Notice     Session opened for admin.                         
 ---- -------------- -------- ---------- -------------------------------------------------- 
root@php:/var/lib/tugboat# 

More thorough review incoming, but the reason this is currently failing is:

----------------------------------------------------------------------------------------------------
Checking core/themes/olivero/olivero.theme


FILE: /var/www/html/core/themes/olivero/olivero.theme
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 635 | ERROR | Doc comment short description must end with a full
     |       | stop
 640 | ERROR | Return type "array [$hue, $saturation, $lightness]"
     |       | must not contain spaces
----------------------------------------------------------------------

Time: 185ms; Memory: 8MB


----------------------------------------------------------------------------------------------------
Checking core/themes/olivero/theme-settings.php

PHPCS: core/themes/olivero/theme-settings.php passed
core/themes/olivero/theme-settings.php passed

----------------------------------------------------------------------------------------------------
Drupal code quality checks failed.
To reproduce this output locally:
* Apply the change as a patch
* Run this command locally: sh ./core/scripts/dev/commit-code-check.sh
OR:
* From the merge request branch
* Run this command locally: sh ./core/scripts/dev/commit-code-check.sh --branch 10.0.x
Custom Commands Failed

Looks like the final compiled JS was not committed. Will compile it and continue the review later tonight.

This is looking so great!

Some additional feedback (with images)

1. The icon that appears in maintenance mode should change too
2. The overlay color should be changed to a transparent black.
3. When setting the color scheme to "ice", I see the hue is set to 190.99. This should probably be rounded.

Needs tests. I can work on these if needed.

At line 116 in oliveroColorTest.js, we're getting the following PHP error:

The website encountered an unexpected error. Please try again later.

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for olivero.settings with the following errors: olivero.settings:color_scheme missing schema in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 94 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).
call_user_func(Array, Object, 'config.save', Object) (Line: 108)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'config.save') (Line: 229)
Drupal\Core\Config\Config->save() (Line: 512)
Drupal\system\Form\ThemeSettingsForm->submitForm(Array, Object)
call_user_func_array(Array, Array) (Line: 114)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 52)
Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 592)
Drupal\Core\Form\FormBuilder->processForm('system_theme_settings', Array, Object) (Line: 320)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 152)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 35)
Drupal\form_test\StackMiddleware\FormTestMiddleware->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 679)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

So the problem is that there's a check to ensure that the theme settings are in the olivero.schema.yml file. And the color_scheme is missing (https://git.drupalcode.org/project/drupal/-/merge_requests/1623/diffs#c0...)

So, we have a choice:

1. We can include that setting in the schema (and default config)
2. We can generate the <select> via JavaScript, since it doesn't really do anything unless JavaScript is enabled.

Thoughts?

We're getting a failure on

[18:42:07] '/var/www/html/core/themes/olivero/js/color-picker.es6.js' is being checked.
[18:42:07] '/var/www/html/core/themes/olivero/js/color-picker.es6.js' is not updated.

color-picker.js needed to be recompiled too.

This is looking so good!

In addition to the MR comments above, there's two more:

1) We should nest the two new form fields in a fieldset to visually (and semantically) indicate that they are related.

2) We need to validate the hex string server-side. If I remove the pattern attribute from the input, it allows me to submit invalid colors. This then throws an error on the front-end when it tries to parse it.

This is so close!

A couple changes:

1) We need to modify the .header-nav-overlay CSS class in core/themes/olivero/css/components/header-navigation.pcss.css to make the background transparent black. Currently the MR is adding a new CSS class in base.pcss.css(which we recently refactored out). We need to remove that from the MR.

2) If you have Olivero up and running and apply the patch, the config does not exist. This makes the form look incorrect.

We probably need to add an update hook to add the config. We might be able to get away with seeing if the config exists -- and if it doesn't then load the default blue value. I'm not sure what's best practice.

I asked in the #contribute channel, and this was the response from @alexpott (among others)

the new default value should be set by an update hook. As @chr.fritsch pointed out - that’s not possible at the moment. We need to land #3259188: Extend post update system to provide themes a way to install newly-required dependencies - so that’s a blocker in my opinion. The problem is that an empty value is not a valid value so it is the update system’s job to fix this.

Another update from the same Slack thread (with a workaround that unblocks us).

From @berdir

as a workaround, couldn't we just put a post update in system.module and check if the theme is enabled? didn't check the issue and can't comment on how close it is.

From @alexpott

Yep @berdir’s suggestion would work - the theme update patch is in pretty good shape and would be good to get done. But I guess it should not block olivero stability.

Updating the summary to add discussion regarding the color module.

We discussed this issue at #3267550: Drupal Usability Meeting 2022-03-11 that issue will have a link to the recording.

The key points are

• This looks and functions great
• We should have some text near the top of the fieldset that indicates its function. Something like "You can use these settings to adjust the primary color of Olivero, which will affect various elements."
• Having a refactored color module would be nice. But this should not be a blocker. Olivero will provide an update the use the color module when that option becomes available.
• It would be nice for the preloaded color palettes to have colors indicating what they look like. This is not a blocking issue.

This MR is a thing of beauty. I can't think of anything else. It works well, is robust, and is well tested.

**Chef's Kiss**

This last change looks perfect. The text is announcing on every change now. Video with VoiceOver attached.

At this point we have sign offs from both the UX and accessibility teams!

Comments from @alexpott in Drupal Slack (https://drupal.slack.com/archives/C1BMUQ9U6/p1648555120415739)

alexpott 4 minutes ago
It’s tricky - determining the same value from a constant every request is unnecessary work. The caching for preprocess_html is the page cache :slightly_smiling_face:

alexpott 3 minutes ago
As stated in the comment I think changing the config to an hsl value means we need to convert back to #hex value - which seems awkward so lets not.

alexpott 2 minutes ago
So we need to remove the if because it is unnecessary - the value is always set. And the we need to fix the schema to use the correct type.

mherchel 1 minute ago
gotcha! Thanks for the clarification!
New

alexpott < 1 minute ago
I left the thoughts on storing the HSL values in case someone thought - “oh I know how that could work”

Changes look perfect. I eyeball'ed the code one last time and it looks good. I also tested out the upgrade script, etc, and made sure that works as expected. Everything looks great. Thanks for the work on this!

Created followup #3275990: Create theme function for dynamically inserted (via JS) form elements within Olivero's color changing form

Opened #3280398: Theme's post updates within update.php refer to themes as "module"

This looks amazing! I tested out the update functionality (and opened the followup in #85). I also reviewed the latest commits since my last review, and that looks good. Finally, I tested out the functionality and was unsuccessful in my attempts to break it.

Amazing work everyone!

Will this also be available for D9?

Nope 😭 It relies on CSS variables. D9 has a commitment to support IE11 (which does not support CSS variables).