Remove Settings Tray integration in Default Admin, keep in Stable 9

The Stable 9 uses of setting tray are intentional and should be left as-is. Updated issue summary.

The default admin uses may need to be resolved though. The point where that resolution gets useful is in contrib with settings tray being used on Drupal 12 sites, so it includes the settings tray styling from default admin even though default admin would not include it anymore.

So settings tray is being removed? If so, we should also remove the settings tray CSS files from default_admin, since we're removing the library definitions.

@mherchel: that is what this issue does exactly, moves the CSS from default_admin to settings_tray so that settings tray can be removed and there is no residues left in default_admin :) Please review :)

ahh! Totally missed that they were moved! 🤦‍♂️

Will review shortly as an apology!

I've been trying to review the MR for a while this afternoon. I decided to test it by loading up a page on main, verifying the presence of the stylesheet links in the HTML, then attempting to do the same after pulling the MR. But it didn't work. After pulling the MR it wouldn't include the links to the stylesheets. Clearing the cache didn't help.

I've been trying to figure out what's wrong with it and to be honest, I have no idea what's the problem. But eventually I decided to convert the procedural hook to OOP and move it into SettingsTrayHooks. After I did that the stylesheet links were included. So maybe it needs to be a method. I'm not sure.

Am I testing this incorrectly?

I also threw a logger line into the procedural function and nothing ever got logged.

Good find, can you contribute that fix? :) Either way it is a good idea even though the modules deprecated for removal are not a focus of hook cleanup :)

Yeah, sure. I can do that now. I just wasn't sure if I was missing something important.

I pushed the conversion.

Anyone have a recommendation for testing? When I install settings_tray and enable default_admin I don't see the contextual icon

Ok this is awkward. I was wondering why @smustgrave finds it hard to test this, but I cannot reproduce settings tray actually working in any combination core setup with or without the MR applied in default_admin.

One major thing is settings tray depends on the old toolbar, so you need to disable navigation and use the old toolbar.

Then you would look at blocks with "Quick edit" functionality on them, which opens the settings tray when you click it:

I can reproduce this with Olivero on the frontend. I cannot reproduce this feature working on the backend with Default Admin.

I also wanted to check if it works in Claro at least, but Claro also does not seem to show any of the contextual menus. While it does put the border around the editable areas same way like Default Admin does.

So this whole experience is VERY BROKEN in core.

I don't think we need to fix it here, we can do a best effort to move the code, so that whoever picks it up they can look to fix it?

@dcam did the feature actually work for you or just the style sheet appearing? :D

There is explicit code for this in Claro and Default Admin:

  /**
   * Implements hook_preprocess_block() for block content.
   *
   * Disables contextual links for all blocks except for layout builder blocks.
   */
  #[Hook('preprocess_block')]
  public function preprocessBlock(array &$variables): void {
    if (isset($variables['title_suffix']['contextual_links']) && !isset($variables['elements']['#contextual_links']['layout_builder_block'])) {
      unset($variables['title_suffix']['contextual_links']);
      unset($variables['elements']['#contextual_links']);
      $variables['attributes']['class'] = array_diff($variables['attributes']['class'], [
        'contextual-region',
      ]);
    }
  }

Commenting this out brings back the contextual functionality in Claro, and with Navigation disabled and Toolbar and Settings Tray enabled then Settings Tray does work.

Traced the origins of this back to #2487025: Remove contextual links in Seven - Claro was initially a copy of Seven and this code was inherited from there. So it looks like Contextual, and in turn Settings Tray, are not meant to work in any admin theme.

Maybe the correct thing to do here is strip out all mentions of Settings Tray from admin themes, and add an explicit check in Settings Tray so it is only enabled on non-admin pages? Perhaps even do the same for Contextual in a followup?

@dcam did the feature actually work for you or just the style sheet appearing?

I'm at work right now, but was testing it on my personal laptop at home. But I definitely remember that the Quick Edit link was rendered in the contextual links. The strange thing is that I also definitely had Navigation enabled. So I don't have an explanation. @smustgrave and I communicated about this via Slack and I didn't have any answers for him either. If it only works with Toolbar then that makes it even more confusing for me.

Actually, I know that I had Navigation enabled. I'd been testing another issue in it. But now I remember that enabling Settings Tray required Toolbar to be enabled. So maybe that's why my experience was different. Because Navigation suppresses the rendering of Toolbar, right? So I had both Navigation and Settings Tray links on the page.

Based on #19 sounds like it should just be removed? If maintainer in contrib wants to make it work in admin themes that can be up to them?

Took a similar stance with shortcut not working in default admin

Updated issue summary based on updated direction to just remove this from Default Admin.

Maybe the correct thing to do here is strip out all mentions of Settings Tray from admin themes, and add an explicit check in Settings Tray so it is only enabled on non-admin pages? Perhaps even do the same for Contextual in a followup?

Pushed the updated MR that implement removing this code :) Settings Tray being a module on path of removal from Drupal core I don't think its a priority to build new features for it like blocking it on admin pages, especially since other admin themes could theoretically work with it as shown above, it is just that the two core admin themes don't. :)

So I think we should focus on cleaning core up and not build new features which may or may not be correct for a module we are removing anyway :)

Fully green of course, ready for your reviews again :)

Seems like good removals now!

LGTM! +1

Committed and pushed b0900357ea5 to main and d8e56ca4170 to 11.x and cbde8d690c4 to 11.4.x. Thanks!