Navigation leverage icon core API

This MR is currently made of 2 parts:

• Commit 22403b64 : Everything inside /core/asset , /core/lib, /core/module/system and /core/tests is a duplicate of #3483209: Navigation leverage icon core API which was taken as a starting point. Of course, this part will be removed when the other issue is merged into 11.x
• Commit 02b0c34: The changes in /core/modules/navigation is the interesting part for this issue

So, in /core/modules/navigation, we did the expected changes:

• a new navigation.icons.yml file with the icon pack
• the move of the icon logic from the SDC component styelsheet (toolbar-button.pcss.css) to the SDC component template (toolbar-button.twig)

In order to simplify this logic, we also renamed some SVG files in the toolbar-button to be able to map automatically the icons ID from the presenter template embedding the SDC component (navigation-menu.html.twig).

Work in progress. Will be in review after #3483209: Navigation leverage icon core API is merged.

Thank you for great start. I see some bem mistakes. Gonna fix it.

I don't think we should define viewbox somewhere

I don't think we should define viewbox somewhere

It depends of the icon pack we are implementing. Th goal of this Icon API is to provide simple tools to faithfully implement the icon packs as documented upstream by the icons provider.

In navigation module, we are both the icons provider and the icon pack definition author. So, we can do anything. If it works OK without the viewBox attribute, and you think it will be better without it, we can remove it.

I think we should keep the original viewBox attribute if it is not reassigned by the template

Here I added a simple illustration of how the absence of the original attribute breaks the adaptability of icons.

https://codepen.io/finnsky/pen/jOgKdWz

For example, in the current task we have a lot of work to do to rewrite all the icons, which could have been avoided if we had kept the original viewBox attribute

It's not that I'm afraid of this work. I just think the approach of deleting your own SVG properties is wrong

Also it will not work in cases when we will need different size of icons on different screen sizes. (Pretty often frontend task)

@mogtofu33 thank you for this and for all that initiative

@mogtofu33 hello!
It is possible to update that MR? Would be cool to have it onboard before stable release

Thank you very much!
This is great step!

We also need to clean up some css tricks which we did for iconless buttons. I'll care about it in next days.

Messages appear in Umami demo profile only for now.
https://www.drupal.org/project/drupal/issues/3441100#comment-15984062

I looked at the MP! It looks great!

Here's what I added;

Since the icons have the same namespace, they can't be in different folders to avoid name collisions (spoiler: there was one already. Chevron in the button and menu item)
I moved all the icons to one folder

We can't use the toolbar-button--icon class for all the icons. Firstly, it doesn't follow the Bem style, and secondly, we need different classes. I added my own classes to the icons right in the config! Great!!!!

I removed the fill attribute from the icon. It's much better to set this from the CSS, especially since we have classes

Added icons to places where they weren't there yet. Different chevrons. Back button.

Corrected the styles and removed some hotfixes from recent tasks

Corrected the ajax animation

Corrected the messages

What needs to be done?

Need to fix tests

And most importantly:

If there is no icon, we should display the first two letters of the link name
https://gyazo.com/ed31b0f04e1d5ef87e278152bf1a98f1

In toolbar-button.twig
{% if icon is empty %} does not work
{% if icon|render is empty %} works

To check, add any link to the top level of the menu

Hi guys,

Things are taking shape, that's great.

In toolbar-button.twig
{% if icon is empty %} does not work
{% if icon|render is empty %} works

Early rendering must be avoided in a template, so no |render() filter here (maybe we need to deprecate this filter one day).

Let's not merge until {% if icon %} is enough. Is there something we need to change in the Icon API side?

yeah, seems so we need to add checking if icon in existing icons list. this should be condition.

Is there something we need to change in the Icon API side?

Seems no. Looking good! Thank you!

I have idea :)

I used the power of CSS

We couldn't do this before because we didn't have an element in DOM. But now we know that if there is no icon we will add an attribute. I think it's great

All that's left is to fix the tests. Yaay

Only one test failure:

2x smaller StylesheetBytes!!!

All tests green!

Checked the MR and progress here is amazing. I would really love to have it merged soon.

Just mentioning a few things to take into account:

• Tested locally, and it seems that this MR also solves #3505124: The icon for the more actions button is not visible. That issue might need to be updated
• Looks like some of the changes requested in #3505291: Undo premature changes in the Navigation module that were made according to a non-existent (yet) design are being applied here. The scope of this issue and that one might be updated
• #3445993: Provide a NavigationLinkBlock Plugin and use Help as an usage example Introduced a new navigation block that has to be integrated with the Icon API as well. I would open a follow up for it since the benefits of this one are more relevant than those adjustments and might need some unrelated architectural discussions

Code looks good to me and I can't appreciate any visual regression while testing manually, but I would love a second opinion from a more FE specialized reviewer before marking it as RTBC.

Created #3507057: Integrate the new Navigation Icon API with NavigationLinkBlock as follow-up for the NavigationLinkBlock internals

@mogtofu33, @pdureau

I have question, maybe you have idea?

How technically modules which extend Navigation may set their own icons with current approach?

i'm thinking about
modulename.icons.yaml
and
{{ icon(‘modulename’, icon, { class: ‘toolbar-button__icon’, size: 20 }) }}

It can be SDC option:
icons_lib:
default: navigation

Modules like: (before they had CSS option for this)

https://www.drupal.org/project/navigation_extra_tools

CSS option for extending still exists. But maybe we can make it ideal ;)

I'm reading the issue backscroll and the Icon API docs, since I was not very familiar with it and led me to a couple of questions about how Icon API would integrate with Navigation in a generic way.

One of my main concerns from a long time ago is about how to map the icons with navigation items. Current approach is based on some magic CSS classes that could be implemented by Navigation module for the default core values or by any other module or theme to override those values or give support for new navigation items.

This icon based approach seems to be based in the match between the menu items and the icons provided by Navigation module. The Icon pack is hardcoded in the twig templates, so will not be easy for other modules to include their icons in Navigation. The CSS magic class approach is still there as a fallback, but would be great to figure out an Icon API based path for other modules and custom menus.

I could think of using hook_icon_pack_alter() to provide extra icon sources for the navigation pack, but not sure if that would work in a generic way.

Icon UI Menu module looks very promising but we cannot rely on that here unless that module is included in core, or at least part of the functionality, and I think we need to figure out an alternative with the current tools we have now.

I don't have a clear answer for this, but maybe Icon API maintainers could come with a proposal that would help to have all the benefits of the Icon API while giving some extra flexibility to the whole Navigation Icon ecosystem.

Well then let's leave it as is for now.

This MR already solves many problems on the frontend.
In the modules that use Navigation nothing will break and it will simply display the first two letters and the Icon can be added in CSS.
Which is quite flexible.

What is really needed here is good testing.

Really great work here!

I've done a first pass over the backscroll, static review and visual regression review. As well as familiarizing myself with the Icon API docs.

I've left a couple of comments on the MR. In addition, I found a couple of visual regressions:

1. The chevron has a smaller stroke width here than on 11.x.

2. The Top Bar edit button is regressed as well. It has the wrong icon now and the wrong font-weight.

I still have a lot of testing to do, including but not limited to:

• Windows high contrast (should be better here from static review - exciting)
• RTL testing
• Accessibility testing
• Review scenarios where toolbar-message.pcss.css gets used
• Review scenarios where navigation--message.html.twig gets used
• More visual regression testing

Thank you for starting review it.

1. The chevron has a smaller stroke width here than on 11.x.

Fixed

2. The Top Bar edit button is regressed as well. It has the wrong icon now and the wrong font-weight.

They are actually structural fixes. Regressions were added previously.
Few of them out of scope of that ticket. But they should be done anyway.
https://www.drupal.org/project/drupal/issues/3505291

They are actually structural fixes. Regressions were added previously.
Few of them out of scope of that ticket. But they should be done anyway.
https://www.drupal.org/project/drupal/issues/3505291

I'll follow up with @ckrina on the Top Bar designs and their status. In the meantime, I do agree that the changes are out of scope for this issue. We should focus on the Icon API here, and not see any visual regressions from the state of 11.x. Just to ease the review for other folks coming in.

We should focus on the Icon API here, and not see any visual regressions from the state of 11.x

We should not compare the appearance with version 11. But with the design that does not exist.

In fact, only the color and font thickness of the button are beyond the scope of this task. But this button is not in the design. What is currently in version 11 is something random. In addition, it was added incorrectly.

So I don't see anything critical if it is here.

Further testing:

Windows high contrast (should be better here from static review - exciting)

PASSED

Looking great! Can confirm that we're seeing some fixes, namely with the icons, for the windows HC issues noted in #3438477: High contrast mode needs some more refinement. Will need to update that issue once this lands. Here are some screenshots of various testing scenarios:

RTL testing

PASSED

Looking good here! Here are some screenshots of various test scenarios:

Accessibility testing

PASSED

Tested with VoiceOver on Mac. Things are looking great here as far as I can tell. Can confirm that #3475979: Remove prefixed abbreviations from top-level menu items is resolved here, which is very nice.

Review scenarios where toolbar-message.pcss.css gets used
Review scenarios where navigation--message.html.twig gets used

PASSED

This is the Demo Umami message. To test need to install the demo_umami profile, enable navigation and look at an admin page. The message appears in the navigation correctly and fixes the bug in Chrome where the icon is not visible. Here are some screenshots showing all good:

Unrelated, but I did notice some slight alignment and spacing issues with the Demo Umami message. I'll file a follow up issue for this if we don't already have one.

Otherwise, I want to look through the code again, and consider @plopesc's comment.

This is looking great though.

We should not compare the appearance with version 11. But with the design that does not exist.

In fact, only the color and font thickness of the button are beyond the scope of this task. But this button is not in the design. What is currently in version 11 is something random. In addition, it was added incorrectly.

Checked with @ckrina. The designs for the top bar will still see further refinement, that will be followed by a design implementation issue at some point. It would still be ideal not to be seeing visual regressions from 11.x here, but I'm OK with it if others are, not willing to hold us up on that.

I've sketched out a POC for a mapping from menu item id to icon (icon_pack_id and icon_id). I've put that up as a separate branch and filed a MR against the 3483209-navigation-use-icon-api branch so as not to clutter that branch for now:

https://git.drupalcode.org/issue/drupal-3483209/-/merge_requests/1/diffs

The main bit is a new service, \Drupal\navigation\Menu\MenuLinkIconMap that has a static map from menu link id to an (icon_pack_id, icon_id) pair. There is also a new alter hook for this, hook_navigation_menu_link_icon_map_alter. That mapping gets used to avoid hardcoding the icon_pack_id and icon_id's in the templates. It also has the nice side effect of naming the icon SVG files in the way they were intended from the phosphor icon library.

This is meant as a quick way to setup a mapping that would allow others to alter it relatively easily. In future, it could be managed in config, with a UI, or glue it together with the existing ui_icons module.

Curious to hear what @mogtofu33 and/or @pdureau think about the POC, or any others.

Thank you for jumping into this @m4olivei!

Checked the MR and in general it adds architectural improvements to the initial approach, since icons are now more dynamic, could be overridden and allows other 3rd party modules to define new icons to be used. AFAIK we have navigation_extra_tools & dashboard, but could be more in the future.

On the other hand, the system is not very intuitive for non-experienced users. And I ma not sure how this would work with menu items created through the UI. We might need to implement a contrib module on top of ui_icons to handle it.

My other concern would be related to the fact that this new API might need to have a more or less defined plan from now to the date where all the pieces could come together and have a proper mapping between the menu items and the icons. That might require some help from a Framework Manager with a better understanding of the whole landscape.

Thank you again, this is a great step to open that conversation.

My other concern would be related to the fact that this new API might need to have a more or less defined plan from now to the date where all the pieces could come together and have a proper mapping between the menu items and the icons. That might require some help from a Framework Manager with a better understanding of the whole landscape.

The goal and the scope of this API was to be "the SDC of icons", providing lower level tools for developers:

• a discovery mechanism for icon packs
• an extractor plugin type to collect the icons from each pack
• a render element and a twig function to use those icons in code

Because we believed those pieces were enough:

• for Drupal Core, to solve the current issues with icon management (as we can see in the current issue)
• for Drupal Contrib, to consolidate the implementation of the numerous modules managing icon packs and icons, and make them more compatible with each others (this effort will be visible after Drupal 10 end of support I guess)

This API is not currently providing:

• Form Element(s): but it may be a good idea to add one or two in a few months, picked among the best from the emerging ecosystem of contrib modules
• Higher level integration with Drupal config (menu, field storage, ckedtor5, text formats...) because we are expecting the emerging ecosystem of contrib modules to be more creative and dynamic with those tasks
• Other interesting but currently out-of-scope mechanisms, like allowing a icon from a icon pack to replace the icon from another icon pack (so a theme can replace Navigation icons for example)

We have an exciting future ahead

I want to thank everyone again for their enthusiasm!

But also to clarify that this MR already makes many useful improvements!

Let's do everything to get it merged as soon as possible and continue discussing the icon configuration in a separate issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/11172
For example, there are several comments here that we should address here, or do later?

This video explains a lot ;)

I'll check it

Thanks for all the feedback!

Let’s look at general comments, then look at POC comments:

General comments

From @pdureau in #45

This API is not currently providing: [...] Higher level integration with Drupal config (menu, field storage, ckedtor5, text formats...) because we are expecting the emerging ecosystem of contrib modules to be more creative and dynamic with those tasks

This is what we're running into here, of course. Navigation needs icons against each menu item displayed. The POC I put together is meant to address this in a light-touch way, but perhaps we can defer the POC approach or one like it to a follow-up issue.

From @mogtofu33 in #46

The core icon API do not provide (yet) a switch/override/weight system for the discovery, because it's based on the core YamlDiscovery there is an override based on provider name and type in this order: module by name, profiles by name, themes by name.

Neat! This is encouraging. At least there would be some way to override this prior to doing some sort of mapping like is done in the POC.

POC comments

From @plopesc in #44

On the other hand, the system is not very intuitive for non-experienced users. And I ma not sure how this would work with menu items created through the UI. We might need to implement a contrib module on top of ui_icons to handle it.

Agree it's not very intuitive; the POC as is definitely has developers as the target audience rather than a site admin. It would look like this:

/**
 * Implements hook_navigation_menu_link_icon_map_alter().
 */
function mymodule_navigation_menu_link_icon_map_alter(array &$map) {
  $map['menu_link_content:6db5002e-413e-482f-b971-a29b6c689f10'] = [
    'icon_pack_id' => 'navigation',
    'icon_id' => 'eye',
  ];
}

Getting that menu link id is non-trivial. It's kind of exposed in the markup as the id on the <li> element. Otherwise you'd have to either do some xdebug'ing, or look at the DB and know how to compose it together 💀.

From @plopesc in #44

My other concern would be related to the fact that this new API might need to have a more or less defined plan from now to the date where all the pieces could come together and have a proper mapping between the menu items and the icons. That might require some help from a Framework Manager with a better understanding of the whole landscape.

Yep, I hear this. This speaks follow up issue to me.

From @mogtofu33 in #46

Your work on the NavigationMenuLinkTree suggest I should work on a core issue to allow MenuLinkTree to handle icon ids.

+1 from me. I'd be very interested to see if core committers would go for something like that. Formalizing where that data gets stored would probably help ui_icons module. Especially considering the challenge of storing the data for all menu items (both YML discovered, eg. *.links.menu.yml and menu_link_content entities), see #3482151: Menu: support non content menu links.

I agree with @plopesc on the not very intuitive, perhaps set the pack_id as a setting and the mapping too instead of hook would be better. You can have the pack list option from IconPackManager::listIconPackOptions().

Yep. More evidence that we probably need a follow up to fully flesh it out.

Next steps

I'm convinced the mapping should be a follow up issue to address.

To @finnsky's point in #47 there is some good stuff here. I'm going to try and address the two issues that I pointed out in the MR for this issue.

I rolled back the use of icons to the clean class.
Now everything works. And that problem from the video is no longer repeated.

Let's test!

To @finnsky's point in #47 there is some good stuff here. I'm going to try and address the two issues that I pointed out in the MR for this issue.

@finnsky's too fast for me! Will test the latest. Looks promising from a quick glance at the code changes.

Found two small issues in testing:

First, it looks like we need to rename the media.svg asset. If you install demo_umami, which has a Media link in the Content menu, you can see the icon is missing:

Second, in RTL the expand/collapse button chevron icon on the navigation does not rotate to respond to the state, eg.

https://www.drupal.org/files/issues/2025-03-03/rtl_expand_collapse_btn_d...

Thank you for review. Gonna check now.

One other thing for us all to be aware of here, is that Gin will need to update its style overrides for the navigation icons after this lands. Thats not an issue, that I'm aware of, given the experimental status. Just FYI.

Gin on 11.x

Gin on this issues MR

Noticed that the issue description needed some love. Updated.

RTL issue and media icon issue that I noted in #52 are fixed. I've otherwise retested in RTL, Windows High Contrast, and VoiceOver. All are looking good to me!

I'm going to go ahead and say this probably could use a change record. Even though Navigation is experimental, it would probably be beneficial for community folks like Gin (see #55) to be aware of this change and have a simple note to follow to update.

small merge conflict. Other than that it looks good to me, great progress and thanks for the extensive testing! Feel free to RTBC, happy to commit this :)

Not sure about the change record. I'm guessing there are folks working on gin here no? we might add a note to the existing icon api change record but I'm not sure what would be in the change record? Also can't commit if we don't have the change record. so in the spirit of not making our life harder, I'll remove the tag.

Thank you all for participating!

Committed dd95187 and pushed to 11.x. Thanks!

Not sure about the change record. I'm guessing there are folks working on gin here no?

I don't think there is anyone here on this issue working on Gin. FWIW, now that this has landed, I've put up an MR for Gin that shows the changes required to have Gin continue to override navigation icons to customize: #3510801: Update Core Navigation Icon Overrides.

we might add a note to the existing icon api change record but I'm not sure what would be in the change record?

I think what would be valuable is to document the YAML discovery override technique that I'm using in the Gin MR in #3510801: Update Core Navigation Icon Overrides. @mogtofu33 suggested this in #46:

This is not intuitive but just how YamlDiscovery works to lookup files. I will try to add this to the current icon api documentation.

I'll take a crack at this.

I've updated the icon pack API change record: #3490350: New API for icon management. Interested to hear from @mogtofu33 on that. Overriding an icon pack is possible, but it's not the best DX. That's not anything to do with this issue, but perhaps might inspire further improvements.