Introduce a mechanism to provide an alternate Claro design for the toolbar in the future

Is there a reason the designs can't be included in Claro, then added to Toolbar module when stable? That's what we typically do when experimental modules need to add to themes. They ship the templates and CSS themselves initially, and then they're copied to the appropriate theme when stable.

We do have one or two past example I can think of where we referenced something less stable or not in core, but those were situations where we had to in order to avoid incompatible modules from being enabled and it's something we try to avoid.

Is there a reason the designs can't be included in Claro, then added to Toolbar module when stable?

This would work for Claro when its included in Drupal Core. However, if I am interpreting the proposed resolution correctly, laurii suggests adding an API to render elements in the Admin theme (eg. Claro) when viewing a page that is not the current admin theme (eg. Bartik). This would then enable contrib admin themes (eg. Adminimal) to apply their styling to the Toolbar as well.

Although it's now possible for themes to declare dependencies on Modules, it's not currently possible to enable those depended-upon modules as part of enabling the theme. Were a module to do this it would probably work best as a hidden one, but this couldn't happen since it would need to be explicitly enabled.

Because of this, I'm more inclined to go with a customization to an existing module.

This patch adds logic to toolbar.module that makes it possible to override and extend toolbar libraries like any other library, but it will be applied to the front end theme as well. This could pretty easily be moved to a module or somewhere else centralized as very little about it is toolbar-specific.

Just thought of a cleaner way to implement #9. Move that to Claro.theme with a small change
change
if ($extension === 'toolbar' && !\Drupal::service('router.admin_context')->isAdminRoute() && $admin_theme === 'claro') {

to
if (in_array($extension, $modules_claro_can_style_on_fe) && !\Drupal::service('router.admin_context')->isAdminRoute() && $admin_theme === 'claro') {

That way, if Claro needs the ability to style additional modules on the front end, it only requires adding a new item to the $modules_claro_can_style_on_fe array.

(if this hook doesn't fire in .theme files, that can be changed pretty easily)

If that approach works this can probably be re-scoped to Claro.

EDIT: that wont work - those hooks don't fire when it isn't the active theme (got thrown because the libraries are available)
The ease in which a new module can be added seems to favor using this approach in a module. My concern about the module won't really be an issue once Claro is the default admin theme...

@bnjmnm what if we add an module which basically just adds the toolbar stylesheet to the frontend when the user is logged in? So everything stays in Claro itself (more of a workaround until it's the default theme) and it can be deprecated later on.

That's the approach I've done with Gin & Gin_toolbar. Toolbar is a module which injects the toolbar CSS from the Gin theme.

You could check & inject that via the toolbar module.

I took a cue from #11 and created a module. This patch includes that and some extra code to enable manual review.

Any modules that Claro should have access to should be specified in claro.info.yml via the front-end-library-access: property.

front-end-library-access:
  - toolbar

If this is specified, then all of Claro's libraries-override and libraries-extend will modify those module's libraries on front end as well.

To try this out:

1. Apply the patch
2. Enable the claro_front_end module
3. In claro.info.yml, there are several commented configurations in libraries-override: and libraries-extend: that provide examples of this module working. Uncomment and clear cache to see examples of extending and overriding in different ways.

This approach means that when it comes time for Claro to modify quickedit, contextual links, etc, they will have that ability by adding the module to front-end-library-access:

#12 failed tests as it adds a new module but the composer part hasn't been done. This can still be manually tested and should get that before anything additional is done.

This adds the ability for Claro to override templates as well. It will only do this to templates that were added to the registry in modules specified in front-end-library-access:. This may get ugly if it was necessary to do something from the system module, but there are other ways to implement this if that seems needed.

Like previously mentioned in #14, these test failures are due to not integrating composer yet. Switching to Needs Review as what is there does merit review as it proves the concept of Claro being able to selectively control front end libraries and templates.

As one of the people involved with the current Toolbar module: I just found this issue, I'm following it, feel free to ping me to help you find pointers for original design decisions. I can't promise anything, but I can probably help you find answers a bit faster 🤓

@lauriii reviewed this with Gábor and I on the Monthly Cross-Core Collaboration meeting. (Only speaking for myself here, but did run this past Gábor and he agreed.)

Questions:
Should admin themes be able to control admin components on the frontend? This includes Toolbar, Quick Edit, Contextual Links, Media Library, Layout Builder and Settings Tray.

I don't think I can speak to actual implementation of the solution, but the problem being raised, where depending on what path you're on, you see one style of toolbar/etc. or another, is extremely disruptive to the overall user experience, so huge +1 to fixing it. It's an enormous visual regression otherwise, and seems borderline critical.

Logically though, it does make a lot of sense that the themes (+ theme-related modules) would be responsible for the styling, vs. functional modules. So +1 to the general gist of this issue.

If needed, can we introduce a new visible module that supports Claro? Users will have to enable this module manually, prior to installing Claro since theme installer cannot install modules.

Hm. I'm really sorry, but that does not sound like acceptable UX to me, unless there were zero other options of getting Claro out in time for 9.1. :\

Because people generally do not read docs, they will naturally go to Admin > Appearance to adjust their theme and expect that when they check-off Claro it will "just work." (Just like if you check off "Migrate Drupal UI" in modules, it will automatically enable the dependent modules that it needs to function.) We would need some kind of kludgey UI-based workaround to circumvent this natural behaviour (For example, greying out the ability to install, supplementing it with help text that directs them back to the modules page with manual instructions on what to do once they're there), and rather than bikeshedding about what that kludgey workaround is/could be, it would be way preferable to just fix the underlying issue that is forcing this workflow.

Which of the proposed solutions seem most feasible?

It sounds like the "automatically enable dependencies" bit was de-scoped from the original "let themes depend on modules" patch. It would be great to try and understand what the "level of effort" would be to implement that missing piece, because that seems like it would help not only Claro but also any contrib/custom theme that wants to also depend on a module. This also prevents the need of inventing + testing + documenting some new API for this.

In other words, Option 4 without the cons would be my vote, FWIW. :) Better still if the underlying module dependency can be hidden, but not a hard requirement.

Two other "product management-y" things:

1. I see several cons in the list that relate to contingency planning if Claro one day gets removed. From a product management POV, that is not going to happen. We demonstrated "beyond a reasonable doubt" back when we last surveyed the community in 2018 to determine why D8 was not being adopted, that the lack of a modern admin experience is one of the top reasons, and so even if Claro dithered around in beta for another 8 releases (hopefully not :)), we would not want to remove it from core. We need it in order to increase Drupal adoption, the accessibility improvements are already light-years beyond Seven, multiple high-profile contrib modules such as Webform have already done integration work with it, and there are not exactly a swarm of competing initiatives trying to also update Drupal's admin theme. :P So our assumptions when comparing/contrasting solutions should proceed along the lines that Claro is going to replace Seven as the default admin theme at some point in the D9 cycle, and that the sooner that happens, the better for everyone.

2. We should also recognize that the number of people who are going to write admin themes, and esp. admin themes that are extremely opinionated on what "admin-y" front-end elements like Toolbar, etc. look like, is infinitesimally small. We can count those projects on one hand. We should therefore be careful about the "perfect being the enemy of the good" in this regard. There is far, far greater value to Drupal to Claro being in the hands of site builders sooner than later vs. having the perfectly architected solution for this particular edge case.

#20 +1

I think this issue #3100374: Make it possible to install dependent modules when installing theme will make "option 4 without the cons" - possible.

There was interest in a proof of concept for loading Claro assets in FE themes on a per-module basis. This patch is a proof of concept for Toolbar, and includes a Claro toolbar patch from #3020422: Toolbar style update. The patch itself is large due to including the Claro styles, but the approach itself is only a few changes in toolbar.module

function _toolbar_is_claro_admin() {
   $admin_theme = \Drupal::configFactory()->get('system.theme')->get('admin');
   return !\Drupal::service('router.admin_context')->isAdminRoute() && $admin_theme === 'claro';
}

/**
 * Implements hook_library_info_alter().
 *
 * The default theme will use Claro's styles when Claro is the admin theme.
 */
function toolbar_library_info_alter(&$libraries, $extension) {
  if (_toolbar_is_claro_admin() && $extension === 'toolbar') {
    $toolbar_css = &$libraries['toolbar']['css'];
    $css_to_override = [
      'css/toolbar.module.css' => [
        'concern' => 'component',
        'new_key' => '/core/themes/claro/css/components/toolbar.module.css',
      ],
      'css/toolbar.theme.css' => [
        'concern' => 'theme',
        'new_key' => '/core/themes/claro/css/theme/toolbar.theme.css',
      ],
      'css/toolbar.icons.theme.css' => [
        'concern' => 'theme',
        'new_key' => '/core/themes/claro/css/theme/toolbar.icons.theme.css',
      ],
    ];
    foreach ($css_to_override as $old_key => $override) {
      $concern = $override['concern'];
      $asset_properties = $toolbar_css[$concern][$old_key];
      // Remove the existing toolbar CSS.
      unset($toolbar_css[$concern][$old_key]);

      // Add Claro's Toolbar CSS.
      $toolbar_css[$concern][$override['new_key']] = $asset_properties;
    }
  }
}

And inside template_preprocess_toolbar():

// If Claro is the admin theme, Toolbar will be styled by Claro and should
  // receive the same preprocessing it would in Claro.
  if (_toolbar_is_claro_admin()) {
    require_once DRUPAL_ROOT . '/core/themes/claro/claro.theme';
    claro_preprocess_toolbar($variables);
  }

+1 to proceeding with something along the lines of #22 rather than postponing this on #3100374: Make it possible to install dependent modules when installing theme. In terms of implementation:

1. +++ b/core/modules/toolbar/toolbar.module
   @@ -290,3 +298,42 @@ function _toolbar_get_subtrees_hash() {
   +function toolbar_library_info_alter(&$libraries, $extension) {
   

   Rather than inlining the implementation into this function, let's have this call claro_library_info_alter() and put the implementation there. However, in that case it would be that function checking $extension === 'toolbar', which means this wrapper function more properly belongs in system.module.

2. +++ b/core/modules/toolbar/toolbar.module
   @@ -290,3 +298,42 @@ function _toolbar_get_subtrees_hash() {
   +    $css_to_override = [
   +      'css/toolbar.module.css' => [
   +        'concern' => 'component',
   +        'new_key' => '/core/themes/claro/css/components/toolbar.module.css',
   +      ],
   +      'css/toolbar.theme.css' => [
   +        'concern' => 'theme',
   +        'new_key' => '/core/themes/claro/css/theme/toolbar.theme.css',
   +      ],
   +      'css/toolbar.icons.theme.css' => [
   +        'concern' => 'theme',
   +        'new_key' => '/core/themes/claro/css/theme/toolbar.icons.theme.css',
   +      ],
   +    ];
   

   This duplicates what's in claro.info.yml, so rather than duplicating it, how about parsing it from that file instead?

3. +++ b/core/modules/toolbar/toolbar.module
   @@ -290,3 +298,42 @@ function _toolbar_get_subtrees_hash() {
   +   return !\Drupal::service('router.admin_context')->isAdminRoute() && $admin_theme === 'claro';
   

   I think the intent here is to return true when Claro is the admin theme and not the active theme, because if it's the active theme, then the theme system is already making it participate in preprocessing and library overriding. However, !isAdminRoute() isn't a sufficient guarantee that it's not the active theme. What if Claro is also set as the front-end theme? Or, what if a theme negotiator decides to use Claro for a non-admin route? So instead of checking for admin route, what about checking ThemeManagerInterface::getActiveTheme()?

4. +++ b/core/modules/toolbar/toolbar.module
   @@ -290,3 +298,42 @@ function _toolbar_get_subtrees_hash() {
   +function _toolbar_is_claro_admin() {
   

   If we're moving toolbar_library_info_alter() to system_library_info_alter(), then this needs to move to system.module as well. Also, per above, we're not just checking that claro is the admin theme, but also that it's not the active theme, so perhaps rename to _system_is_claro_admin_and_not_active() or _system_claro_is_inactive_admin_theme().

5. +++ b/core/modules/toolbar/toolbar.module
   +++ b/core/modules/toolbar/toolbar.module
   @@ -129,6 +129,14 @@ function template_preprocess_toolbar(&$variables) {
   

   If we move _toolbar_is_claro_admin() to system.module, then we should probably move this function's additions to system_preprocess_toolbar() as well. Which also has the benefit that if in other issues we need to implement preprocess wrappers for other (non-toolbar) components, then they can all be centralized in system.module.

6. Because this patch adds logic that's conditional on whether Claro is the admin theme, we also need to add cache invalidation for when that changes. In the preprocess function, we need to add $variables['#cache']['tags'][] = 'config:system.theme'. And for the library_info_alter, we need to add an event subscriber for ConfigEvents::SAVE, to invalidate the 'library_info' cache tag if the admin theme got changed.

To facilitate manual review this still includes the patch from #3020422: Toolbar style update

This is what is specific to this issue:
system.module

/**
 * Determines if Claro is the admin theme but not the active theme.
 *
 * @return boolean
 *   TRUE if Claro is the admin theme but not the active theme.
 */
function _system_is_claro_admin_and_not_active() {
  $admin_theme = \Drupal::configFactory()->get('system.theme')->get('admin');
  $active_theme = \Drupal::theme()->getActiveTheme()->getName();
  return $active_theme !== 'claro' && $admin_theme === 'claro';
}

/**
 * Implements hook_library_info_alter().
 */
function system_library_info_alter(&$libraries, $extension) {
  // If Claro is the admin theme but not the active theme, grant Claro the
  // ability to override the toolbar library with its own assets.
  if ($extension === 'toolbar' && _system_is_claro_admin_and_not_active()) {
    require_once DRUPAL_ROOT . '/core/themes/claro/claro.theme';
    claro_system_module_invoked_library_info_alter($libraries, $extension);
  }
}

/**
 * Implements hook_preprocess_toolbar().
 */
function system_preprocess_toolbar(array &$variables) {
  // If Claro is the admin theme but not the active theme, still include Claro's
  // toolbar preprocessing.
  if (_system_is_claro_admin_and_not_active()) {
    $variables['#cache']['tags'][] = 'config:system.theme';
    require_once DRUPAL_ROOT . '/core/themes/claro/claro.theme';
    claro_preprocess_toolbar($variables);
  }
}

claro.theme

/**
 * Called via system.module via its hook_library_info_alter().
 *
 * hook_library_info_alter() is not directly called in .theme files.
 */
function claro_system_module_invoked_library_info_alter(&$libraries, $extension) {
  if ($extension === 'toolbar') {
    $claro_info = \Drupal::service('theme_handler')->listInfo()['claro']->info;
    $path_prefix = '/core/themes/claro/';
    $claro_toolbar_overrides = $claro_info['libraries-override']['toolbar/toolbar'];
    foreach ($claro_toolbar_overrides['css'] as $concern => $overrides) {
      foreach ($claro_toolbar_overrides['css'][$concern] as $key => $value) {
        $config = $libraries['toolbar']['css'][$concern][$key];
        $libraries['toolbar']['css'][$concern][$path_prefix . $value] = $config;
        unset($libraries['toolbar']['css'][$concern][$key]);
      }
    }
  }
}

\Drupal\system\EventSubscriber\ConfigCacheTag

// Library overrides potentially change for the default theme when the admin
    // theme is changed.
    if ($config_name === 'system.theme' && $event->isChanged('admin')) {
      $this->cacheTagsInvalidator->invalidateTags(['library_info']);
    }

This adds tests and makes the patch reviewable and committable without having to add or consult #3020422: Toolbar style update. Claro does override three toolbar CSS files, but the files they are overridden with are copied from system module, and only edited to change the relative paths of background images. The toolbar styles are unchanged, but they now come from files located in Claro.

Made some changes so my locally-passing tests don't fail on DrupalCI like they did in #26.
Also expanded the functionality to include toolbar.menu.css as that's been added to the scope of #3020422: Toolbar style update, and this is reflected in the tests.

fail in #28 was an unrelated test known to intermittently fail.

Addresses #31

The issue #3020422: Toolbar style update will also be overriding templates, so that has been added to the scope here, and the test coverage was expanded to reflect that addition.

Addresses #34 and #35 plus a few CS nits, added a missing toolbar.menu.pcss.css (previously only the .css had been added), and moved some of the logic in system_theme_registry_alter() to claro.theme, similar to the pattern used by system_library_info_alter()

Adding the cachetag unconditionally required updating a test.

#41.1

Do we have a specific reason we haven't used same pattern for naming the claro_preprocess_toolbar as we have for the hook_registry_alter and hook_library_info_alter?

Yes, claro_preprocess_toolbar is used by both Claro and the non-Claro default theme using Claro's toolbar styles. The hook_registry_alter and hook_library_info_alter implementations are not needed by Claro, only by the non-Claro default theme. The reason for placing the functions in claro.theme can be found in #23

#41.2 The test was expanded to cover stylesheet loading order.

Test failure was an unrelated test known to randomly fail. Returning to prior status.

What exactly is RTBC here?

• There are no screenshots here. Surely we'd want those before RTBC if this is introduces a new design?
• I'm confused about the relationship between this issue, and #3020422: Toolbar style update. Both issues have large patches, and they are touching many of the same files.

There are no screenshots here. Surely we'd want those before RTBC if this is introduces a new design?

There's no new design in this patch. This adds functionality where - if Claro is the admin theme - Claro will style the toolbar in the default theme as well, whether or not the default theme is Claro. This makes it possible for Claro to update the toolbar styles and have them be consistent whether or not the user is visiting admin pages.

'm confused about the relationship between this issue, and #3020422: Toolbar style update. Both issues have large patches, and they are touching many of the same files.

Now that you mention it, this distinction doesn't seem like it would be clear to those who haven't been working closely on these issues. Tagging with "Needs issue summary update". It may be me doing the IS update, but can't at this moment.

Thanks. So this is the underlying mechanism to load libraries? I think I made a suggestion for such a thing elsewhere, a while back.

Will this work for any admin theme, such as those provided by some distros?

This still needs an issue summary update as to what is going on.

I updated the issue summary.

Re: #48

So this is the underlying mechanism to load libraries? I think I made a suggestion for such a thing elsewhere, a while back.

Will this work for any admin theme, such as those provided by some distros?

Not in the scope of this issue, this is specific to just Toolbar CSS/Templates, and only when Claro is the admin theme. #20 elaborates on why this approach is favored over waiting on the implementation of broader solutions such as #2195695: Admin UIs on the front-end are difficult to theme or #2632584: Add a "public admin" theme subtype so that admin themes can have subthemes for front-end UI components.

Retitling based on that. Noticed the release/framework manager signoffs are not yet provided, so cannot commit.

Reroll.

With the current scope of this issue, I don't see any release management implications to this patch, so removing the need for release manager review.

This looks good to me from a framework management perspective, so removing that tag too.

Here's some trivial whitespace changes for coding standards.

Pushed to 9.2.x and 9.1.x.