Problem/Motivation

When adding a Menu Item, it is possible to add special items like <nolink> or <button>

According to Menu Link form:
Enter <nolink> to display link text only. Enter <button> to display keyboard-accessible link text only.

Current navigation twig template structure does not respect these items and render them as links whose href is empty, having a different behavior than the expected one.

Attaching screenshots of the same menu as part of navigation or as a block to show the differences:

Steps to reproduce

  • Install a Drupal site and enable navigation
  • Edit any of the menus included in the navigation bar
  • Add menu links whose Link field value is <nolink> or <button>
  • Confirm that those items are shown as empty links instead of the expected HTML markup

Proposed resolution

Update navigation twig templates to respect the special menu items behavior.
Checked how menu.html.twig works and it seems this piece of code generates the expected markup:

{{
  link(
    item.title,
    item.url,
    item.attributes.removeClass(item_classes).addClass(link_classes)
  )
}}

Could be worth to explore how it works and try to reuse or replicate its behavior.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 3447837-nr-bot.txt4.09 KBneeds-review-queue-bot
#12 Monosnap Edit menu Administration | Drush Site-Install 2024-05-24 15-55-58.png192.88 KBm4olivei
#8 Umami-BeforeMR.png37.6 KBkanchan bhogade
#8 BeforeMR.png38.46 KBkanchan bhogade
#8 Umami-AfterMR.png107.03 KBkanchan bhogade
#8 AfterMR.png37.52 KBkanchan bhogade
menu-block-special-links.png24.31 KBplopesc
umami-special-menu-links.png208.55 KBplopesc

Issue fork drupal-3447837

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

plopesc created an issue. See original summary.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue tags: +Navigation stable blocker
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Assigned: Unassigned » kostyashupenko

In progress

kostyashupenko opened merge request !8117

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Assigned: kostyashupenko » Unassigned
Status: Active » Needs review

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Need tests

Can we had a test case to cover this scenario please

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
Issue tags: -Need tests

Tests added for this new scenario.

kanchan bhogade’s picture

kanchan bhogade commented
StatusFileSize
new AfterMR.png37.52 KB
new Umami-AfterMR.png107.03 KB
new BeforeMR.png38.46 KB
new Umami-BeforeMR.png37.6 KB

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

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

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

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
StatusFileSize
new Monosnap Edit menu Administration | Drush Site-Install 2024-05-24 15-55-58.png192.88 KB

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

finnsky’s picture

finnsky commented
Status: Reviewed & tested by the community » Needs work

I would like to fix some css and twig here.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs work » Needs review

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3447837-nr-bot.txt4.09 KB

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.

finnsky’s picture

finnsky commented

@m4olivei

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

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
finnsky’s picture

finnsky commented
Issue tags: +Needs change record

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

finnsky’s picture

finnsky commented
Issue tags: -Needs change record

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

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Needs work

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.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs work » Needs review

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

finnsky’s picture

finnsky commented
Issue tags: +Needs frontend framework manager review
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

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

Thank you!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs review

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

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Per the response in #3458215: Migrate Toolbar button to SDC these appear to be separate.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Assigned: Unassigned » m4olivei
Status: Reviewed & tested by the community » Needs work
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs work » Needs review

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.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

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

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Assigned: m4olivei » Unassigned
plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs frontend framework manager review

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

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.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Reviewed & tested by the community

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

I think this one can be marked as RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed 5f44b4b4 on 10.3.x 
    Issue #3447837 by finnsky, m4olivei, plopesc, kostyashupenko, kanchan...

  • alexpott committed a5a8967c on 10.4.x 
    Issue #3447837 by finnsky, m4olivei, plopesc, kostyashupenko, kanchan...

  • alexpott committed 2305daca on 11.0.x 
    Issue #3447837 by finnsky, m4olivei, plopesc, kostyashupenko, kanchan...

  • alexpott committed 1c67d3b5 on 11.x 
    Issue #3447837 by finnsky, m4olivei, plopesc, kostyashupenko, kanchan...

Status: Fixed » Closed (fixed)

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