🇪🇸Spain @plopesc

Code looks good in general. Default config definition would be needed, though.

Also added a minor suggestion that might be out of scope.

Are tests also considered for this new feature?

Created new MR against 3.x

Update branch

Code looks good, tests are green and the UX of autocomplete widgets is much better now.

Screenshot of the Blog Post form:

I think it covers the agreed objectives for this initial issue. I would mark it as RTBC, but I'd rather to have a second pair of eyes more familiar with the issue actual scope.

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

Created MR. Attached before & after screenshots of the Transaction from.

Before:

After:

plopesc → created an issue.

Branch rebased and MR is green. Back to NR.

MR created.

plopesc → created an issue.

We experienced something similar while integrating Navigation in one of our sites.

There was a global rule to apply specific styles to all the buttons across the whole site that cause conflicts with Navigation.

As reference, this is the diff that made the trick in that project:

-button,
+/* Apply button styles in layout container to avoid global overrides */
+.layout-container:where(button),
 [type="submit"] {
...

Taken the CSS bits from the mentioned issue. Ready for review.

@nod_ if I'm not mistaken, this bug was a regression introduced by 📌 Use a placeholder for the navigation toolbar Active to adapt Navigation to 📌 Render the navigation toolbar in a placeholder Active and looks like that none of them were backported to 11.1.x.

Not sure if we need to backport this one to 11.1.x then.

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.

Code looks good to me!

And it seems new bits fond are coming as part of other MRs.

Note for other reviewers: Instead of the 3 MRs approach suggested in #5 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active , the lines of code that triggers the error are in MR 11257, which will be always green because the Top Bar is not going to be present since the line to force it have been reverted. Please check the previous pipeline to confirm that worked, or test it locally adding back those lines.
That MR is ready to be merged as it is.

Last code bits have been addressed and tests are green now.

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.

Looks good to me. Thanks!

Thank you for your feedback @berdir.

Moved the post_update hook to the parent module to reduce possible frictions.

Would be great to grant credit in this issue to all the people who worked on the original one and helped to disclose the top bar visibility issues and fix them.
However I'm not allowed to grant credit by myself, I would ask core committers to add credits on my behalf, specially to joaopauloc.dev.

plopesc → created an issue.

Since this is not an easy bug to reproduce, created 2 branches:

• 3508199-navigation-top-bar-axe-test-only: It contains only the code necessary to make the error visible in Nightwatch tests
• 3508199-navigation-top-bar-axe: It should contain the code necessary to make the error visible in Nightwatch tests + the changes to fix it

Once tests in 3508199-navigation-top-bar-axe are green and approved, might be necessary to create a new branch that contains only the code necessary to fix the error. This branch will contain a diff between the 2 original ones, and that's the one that should be merged.

I know this is a bit tricky, but I have not been capable to figure out a better flow for this issue. If someone else have a better proposal, please feel free to show your concerns and ideas.

Postponing on 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active since some accessibility issues were found. See https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431574#L2723

Besides that, there is another Kernel test failing reporting something related to navigation_top bar module tests. Those tests were removed as part of this MR. Tried to reproduce locally and the test is passing. See https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431573#L659
Any idea about how this test could be addressed?

Add 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active

plopesc → created an issue. See original summary → .

Created MR that removes the checks for the module flag and marks it as obsolete.

Added a couple of questions to the MR about the bureaucracy of the process of marking the module as obsolete.

Would be great to have this enabled by default just removing the flag module.

Just a couple of questions.

For sites where the flag module is already enabled, how should we proceed? Just removing the module would cause an error. On the other hand, this was an alpha module within an experimental one, so people who enabled it should be able to manage this scenario.

Navigation is used in Drupal CMS, where Gin Toolbar is enabled as well. Removing the flag would automatically enable both top bars for sites using the 1.x branch, I think.

These 2 questions, and maybe others I missed, make me think that we might need a 2 steps path.

LGTM! RTBC +1

Bumping 📌 Adjust how Help link and Content links are removed from the Administration menu for the Navigation bar RTBC to Stable Blocker.

Bunmping it to stable blocker since this bug could cause unexpected behaviors in contrib modules. Found it problematic with Recurring Events.

Thank you! Just a minor comment regarding a redundant permission and this one should be ready to go :)

MR rebased and 3rd party tests included last weeks updaed accordingly. It is green now.

The alter hook suggested by catch was already implemented.

Also found that it can cause issues with contrib modules declaring menu items depending on the `system.admin_content` menu link.
Hence bumping the priority and marking it as a Bug.

Thank you for jumping into this.

Left some comments in the MR.

Went through the proposal and looks great to me.
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.

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

Environment Indicator provides its own top bar when detects that Toolbar integration is not present. That leads to unexpected behaviors when toolbar module is disabled or its integration disabled.

1. Install Drupal 11.x using standard profile
2. Install Navigation and Environment Indicator modules
3. Uninstall Toolbar
4. Add the Environment Indicator Navigation block to the sidebar

Actual behavior:

The extra top bar is unnecessary, but there is no way to get rid of it, since it is hardcoded in hook_preprocess_html() and hook_page_top().

For the hook_navigation_content_top implementation could be simple to just not add this top bar when Navigation module is enabled and its integration enabled. However, for the navigation sidebar block based approach, it's not trivial to found whether the block has been added to the sidebar, even if possible.

We're working on these tests in 🐛 Navigation Expand/Collapse logic is not working properly in conjunction with Big Pipe Active . If finally approved there, this one could be closed.

Rebased and made some adjustments. I think it needs a new round of reviews to ensure there are no regressions.

Minor suggestion about the usage of the class constant instead of the hardcoded value.
Test needs to be adjusted to reflect the new cache tag.

If I followed the reasoning correctly, that would imply to add a new prop or slot to the navigation:toolbar-button component.
Not sure if that would increase the component complexity more than we would like to in edge case scenarios where both title and button_label properties are set.

Back to the proposal, being able to make it happen at that level would simplify the overall logic. If that does not generate any undesired side effect, looks promising.

An extra pair of eyes from a more experienced FE would help to have a better idea of the pros and cons.

The comment related to the FunctionalJavascript test has not been resolved or implemented, so might need to be checked explicitly.
I think there was a misunderstanding related to the usage of static::NAVIGATION_LINKS_MENU, probably I was not clear enough in my original comment.

Created MR 11227 that demonstrates that test-only changes break the Nightwatch test, while the original MR 11124 is still green.

Made some new changes in the Nightwatch test to ensure that cache is cleared before reloading the navigation after installing bigpipe and it failed locally without the JS changes.

However, the test-only job stays green. I don't see whether the specific nightwatch test was executed either. Could it be something wrong with the CI job?

Tested manually and it looks very good after last changes.

We might need some test coverage to consider this one completed, though. This test could be inspired in Drupal\Tests\contextual\FunctionalJavascript\EditModeTest, where the toolbar integration was tested.

Besides that, we need to consider a couple of things:

• Eye icon is not being visible in latest chrome version. See 🐛 The icon for the more actions button is not visible Active
• Icons are being migrated to Icon API 📌 Navigation leverage icon core API Needs work . Might need to take this into account and migrate the eye icons to this new approach if that issue lands first

plopesc → changed the visibility of the branch 11.x to hidden.

plopesc → changed the visibility of the branch 3465295-integrate-top-bar-contextual-links-preview to hidden.

The idea is great and the code in the MR looks sensible to me.

However, I have not been able to find a way to make it work. This is probably out of my limited FE knowledge.

Adding some debugging information that could be useful for a FE development who could take a look into this:

Replace code in NavigationRenderer::buildNavigation() by something similar to this:

  public function buildNavigation(array &$page_top): void {
    $page_top['navigation'] = [
        '#type' => 'html_tag',
        '#tag' => 'aside',
        '#attributes' => [
          'class' => ['admin-toolbar'],
          'id' => 'admin-toolbar',
        ],
        'child' => [
          '#type' => 'container',
          '#attributes' => [
            'class' => ['admin-toolbar__displace-placeholder'],
          ],
        ],
    ];
  }

When any page is loaded, the space for the navigation bar should be reserved, but empty.

Once that behavior is achieved, that chunk of HTML should be moved to the '#lazy_builder_preview' render array property in the original output.

Created 📌 [PP-1] Integrate the new Navigation Icon API with NavigationLinkBlock Postponed as follow-up for the NavigationLinkBlock internals

plopesc → created an issue.

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 🐛 The icon for the more actions button is not visible Active . That issue might need to be updated
• Looks like some of the changes requested in 🐛 Undo some changes Active are being applied here. The scope of this issue and that one might be updated
• 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review 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.

Approach looks great and would simplify the code significantly if we can live with the extra cache items generated.

Those extra cache elements were my main concern to try to avoid caching per user initially. Since we are using the new placeholder approach, only the use menu bits are being cached per user, so seems a good balance.

Added a few comments to the MR, also a test might need to be updated to adapt it to the new cache tags.

IS might need to be updated as well, the current approach differs from the original one.

Thank you for the clean up and taking care of the unused dependency. That was my fault, I think.

From my perspective, it can be marked as RTBC.

Updated the issue summary to reflect the last requirements from a11y team.

Tested locally and worked as expected.

Fixed the Kernel tests failing due to conflicts between DomDocument::loadHtml and HTML5 tags (https://www.php.net/manual/en/domdocument.loadhtml.php)

I think this is ready for final review.

Tested locally disabling the views mentioned in the IS and test coverage is great.

Not only adds test coverage for the reported bug, but also for the rest of the logic in the NavigationContentLinks class.

Checked the code and looks good to me. Added a couple of comments in the MR that might help to clean it up a bit.

Agree that a possible 3rd color for this new status might be a good addition, but we could use this MR as an intermediate step while the UX and design teams come up with a final solution.

Thank you!

Tested manually and it behaves as expected, fixing the reported bug.

However, the JS here is out of my technical knowledge. So I prefer to have the RTBC signoff from a more experienced FE developer.

Thank you!

Created MR where the mentioned if clause is replaced by a check for the element in the context.
Also updated the specific Nightwatch test for this functionality to use big_pipe.

Tested manually and at first glance it works well, but a review from more experienced JS developer would be appreciated.

Thank you for reporting!

Filled 🐛 Navigation Expand/Collapse logic is not working properly in conjunction with Big Pipe Active to keep track of it.

plopesc → created an issue.

They're not directly related to caching, but the way it was supposed to cache the navigation HTML markup in local storage to not serve it in every request was not compatible with having IDs in the navigation HTML markup.

All the discussion and the reasoning is available in 📌 Implement a caching strategy for the menu links Active .

The

IDs were removed in https://git.drupalcode.org/project/drupal/-/merge_requests/7980/diffs?co... and they need to be brought back using JS for a11y reasons.

The

Those IDs can still be there, since we are changing the way to improve the Navigation performance.

Made a rebase once 📌 Use a placeholder for the navigation toolbar Active was merged and addressed the last comment in the MR.

I think we can close this one.

The scenario reported in the issue is already covered in NavigationUserBlockTest::testNavigationUserBlock().

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

Created 📌 Add tests to ensure that Navigation Top Bar Page actions respect the original local tasks weight Active to take care of the missing test.

plopesc → created an issue.

Great job with the tests! Added some comments to the MR to polish them a bit.

Worked on the bits mentioned in the MR. I think this is ready for a new review.

I already made the Navigation changes in the old MR that was replaced by the current one, so I could easily cherry-pick them. On the other hand, keeping the focus here would make this issue simpler.

I'm totally OK if you feel that's better to have methods separated, just wanted to reduce a bit the amount of code, but the downsides could be higher than the benefits.

Regarding docblocks, I can take a look there, current ones were just a copy&paste from the suggested in #12.

MR created. It still need some tests, though.

Checked the codebase and found out that NavigationContentLinks class does not have test coverage yet. This issue could be a good opportunity to add a new NavigationContentLinksTest class and test the behavior of the menu modifications made by Navigation module.

plopesc → created an issue.

New MR open that adds the missing dependency.

Great catch!

plopesc → changed the visibility of the branch 3480568-navigation-recipe to hidden.

Back to Needs Review once conflicts have been solved and last comments in the MR addressed.

If the changes here are good and the approach is validated, it might be time to figure out how to implement specific tests.

MR has been merged and 2.1.0 released.

Thank you all for your help here!

Drop 9.x support since new changes breaks compatibility with that core version.

Preparing a new 2.1.0 module release fro 10.3+.

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

plopesc → changed the visibility of the branch 3493911-add-a-cachedplaceholderstrategy to hidden.

MR updated. Back to Needs Review.

Thank you for your feedback.

Started to work on a draft MR based on the boilerplate code from #11 & #12 with some minor adjustments to make it work, but found some issues related to recursion of CachedStrategy & Big Pipe. After the 1st load, once the cache is warm, the page content is not being loaded and the following error is thrown instead:
The #lazy_builder property must have an array as a value, containing two values: the callback, and the arguments for the callback. in <assert() (line 327 of core/lib/Drupal/Core/Render/Renderer.php)

It seems due to the fact that the same placeholder is being rendered in BigPipeStrategy::createBigPipeNoJsPlaceholder() twice from different places, ended up in having the same #lazy_builder callback duplicated. Hence, when the #attached arrays are merged as part of the page build process, the callback array has 4 elements instead of 2. This is happening for the Logout link placeholder, which is rendered both in the User Account menu and in Navigation.

Steps to reproduce

1. Checkout the MR branch
2. Install Standard profile and Navigation module
3. Login as admin
4. Go to the homepage while the cache is cold, and the page is loaded as expected
5. Reload the page, once the cache is warm
6. Confirm that page content is not being displayed and the error above is included in the HTML markup

I'm not sure how to proceed here, we could indicate at build time that #lazy_builder callbacks should not be merged, but that could be complex. Another option could be to keep track of the generated placeholders somewhere and avoid duplicates.

Any idea?

plopesc → created an issue.

Thank you for working on this new approach! Good progress here.

Left some comments in the MR.

I could have some bandwith to start to work on this issue this week.
Went through the comments and it's not 100% clear to me from where I should start my investigation, or even if all the pieces to start are already in place.
Would be great if you could add comment to indicate the possible steps to follow that would help me to kick it off.

Made a couple of simple proposals to try to unblock this one:

When there are 2 level tasks, show the 1st level in the page actions drop down and shown the 2nd level tasks as 2nd level tasks, like in the original behavior:

When there are 2 level tasks, show the 1st level in the page actions drop down and shown the 2nd level tasks as 1st level tasks, promoting them one level:

Any of these options would not need to define any extra design, nor define complex behaviors. They just set a rule about how to deal with the 2nd level local tasks in a generic way.

Created follow-up issue 📌 [PP-1] Convert Navigation messages component to SDC Postponed to address the SDC conversion.

Postponed on 📌 Integrate Umami message Active

plopesc → created an issue.

Thank you @ckrina.

Moving it outside of the dropdown will make things a bit easier. It could be implemented as a new TopBarItem plugin placed in the Top Bar's Tools region. Also it would allow to reuse, or at least take as inspiration, the JS implementation used for the Toolbar module.

Conflicts solved and tests are still green. Moving back to RTBC.

Upgrade path is now a post_update hook. Marking as Needs Review to get a valid manual review from the community.

Upgrade path added and moved back to RTBC.

Created a Content Moderation specific test class as part of 🐛 Navigation Top Bar should Edit button as the primary action when viewing a forward revision Active that could be useful to define the tests in this issue.