Issue #3447837 by finnsky, m4olivei, plopesc, kostyashupenko, kanchan bhogade,...

Issue #3447837 by finnsky, m4olivei, plopesc, kostyashupenko, kanchan bhogade, smustgrave, alexpott: Special Menu items are rendered as empty links in navigation

core/modules/navigation/templates/navigation-menu.html.twig

@@ -8,16 +8,33 @@
 
 {% macro menu_items(items, attributes, menu_level) %}
   {% for item in items %}
+
+    {% set item_link_tag = 'a' %}
+
+    {% if item.url.isRouted %}
+      {% if item.url.routeName == '<nolink>' %}
+        {% set item_link_tag = constant('\\Drupal\\Core\\GeneratedNoLink::TAG') %}
+      {% elseif item.url.routeName == '<button>' %}
+        {% set item_link_tag = constant('\\Drupal\\Core\\GeneratedButton::TAG') %}
+      {% endif %}
+    {% endif %}
+
+    {% if item.url.options.attributes is empty %}
+      {% set item_link_attributes = create_attribute() %}
+    {% else %}
+      {% set item_link_attributes = create_attribute(item.url.options.attributes) %}
+    {% endif %}
+
     {% set item_id = ('navigation-link--' ~ item.original_link.pluginId)|clean_unique_id %}
     {% if menu_level == 0 %}
-      {% if item.below is empty  %}
+      {% if item.below is empty %}
         <li id="{{ item_id }}" class="toolbar-block__list-item">
           {% include '@navigation/toolbar-button.html.twig' with {
-            attributes: create_attribute({ 'href': item.url|render, 'data-drupal-tooltip': item.title, 'data-drupal-tooltip-class': 'admin-toolbar__tooltip' }),
+            attributes: item_link_attributes.setAttribute('href', item.url|render|default(null)).setAttribute('data-drupal-tooltip', item.title).setAttribute('data-drupal-tooltip-class', 'admin-toolbar__tooltip'),
             icon: item.class|clean_class,
-            html_tag: 'a',
+            html_tag: item_link_tag,
             text: item.title,
-            extra_classes: 'toolbar-button--collapsible',
+            extra_classes: 'toolbar-button--collapsible ' ~ (item_link_tag == 'span' ? 'toolbar-button--non-interactive'),
           } only %}
         </li>
       {% else %}
@@ -32,7 +49,7 @@
           } only %}
           <div class="toolbar-popover__wrapper" data-toolbar-popover-wrapper inert>
             {% if item.url %}
-              {{ link(item.title, item.url, create_attribute({'class': ['toolbar-popover__header', 'toolbar-button', 'toolbar-button--large', 'toolbar-button--dark']})) }}
+              {{ link(item.title, item.url, item_link_attributes.addClass(['toolbar-popover__header', 'toolbar-button', 'toolbar-button--large', 'toolbar-button--dark'])) }}
             {% else %}
               <span class="toolbar-popover__header toolbar-button toolbar-button--large toolbar-button--dark toolbar-button--non-interactive">{{ item.title }}</span>
             {% endif %}
@@ -46,11 +63,12 @@
     {% elseif menu_level == 1 %}
       <li class="toolbar-menu__item toolbar-menu__item--level-{{ menu_level }}">
         {% if item.below is empty  %}
-          <a
-            href="{{ item.url }}"
-            class="toolbar-button"
-            data-index-text="{{ item.title|first|lower }}"
-          >{{ item.title }}</a>
+          {% include '@navigation/toolbar-button.html.twig' with {
+            attributes: item_link_attributes.setAttribute('href', item.url|render|default(null)),
+            text: item.title,
+            html_tag: item_link_tag,
+            extra_classes: item_link_tag == 'span' ? 'toolbar-button--non-interactive',
+          } only %}
         {% else %}
           <button
             class="toolbar-button toolbar-button--expand--down"
@@ -69,11 +87,13 @@
     {% else %}
       <li class="toolbar-menu__item toolbar-menu__item--level-{{ menu_level }}">
         {% if item.below is empty  %}
-          <a
-            href="{{ item.url }}"
-            class="toolbar-menu__link toolbar-menu__link--{{ menu_level }}"
-            data-index-text="{{ item.title|first|lower }}"
-          >{{ item.title }}</a>
+          {{ link(item.title, item.url, {
+            'class': [
+              'toolbar-menu__link',
+              'toolbar-menu__link--' ~ menu_level,
+            ],
+            'data-index-text': item.title|first|lower
+          }) }}
         {% else %}
           <button
             class="toolbar-menu__link toolbar-menu__link--{{ menu_level }}"

core/modules/navigation/templates/toolbar-button.html.twig

@@ -10,7 +10,7 @@ appear after main classes #}
 <{{html_tag|default('button')}}
   {{ attributes.addClass(classes) }}
   data-index-text="{{ text|first|lower }}"
-  data-icon-text={{- text|render|trim|slice(0, 2)|join('') }}
+  data-icon-text="{{- text|render|trim|slice(0, 2)|join('') -}}"
 >
   {% if avatar %}
     <span class="toolbar-button__avatar">{{~ avatar ~}}</span>

core/modules/navigation/tests/src/Kernel/SystemMenuNavigationBlockTest.php → core/modules/navigation/tests/src/Kernel/NavigationMenuBlockTest.php

@@ -6,7 +6,9 @@
 
 use Drupal\block\Entity\Block;
 use Drupal\Core\Render\Element;
+use Drupal\Core\Render\MetadataBubblingUrlGenerator;
 use Drupal\Core\Routing\RouteObjectInterface;
+use Drupal\Core\Routing\UrlGenerator;
 use Drupal\KernelTests\KernelTestBase;
 use Drupal\navigation\Plugin\Block\NavigationMenuBlock;
 use Drupal\system\Entity\Menu;
@@ -20,7 +22,7 @@
 use Symfony\Component\Routing\RouteCollection;
 
 /**
- * Tests \Drupal\navigation\Plugin\Block\SystemMenuNavigationBlock.
+ * Tests \Drupal\navigation\Plugin\Block\NavigationMenuBlock.
  *
  * @group navigation
  * @see \Drupal\navigation\Plugin\Derivative\SystemMenuNavigationBlock
@@ -28,7 +30,7 @@
  * @todo Expand test coverage to all SystemMenuNavigationBlock functionality,
  * including block_menu_delete().
  */
-class SystemMenuNavigationBlockTest extends KernelTestBase {
+class NavigationMenuBlockTest extends KernelTestBase {
 
   /**
    * {@inheritdoc}
@@ -102,19 +104,31 @@ protected function setUp(): void {
     $routes = new RouteCollection();
     $requirements = ['_access' => 'TRUE'];
     $options = ['_access_checks' => ['access_check.default']];
-    $routes->add('example1', new Route('/example1', [], $requirements, $options));
+    $special_options = $options + ['_no_path' => TRUE];
     $routes->add('example2', new Route('/example2', [], $requirements, $options));
-    $routes->add('example3', new Route('/example3', [], $requirements, $options));
     $routes->add('example4', new Route('/example4', [], $requirements, $options));
-    $routes->add('example5', new Route('/example5', [], $requirements, $options));
-    $routes->add('example6', new Route('/example6', [], $requirements, $options));
-    $routes->add('example7', new Route('/example7', [], $requirements, $options));
-    $routes->add('example8', new Route('/example8', [], $requirements, $options));
     $routes->add('example9', new Route('/example9', [], $requirements, $options));
+    $routes->add('example11', new Route('/example11', [], $requirements, $options));
 
+    // Mock special routes defined in system.routing.yml.
+    $routes->add('<nolink>', new Route('', [], $requirements, $special_options));
+    $routes->add('<button>', new Route('', [], $requirements, $special_options));
+
+    // Define our RouteProvider mock.
     $mock_route_provider = new MockRouteProvider($routes);
     $this->container->set('router.route_provider', $mock_route_provider);
 
+    // Define our UrlGenerator service that use the new RouteProvider.
+    $url_generator_non_bubbling = new UrlGenerator(
+      $mock_route_provider,
+      $this->container->get('path_processor_manager'),
+      $this->container->get('route_processor_manager'),
+      $this->container->get('request_stack'),
+      $this->container->getParameter('filter_protocols')
+    );
+    $url_generator = new MetadataBubblingUrlGenerator($url_generator_non_bubbling, $this->container->get('renderer'));
+    $this->container->set('url_generator', $url_generator);
+
     // Add a new custom menu.
     $menu_name = 'mock';
     $label = $this->randomMachineName(16);
@@ -127,27 +141,33 @@ protected function setUp(): void {
     $this->menu->save();
 
     // This creates a tree with the following structure:
-    // - 1
+    // - 1 (nolink)
     // - 2
-    //   - 3
+    //   - 3 (nolink)
     //     - 4
     //       - 9
-    // - 5
-    //   - 7
+    // - 5 (button)
+    //   - 7 (button)
+    //     - 10 (nolink)
     // - 6
-    // - 8
+    // - 8 (nolink)
+    //   - 11
+    //     - 12 (button)
     // With link 6 being the only external link.
     // phpcs:disable
     $links = [
-      1 => MenuLinkMock::create(['id' => 'test.example1', 'route_name' => 'example1', 'title' => 'title 1', 'parent' => '', 'weight' => 0]),
+      1 => MenuLinkMock::create(['id' => 'test.example1', 'route_name' => '<nolink>', 'title' => 'title 1', 'parent' => '', 'weight' => 0]),
       2 => MenuLinkMock::create(['id' => 'test.example2', 'route_name' => 'example2', 'title' => 'title 2', 'parent' => '', 'route_parameters' => ['foo' => 'bar'], 'weight' => 1]),
-      3 => MenuLinkMock::create(['id' => 'test.example3', 'route_name' => 'example3', 'title' => 'title 3', 'parent' => 'test.example2', 'weight' => 2]),
+      3 => MenuLinkMock::create(['id' => 'test.example3', 'route_name' => '<nolink>', 'title' => 'title 3', 'parent' => 'test.example2', 'weight' => 2]),
       4 => MenuLinkMock::create(['id' => 'test.example4', 'route_name' => 'example4', 'title' => 'title 4', 'parent' => 'test.example3', 'weight' => 3]),
-      5 => MenuLinkMock::create(['id' => 'test.example5', 'route_name' => 'example5', 'title' => 'title 5', 'parent' => '', 'expanded' => TRUE, 'weight' => 4]),
-      6 => MenuLinkMock::create(['id' => 'test.example6', 'route_name' => '', 'url' => 'https://www.drupal.org/', 'title' => 'title 6', 'parent' => '', 'weight' => 5]),
-      7 => MenuLinkMock::create(['id' => 'test.example7', 'route_name' => 'example7', 'title' => 'title 7', 'parent' => 'test.example5', 'weight' => 6]),
-      8 => MenuLinkMock::create(['id' => 'test.example8', 'route_name' => 'example8', 'title' => 'title 8', 'parent' => '', 'weight' => 7]),
+      5 => MenuLinkMock::create(['id' => 'test.example5', 'route_name' => '<button>', 'title' => 'title 5', 'parent' => '', 'expanded' => TRUE, 'weight' => 4]),
+      6 => MenuLinkMock::create(['id' => 'test.example6', 'route_name' => '', 'url' => 'https://www.drupal.org/', 'title' => 'title 6', 'parent' => '', 'weight' => 5, 'options' => ['attributes' => ['target' => '_blank', 'class' => ['external-link']]]]),
+      7 => MenuLinkMock::create(['id' => 'test.example7', 'route_name' => '<button>', 'title' => 'title 7', 'parent' => 'test.example5', 'weight' => 6]),
+      8 => MenuLinkMock::create(['id' => 'test.example8', 'route_name' => '<nolink>', 'title' => 'title 8', 'parent' => '', 'weight' => 7]),
       9 => MenuLinkMock::create(['id' => 'test.example9', 'route_name' => 'example9', 'title' => 'title 9', 'parent' => 'test.example4', 'weight' => 7]),
+      10 => MenuLinkMock::create(['id' => 'test.example10', 'route_name' => '<nolink>', 'title' => 'title 10', 'parent' => 'test.example7', 'weight' => 7]),
+      11 => MenuLinkMock::create(['id' => 'test.example11', 'route_name' => 'example11', 'title' => 'title 11', 'parent' => 'test.example8', 'weight' => 7]),
+      12 => MenuLinkMock::create(['id' => 'test.example12', 'route_name' => '<button>', 'title' => 'title 12', 'parent' => 'test.example11', 'weight' => 7]),
     ];
     // phpcs:enable
     foreach ($links as $instance) {
@@ -218,9 +238,12 @@ public function testConfigLevelDepth(): void {
     $expectations['level_2_only'] = [
       'test.example3' => [],
       'test.example7' => [],
+      'test.example11' => [],
     ];
     $expectations['level_3_only'] = [
       'test.example4' => [],
+      'test.example10' => [],
+      'test.example12' => [],
     ];
     $expectations['level_1_and_beyond'] = [
       'test.example1' => [],
@@ -230,10 +253,16 @@ public function testConfigLevelDepth(): void {
         ],
       ],
       'test.example5' => [
-        'test.example7' => [],
+        'test.example7' => [
+          'test.example10' => [],
+        ],
       ],
       'test.example6' => [],
-      'test.example8' => [],
+      'test.example8' => [
+        'test.example11' => [
+          'test.example12' => [],
+        ],
+      ],
     ];
     $expectations['level_2_and_beyond'] = [
       'test.example3' => [
@@ -241,12 +270,19 @@ public function testConfigLevelDepth(): void {
           'test.example9' => [],
         ],
       ],
-      'test.example7' => [],
+      'test.example7' => [
+        'test.example10' => [],
+      ],
+      'test.example11' => [
+        'test.example12' => [],
+      ],
     ];
     $expectations['level_3_and_beyond'] = [
       'test.example4' => [
         'test.example9' => [],
       ],
+      'test.example10' => [],
+      'test.example12' => [],
     ];
     // Scenario 1: test all navigation block instances when there's no active
     // trail.
@@ -277,6 +313,46 @@ public function testConfigLevelDepth(): void {
     }
   }
 
+  /**
+   * Tests the generated HTML markup.
+   */
+  public function testHtmlMarkup() {
+    $block = $this->blockManager->createInstance('navigation_menu:' . $this->menu->id(), [
+      'region' => 'content',
+      'id' => 'machine_name',
+      'level' => 1,
+      'depth' => NavigationMenuBlock::NAVIGATION_MAX_DEPTH - 1,
+    ]);
+
+    $block_build = $block->build();
+    $render = \Drupal::service('renderer')->renderRoot($block_build);
+
+    $dom = new \DOMDocument();
+    $dom->loadHTML((string) $render);
+    $xpath = new \DOMXPath($dom);
+
+    $items_query = [
+      "//li[contains(@class,'toolbar-block__list-item')]/span/span[text()='title 1']",
+      "//li[contains(@class,'toolbar-block__list-item')]/button/span[text()='title 2']",
+      "//li[contains(@class,'toolbar-menu__item--level-1')]/button/span[text()='title 3']",
+      "//li[contains(@class,'toolbar-menu__item--level-2')]/a[text()='title 4']",
+      "//li[contains(@class,'toolbar-block__list-item')]/button/span[text()='title 5']",
+      "//li[contains(@class,'toolbar-block__list-item')]/a/span[text()='title 6']",
+      "//li[contains(@class,'toolbar-block__list-item')]/a[contains(@class, 'external-link')]",
+      "//li[contains(@class,'toolbar-block__list-item')]/a[contains(@class, 'external-link')]",
+      "//li[contains(@class,'toolbar-block__list-item')]/a[@target='_blank']",
+      "//li[contains(@class,'toolbar-menu__item--level-1')]/button/span[text()='title 7']",
+      "//li[contains(@class,'toolbar-block__list-item')]/button/span[text()='title 8']",
+      "//li[contains(@class,'toolbar-menu__item--level-2')]/span[text()='title 10']",
+      "//li[contains(@class,'toolbar-menu__item--level-1')]/button/span[text()='title 11']",
+      "//li[contains(@class,'toolbar-menu__item--level-2')]/button[text()='title 12']",
+    ];
+    foreach ($items_query as $query) {
+      $span = $xpath->query($query);
+      $this->assertEquals(1, $span->length, $query);
+    }
+  }
+
   /**
    * Helper method to allow for easy menu link tree structure assertions.
    *

core/modules/navigation/tests/src/Kernel/NavigationMenuMarkupTest.php

@@ -118,7 +118,7 @@ protected function setUp(): void {
     $links = [
       1 => MenuLinkMock::create(['id' => 'test.example1', 'route_name' => 'example1', 'title' => 'title 1', 'parent' => '', 'weight' => 0]),
       2 => MenuLinkMock::create(['id' => 'test.example2', 'route_name' => 'example2', 'title' => 'Another title', 'parent' => '', 'route_parameters' => ['foo' => 'bar'], 'weight' => 1]),
-      3 => MenuLinkMock::create(['id' => 'test.example3', 'route_name' => 'example3', 'title' => 'A menu link', 'parent' => 'test.example2', 'weight' => 2]),
+      3 => MenuLinkMock::create(['id' => 'test.example3', 'route_name' => 'example3', 'title' => 'Nested menu link', 'parent' => 'test.example2', 'weight' => 2]),
     ];
     // phpcs:enable
     foreach ($links as $instance) {
@@ -149,12 +149,12 @@ public function testToolbarButtonAttributes(): void {
       "//li[contains(@class,'toolbar-block__list-item')]/a[@data-icon-text='ti']",
       "//li[contains(@class,'toolbar-block__list-item')]/button[@data-index-text='a']",
       "//li[contains(@class,'toolbar-block__list-item')]/button[@data-icon-text='An']",
-      "//li[contains(@class,'toolbar-menu__item--level-1')]/a[@data-index-text='a']",
-      "//li[contains(@class,'toolbar-menu__item--level-1')]/a[not(@data-icon-text)]",
+      "//li[contains(@class,'toolbar-menu__item--level-1')]/a[@data-index-text='n']",
+      "//li[contains(@class,'toolbar-menu__item--level-1')]/a[@data-icon-text='Ne']",
     ];
     foreach ($items_query as $query) {
-      $span = $xpath->query($query);
-      $this->assertEquals(1, $span->length, $query);
+      $element = $xpath->query($query);
+      $this->assertEquals(1, $element->length, $query);
     }
   }