Special Menu items are rendered as empty links in navigation

In progress

Good catch.

It's now ready for review.
made also span element not interactable visually (no hover styles). Also added few fixes for forced-colors: active mode

Can we had a test case to cover this scenario please

Tests added for this new scenario.

Hi
I've tested MR !8117 on Drupal 11
The MR is applied cleanly...

Special Menu items' empty links in the navigation issue is fixed.

RTBC+1

Adding SS

Thanks @plopesc for the quick tests

There was 1 failure:
1) Drupal\Tests\navigation\Kernel\NavigationMenuBlockTest::testHtmlMarkup
//li[contains(@class,'toolbar-block__list-item')]/span/span[text()='title 1']
Failed asserting that 0 matches expected 1.
/builds/issue/drupal-3447837/core/modules/navigation/tests/src/Kernel/NavigationMenuBlockTest.php:351
FAILURES!
Tests: 3, Assertions: 35, Failures: 1.<code>

Coverage appears to be there.
Manual testing I get the same as the issue summary.
Code change appears fine

+1 RTBC. I tested locally and it works as you might expect:

I would like to fix some css and twig here.

I took a crack at breaking the standstill on the one issue. Lets see what we all think.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@m4olivei

I love that idea. existing twig function isn't really flexible.

I think it may need change record since we add new function in Drupal Twig here.

CR: https://www.drupal.org/node/3454916 →

Thank you for pushing this one!

Checked code and found out that new approach is not respecting menu link attributes. Added extra tests to confirm it and avoid overlooking this again.

I don't know if the new approach clears up all the doubts about the use of stateless functions and how to handle the use of menu links in SDC expressed in the original version.

That is a bigger concern that not only affects to navigation, but also to how to render menus in SDC. A follow up issue to address this in a global and consolidated way could be worth here.

I think we're all set on all threads. Tests now passing and ready for a re-review.

Marking as RTBC, we need now the framework manager review for the new Twig function.

Thank you!

Is this made obsolete by 📌 Migrate Toolbar button to SDC Needs review ? In the other MR an item without URL is taken into account.

I'd rather not add another Drupal specific twig function.

Per the response in 📌 Migrate Toolbar button to SDC Needs review these appear to be separate.

I found a regression with the use of the new link_tag Twig function in Olivero. That obviously weakens the case for it, as navigation would be the single use of it. Added to the hesitation for adding another Twig function, I've removed it and instead taken inspriation from Olivero's template, using the same conditional for our purposes of deriving the HTML tag name from the URL object.

Should be all set now. Hopefully will be easier to get through without the introduction of the Twig function.

Also note, if we have agreement, @finnsky we can delete the change record.

Code looks good to me!

Once we are not adding the new twig function, I believe the frontend framework manager review is not needed anymore.

I've added a comment on the MR. I think we need to adjust things because we're in a loop and it is possible item_link_tag will have a value from a previous iteration of the loop. Also we can use the constants to keep things consistent.

Code suggestions applied, MR branch synced with 11.x and tests are green.

I think this one can be marked as RTBC.

Committed and pushed 1c67d3b559 to 11.x and 2305dacae3 to 11.0.x and a5a8967cd4 to 10.4.x and 5f44b4b454 to 10.3.x. Thanks!

Backported to 10.3.x as navigation is an experimental module.

Automatically closed - issue fixed for 2 weeks with no activity.