Navigation icons contain hardcoded colors

I've updated the above icons so that they use currentColor for the fill attribute.

In all cases above the fill color was #000000, or black, which seemed like it would play poorly with dark mode.

The only icon I didn't modify was eye.svg since there is another issue open specifically to address that.

See: https://www.drupal.org/project/drupal/issues/3623811#comment-16779480

Making a note here that I intend to upload some screenshots in a moment demonstrating the difference between the old and new icons so the color differences are apparent.

Here are the promised screenshots showing the before and after of the icons in the navigation sidebar.

There is no noticeable difference between the before and after because, as I've discovered, the navigation icons render using the .toolbar-button__icon CSS class, which has has a fill: currentColor rule. This takes care of toggling between the colors in light mode and dark mode even if the SVG doesn't use currentColor. In this case, my MR just focuses on the SVG file value, which has the added effect of making these SVGs more portable if that class name is omitted (as in the case of a custom implementation).

Currently, a user implementing a core icon for a custom purpose has to knowingly call the twig component pattern for icons and also pass a specific settings array into that builder in order to get an icon that respects currentColor, which means that they also have to inherit and override parts of the navigation CSS in order to get access to custom styling.

Here is a sample implementation from my own testing

{{ include('navigation:toolbar-button', {
            attributes: item_link_attributes.setAttribute('href', '/').setAttribute('data-drupal-tooltip', 'test').setAttribute('data-drupal-tooltip-class', 'admin-toolbar__tooltip'),
            icon: {
             'pack_id': 'navigation',
             'icon_id': 'database',
            'settings': {
              'class': 'toolbar-button__icon', # This class contains a fill: currentColor rule that is otherwise missing in the SVG file
              'size': 20
            }
            },
            html_tag: 'a',
            text: 'database',
            modifiers: [
              'collapsible',
              item_link_tag == 'span' ? 'non-interactive' : null,
            ]|filter(v => v is not null),
          }, with_context: false) }}

As for the "radioactive" icon in the test suite, I want to note that the icon appears to be a clone of the "error" icon but with a different name.

Furthermore, it is only used in one context, NavigationTestHoohs.php, specifically for the purpose of discovering newly added icons to an icon pack. In this context, the proper toolbar-button__icon class is never added to the render hook, so the icon does not get rendered with the CSS-implied currentColor rule, and therefore is never tested in a visual capacity. It follows that the fact that I altered the SVG to include the currentColor rule should have no impact on tests.

My defense for retaining the change to his very niche corner case is in terms of improving navigation's best practices for the frontend. SVGs should carry the burden of adapting to their context by default, and CSS should only be used when that default is not sufficient.

// In core/modules/navigation/tests/navigation_test/src/Hook/NavigationTestHooks.php
/**
   * Implements hook_menu_links_discovered_alter().
   */
  #[Hook('menu_links_discovered_alter')]
  public function menuLinksDiscoveredAlter(array &$links): void {
    if (\Drupal::keyValue('navigation_test')->get('menu_links_discovered_alter')) {
      $links['navigation_test.navigation__no_icon']['options']['icon'] = [
        'icon_id' => 'radioactive',
        'pack_id' => 'navigation_test',
      ];
      $links['navigation_test.navigation__default_item']['options']['icon'] = [
        'icon_id' => 'foo',
        'pack_id' => 'bar',
      ];
    }
  }

Adapted the issue summary to the standard template.

Also updated the summary somewhat to reflect my discoveries in #7.