🇨🇦Canada @m4olivei

Discussed in Slack as well. Conversation there couldn't recall a concrete reason for making this issue blocking.

Removing the Navigation stable blocker label.

m4olivei → changed the visibility of the branch 3374901-css-load-order to hidden.

m4olivei → changed the visibility of the branch 3374901-css-load-order to active.

Adding the following recent stable blockers:

• 📌 Local task name expectation in getFeaturedPageActions is fragile Active
• 📌 Show the Navigation Top Bar in 11.1.x and 10.4.x Active

Downgrading priority of ✨ Fix safe triangle bottom position. Active to 'Could Have'.

Added the following under Top Bar:

• 🐛 navigation top bar edit button has strange active (?) colors Active
• 🐛 Navigation top bar overlaps with Olivero menu Active

Replaced 🐛 Styling of front theme affects styling of navigation buttons Active with 🐛 Front-end theme styles can bleed into Navigation Active

Added the following to 'Should have':

• 🐛 High contrast mode needs some more refinement Active
• 📌 Add a placeholder preview for the navigation toolbar Active
• 📌 [PP-1] Integrate the new Navigation Icon API with NavigationLinkBlock Postponed
• 🐛 Create content link missing after creating content type Active

Added 📌 Local task name expectation in getFeaturedPageActions is fragile Active as 'Must have'.

Added 📌 Show the Navigation Top Bar in 11.1.x and 10.4.x Active as 'Must have'.

Added a couple of bugs to 'Nice to have':

🐛 navigation top bar edit button has strange active (?) colors Active
🐛 Navigation top bar overlaps with Olivero menu Active

This was added to 📌 New Toolbar Roadmap: Path to Beta & Stable Active as 'Should have'. Updating here to reflect that it is now a Navigation stable blocker.

Closing this one as a duplicate of 🐛 Front-end theme styles can bleed into Navigation Active . We've seen this recently on a handful of sites. There is some discussion in the issue about how to mitigate. Likely first step will be to mimic the way off-canvas dialog protects against style bleedthrough by shipping CSS with higher specificity in the navigation module.

Thanks all!

I can reproduce on the latest 11.x when I install Drupal's standard profile and enable Navigation module.

Updating issue description.

This issue is still valid. We continue relying on the magic class pattern approach here. In my mind, ideally, we would allow the user to configure a icon_pack_id and icon_id pair for the block plugin that would get used in rendering it. That's an awkward UX experience, given core doesn't have any icon widgets, but, it could be left to contrib to make it better, and/or if/when core adopts a widget for icons.

The referenced issues here are now fixed, eg.

📌 Define the 3 areas the Top Bar will provide Active
📌 [PP1] Show entity information on the Top Bar Postponed

We also had a follow up issue to bake in the moderation state if available:

✨ Display content moderation state in the Navigation Top Bar Active

I believe that has resolved the point at issue that inspired this issue. As @berdir pointed out there is some baked in ability in the plugin system to alter / override things.

I'm going to Close this issue for now. If further situations come up that further illustrate the need for some better DX, we can revisit.

Discussed with @plopesc and @ckrina today. The Top Bar is a Stable blocker. This issue will be needed to mark Navigation as stable. Adding Navigation stable blocker tag.

Adjusted the summary to include "Top Bar" for easy reference.

Changed Category to Bug report.

Added steps to reproduce.

Top Bar has moved along quite a bit since this issue was raised. We are past Top Bar POC stage. This issue is no longer relevant. Closing as outdated.

I also prepped an alternative MR to see how cherry-picking commits from the bug fixes would help. It does, although 🐛 NavigationRenderer manually adds required cache tags Active required fixing a conflict.

I think I prefer back-porting these. In which case, those relevant issues should probably be re-opened to backport. Interested in what others think.

I added a MR to address the cacheability issues. The problem is that the cacheability metadata collected by from API before it calls the #pre_render is being thrown away and overwritten. When workspaces module is enabled, this blows up due to a mismatch with expected cache contexts.

As I alluded to in #8 we could alternatively address this by backporting some issues that were committed to 11.x to 11.1.x. I'm not sure what the feasibility is there. Two of the three issues were bugs. We could probably backport the two bugs without the Integrate Workspaces feature:

• 🐛 NavigationRenderer manually adds required cache tags Active
• 📌 Use a placeholder for the navigation toolbar Active

On 11.x this isn't an issue b/c it's not a #pre_render anymore, it's a #lazy_builder callback.

@robwj you're close to the relevant code by not quite. It looks like there are issue with the cacheability metadata added in \Drupal\navigation\NavigationRenderer::doBuildNavigation.

Relevant issues that have collectively addressed this in 11.x are:

• ✨ Integrate with Workspaces Active
• 🐛 NavigationRenderer manually adds required cache tags Active

Apparently those were not backported to 11.1.x. I'll need to check in with some folks if we want to do that. In the meantime, I'll try and put together an MR to fix this.

I can reproduce the issue. As noted in #3 and in the issue description, it's not reproducible on 11.x. It is reproducible on the latest 11.1.x.

Will investigate further.

Merged the latest from the 11.x branch. Back to Needs review.

We saw this in the wild on the Mukurtu distribution. It's an open source Drupal install profile (which you can set up quite easily!):

m4olivei → created an issue.

Good find. Tagging as Needs test, it's not obvious how to reproduce it. I was able to reproduce it in a very hacky way by adding the following implementation of label() to \Drupal\node\Entity\Node, just to illustrate the issue:

/**
   * {@inheritdoc}
   */
  public function label() {
    return NULL;
  }

IMO, the plugin should be skipped if there isn't a useful title or badge. We skip the badge, avoiding this issue if there isn't one. We should do the same for the label.

Want to note that this patch should get backported to 10.5.x once 📌 Define the 3 areas the Top Bar will provide Active is backported.

Could we flesh out some steps to reproduce in the issue summary to illustrate the problem that we're trying to address?

The related issue here was reverted and re-opened. Marking this one as a duplicate.

Thanks for filing an issue @m4olivei, I was wondering if that was the correct approach, rather than re-opening this one.

I wondered that as well. Since this one has been re-opened, I'll close the one I opened. Feel free to pull from the MR I opened over there if its useful.

m4olivei → created an issue.

Filed as a bug: 🐛 Twig development mode breaks pages with SDC Active

m4olivei → created an issue.

Hey all 👋. I started noticing a PHP WSOD today and after some xdebug'ing, traced it back here:

TypeError: Drupal\Core\Plugin\DefaultPluginManager::doGetDefinition(): Argument #1 ($definitions) must be of type array, null given, called in /var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php on line 25 in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 43 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

To reproduce:

• Install Drupal standard profile
• Login and enable Twig development mode
• Visit a page that uses an SDC, eg. front page of the site, or any Olivero page. You may need to rebuild cache if you had already visited this page before turning on Twig development mode
• 💥 Above error

Reviewing the MR, the overridden getDefinitions method does not call the parent method when Twig debugging is on. What appears to be missing in relation to the parent is a call to $this->setCachedDefinitions, which in turn sets $this->definitions, which is depended in in the place where the error occurs.

Hopefully that's helpful.

Since the merge conflict was simple and tests are green, I'm going to go ahead and set this back to RTBC. Feel free to nudge back if necessary.

Fixed the merge conflict. Very straightforward. There was a change made and then removed to core/modules/navigation/js/user-block.js on 11.x. Here we just don't need that script anymore. Fixed the conflict by deleting it.

Good point @smustgrave. Updated the issue description. Back to Needs Review.

This was a tiny bit tricky b/c we need PostCSS to not mess up the form that is required to provide the empty alt text. Should be OK now. Needs browser testing, but Can I Use shows 91%: https://caniuse.com/?search=content%20alt%20text

Added a few more issues that I noticed.

We recently landed 📌 Navigation leverage icon core API Needs work , which had a positive impact on Windows High Contrast mode. I'll be doing some testing on the latest 11.x with the goal of updating the issue description to reflect the outstanding issues.

This issue was fixed by 📌 Navigation leverage icon core API Needs work 🙌. Can no longer reproduce on the latest 11.x. Closing as such.

I've updated the icon pack API change record: [#3490350]. 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.

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: 📌 [PP-1] Update Core Navigation Icon Overrides Postponed .

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 📌 [PP-1] Update Core Navigation Icon Overrides Postponed . @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.

Now that 📌 Navigation leverage icon core API Needs work landed, this will be needed to override navigation icons as of 11.2.

I've added an MR to show what's needed. At a high level:

• Override the navigation icon pack (gin.icons.yml)
• For all the icons that navigation provides, we need icons of the same name provided by Gin. This was the bulk of the adjustments necessary

m4olivei → created an issue. See original summary → .

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.

Noticed that the issue description needed some love. Updated.

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

Yay! RTBC +1. Thanks @ckrina.

The issue that we're postponed on, 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active has been resolved. Marking this as needs work. Will merge the latest 11.x into the branch and see where we're at.

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... →

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.

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<code> module. Especially considering the challenge of storing the data for all menu items (both YML discovered, eg. <code>*.links.menu.yml and menu_link_content entities), see ✨ Menu: support non content menu links Active .

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.

This looks good to me!

My only hesitation would be that there isn't any design/product input here. I don't see any consideration for workflow / content moderation in the Figma designs I have seen for experience builder or otherwise. I'm not sure how much that matters. I would guess maybe some of that happened offline given @lauriii filed the issue.

Fleshing out the issue summary to reflec the current state.

m4olivei → made their first commit to this issue’s fork.

Addressed some duplication in the navigation titles for non-sighted users. See last comment and commit.

Ready for a re-review and accessibility review.

Ready to go!

@plopesc and/or @finnsky could you review the MR, and my last comment?🤞 RTBC.

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.

Thanks @katannshaw!

OK, so now that we've had some solid accessibility testing done here. I think we're ready to move forward. As @plopesc noted in #5, what we've done here is first expose the Nightwatch axe rule errors by turning the Navigation Top Bar always on in the 3508199-navigation-top-bar-axe-test-only branch. The Nightwatch axe rule errors are here, or:

TEST FAILURE (2m 53s):  
   - 2 assertions failed; 1433 passed
   ✖ 1) Tests/a11yTest
   – Accessibility - Navigation Module - Claro page (2.587s)
   → ✖ NightwatchAssertError
   aXe rule: button-name - Buttons must have discernible text
	In element: .toolbar-button--icon--dots
   → ✖ NightwatchAssertError
   aXe rule: region - All page content should be contained by landmarks
	In element: .top-bar__context

Note that there are other test failures in there as well. This issue is solely focused on the Nightwatch axe rules error.

We've then fixed the Nightwatch axe rule errors in the 3508199-navigation-top-bar-axe branch. We can see that the Nightwatch axe rule errors are fixed here!

The next step will be to take out the code that is exposing the navigation Top Bar here, b/c that is the work of 📌 Show the Navigation Top Bar in 11.1.x and 10.4.x Active . Again, we're only interested in fixing the accessibility issue with the Navigation Top Bar here. I will next push this commit, and the 3508199-navigation-top-bar-axe branch, will be ready to merge!

@catch Now that the 2 issues that this was postponed on are resolved, I'm wondering if we feel like we still need this one?

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.

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 🐛 High contrast mode needs some more refinement Active . 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 🐛 Remove prefixed abbreviations from top-level menu items Active 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.

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 🐛 Undo some changes Active

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.

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

While not ideal, you can work around the issue by using the Pencil icon on any block to bring up the contextual links, then click Move. This pulls up a UI on the right hand side of your screen to allow you to drag and drop the blocks.

Of course, that's a pretty rough experience and should be fixed, but in case anyone is finding this and looking for a workaround.

I've re-tested the issue. While its still reproducible on the latest 11.x, it does seem less pronounced at least.

After discussing in Slack, we've decided to remove the "Navigation stable blocker" tag. As its edge case, and doesn't prevent the user from using the module, it does not need to block Navigation becoming stable.

The code looks great, some nice simplifications while addressing the bug.

I've tested with and without big_pipe. The expand/collapse logic works in both scenarios. I've also tested all of the triggers for expand/collapse, which include the hamburger, close icon, and overlay on mobile as well as the bottom left arrow on desktop. All of those scenarios work as designed.

RTBC for me. Nice work all!

Rebased. Also noted a test failure in NavigationShortcutsBlockTest that was checking cache tags and needed to be updated. I've fixed that, and now tests are passing. Should be good here!

1. I prefer not to duplicate class functionality. We already have [class*="toolbar-button--icon"]
We should use it. Just add a check for the text attribute.

Good point! I've got rid of the extra modifier and class, and instead used a [class*="toolbar-button--icon"]:not([data-icon-text])::before selector. Is that what you meant, or did you have something else in mind?

2. In #6 I pointed out that the close button on mobile is also broken.

Sorry I missed this. I didn't know what you meant, b/c I couldn't see the icon, because it was broken 😆. The above selector catches the Close icon and fixes it as well.

I believe I've addressed all the open threads, which were all minor cleanup / code style adjustments. Ready for review.

And in general, what is top_bar? What is its role? Header or navigation?

I would argue it's not navigation, there are informational bits within it as well. I want to suggest we mark it up as an <aside> with a title for screen readers. This follows what has been done with the sidebar.

Will push an update to show what I mean.

I found that the issue appears to be Chrome's inability to interpret content: attr(data-icon-text); when the data-icon-text attribute is not set. We don't have any text label for this particular button, so I'm proposing that we add a new modifier, icon-only to the toolbar-button component, so we can style it explicitly as content: ''. That will make the ::before pseudo element appear as expected and fix the issue reported here.

This is a quick fix that we should be able to get in quickly before the icon management lands.

Updated issue summary. Marking as active. Will work on an MR.

I did a static review and adjusted some tiny things. The main change was to remove the reference to navigation_top_bar in phpunit.xml.dist, which I believe was causing the test failures. We'll see.

m4olivei → made their first commit to this issue’s fork.

Adding a couple of tiny adjustments to fix a docblock comment, align with other instances of specifying a #lazy_builder callback and a small cleanup now we're getting rid of user-block.js.

Otherwise everything looks good, and works well. I've tested with and without big_pipe. I've also tested the fallback by manually removing all the links from navigation-user-links menu and manually deleting that menu.

Everything looks good to me here.

Test adjustment is great, see my MR comment for more details.

I just re-tested the latest on MR commit on the latest 11.x. Functionally it continues to work as expected.

RTBC for me. Great work @plopesc!

Thanks @plopesc.

We might need to decide as part of 5.x how the integrations with Navigation and Toolbar will be handled, since both would be hook based.
A new option for the toolbar_integration could be added to include the navigation plugin, or we could go for the submodule based approach, one for each of the navigation/toolbar approaches. I don't have a strong opinion here, though.

I'm a fan of adding a new option for the toolbar_integration config. Already an established pattern.

While testing this, found an issue that might need to be considered in this MR.

Nice find! I've pushed a commit to address this. In practice it seems to work fine, although I'm a bit worried it's missing the cacheability metadata for the layout builder config. Again though, it doesn't seem to matter in testing. It would be a bit of a pain to surface that up to environment_indicator_page_top, but not imposiible.

Regarding the related issue in Drupal core that would offer a better integration (avoid the need to configure the block for the Navigation sidebar): ✨ Allow modules to hook into top of content/footer sections of new core navigation Active . Providing some more detail to @plopesc comment in #47.

As @plopesc noted, it's now available in Drupal core >= 11.1.2 ( 11.1.2 release notes → ). It was also backported to 10.5.x, and will be available on 10.5.0 (not yet released). 10.5.0 is set to be released on June 16, 2025 (per Drupal core release schedule → ).

We'll want the current block-based approach for our 10.3.x, 10.4.x and 11.0.x users.

For >= 10.5.0 or >= 11.1.2, users would be better served by using the newly introduced hooks in ✨ Allow modules to hook into top of content/footer sections of new core navigation Active . For this, we should have a new branch of the module with these version constraints, an implementation of hook_navigation_content_top, as well as an upgrade path to remove any environment indicator block from the navigation to avoid a double rendering. I can work on this in a new MR tomorrow.

In summary, I'm proposing we:

• After review, merge MR 53 to the module as is, tag a new release under 4.x
• Create a new 5.x branch
• Prepare an additional MR here for 5.x that has an implementation of hook_navigation_content_top instead of the block plugin as well as upgrade path
• The 5.x MR should adjust core_version_requirement to be ^10.5 || ^11.1.2
• After review, merge the additional MR to 5.x and tag a new 5.0 release

Open to feedback!

Fixed the issue noted with big_pipe by following an approach inspired by olivero, wherein we set a style attribute on the navigation template using the attributes variable. In this way the necessary foreground and background CSS variables are carried with the navigation markup itself, rather than living apart from it, globally in the <head>.

Reviewing this issue as it stands today.

From testing the latest. I found an issue with the styles when you first add the Environment Indicator block to the Navigation sidebar.

Steps to Reproduce

1. Install standard profile on latest Drupal 11.x
2. Enable navigation and environment_indicator module
3. Login as root user
4. Add the Environment Indicator block at /admin/config/user-interface/navigation-block
5. Visit any other page on the site

Expected result

Environment indicator should display and be styled as configured (background and foreground).

Actual result

Environment indicator shows up, but is not styled with the correct foreground and background. Only once you refresh the page, will the foreground and background appear correctly.

I think what's going on here has something to do with Big Pipe. With Big Pipe disabled, we don't have the same issue. My guess is the issue has to do with adding the CSS variables in a preprocess hook, as an html_head #attached property.

Thanks @trackleft2.

What version of Drupal are you using in that test? I'm not seeing that on latest 11.x dev branch or 10.4 🤔.

Added a functional javascript test.

Also, I realize the issue description doesn't accurately describe the latest developments on this issue. Here is a crack at revising it.

This is ready for a re-revew. Summary of changes:

• Merged latest 11.x
• Fixed the issues noted by @finnsky around keyboard search. We now update data-index-text to match the dynamic usename
• Changed the default text of "User" to "My Account" to better fit other places this kind of thing is needed. Updated test to match
• Updated \Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest. We're doing less isValid checks? I think this is a good thing.

I also considered @catch's comment from earlier. For the reasons he mentions, but also as a way to address the small flicker @plopesc noted.

the js looks fine to me (which doesn't mean much because I am bad at js), although I wonder if there's a way to do it using core's existing lazy builder/big pipe workflow without additional js, but... I've never tried to do that with a menu link. And also we could look at that in a follow-up since it wouldn't be a data model or API change at all, whereas the new menu itself feels very stable blocking.

The trouble is that, as far as I can tell, the title of a menu link needs to be a plain string. I tried to create a new Menu plugin (\Drupal\navigation\Plugin\Menu\MyAccountMenuLink) for use as the top level default menu link. My idea was to override \Drupal\Core\Menu\MenuLinkInterface::getTitle and return a render array like:

public function getTitle() {
  return [
    '#lazy_builder' => [
      'user.toolbar_link_builder:renderDisplayName',
      [],
    ],
    '#create_placeholder' => TRUE,
    '#lazy_builder_preview' => [
      // Add a line of whitespace to the placeholder to ensure the icon is
      // positioned in the same place it will be when the lazy loaded content
      // appears.
      '#markup' => ' ',
    ],
  ];
}

This borrows from this approach in user module. The nice thing about this if it worked, would be that BigPipe would take care of intelligently replacing the username without our own custom Javascript, and handling the no JS case well too.

However, it white screens the site. Per the docs on the interface, that method should return a string.

Ultimately I don' t think the flicker is that bad, so at the most, I'd suggest we handle that in a follow up.