Menu blocks in Layout Builder get malformed aria-labelledby attribute

This looks great - nice and clean fix.

Can we get a test-only patch so we can see red/green?

Other than that, this is RTBC in my book

Test only patch

The last submitted patch, 33: 3083181-33-tests-only.patch, failed testing. View results →

Thanks, great work folks - this is RTBC in my book

1. --- a/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   +++ b/core/modules/layout_builder/tests/src/Functional/LayoutBuilderTest.php
   @@ -656,6 +656,7 @@ public function testPluginDependencies() {
        // Assert that the blocks are visible, and save the layout.
        $assert_session->pageTextContains('Powered by Drupal');
        $assert_session->pageTextContains('My Menu');
   +    $assert_session->elementExists('xpath', "//nav[contains(@aria-labelledby, 'block-system-menu-block-my-menu')]");
        $assert_session->elementExists('css', '.block.menu--my-menu');
        $page->pressButton('Save layout');

   Could this get an additional assertion that confirms #block-system-menu-block-my-menu exists, and has content suitable for a label?

2. The issue summary and title make several references to aria-describedby. The solution impacts aria-labelledby. This appears to be the right approach, but the issue summary should be updated to be consistent with that.

Updated the issue summary, +1 for more tests

Updated IS and added additional assertion.

Hi @smustgrave , Tested your patch on drupal version 10.1.x. It was successfully applied and i got solution. Adding screenshots for the reference. Need RTBC+1

Verified patch #38 on 10.1.x-dev. Patch applied cleanly.
Test Result: The NAV element surrounding the menu should have an aria-labelledby attribute has correct and complete value.
Screenshot available in #39

RTBC +1

Tested in 10.1.x-dev. by adding 2 user account menus and was able to get the same result as #40.

I worked with vleeayo1 on this for their first contribution as part of the Discover Drupal program! Based on convos above this should be RTBC. Code and tests make sense to me

The last submitted patch, 38: 3083181-38.patch, failed testing. View results →

Random failure

I think we should be using \Drupal\Component\Utility\Html::getUniqueId to make sure that each block gets a unique id even if the layout is rendered multiple times on a single page. I'm not sure that we need the str_replace call but we may as well keep it since it looks like colons would not be converted to a dash but just removed by \Drupal\Component\Utility\Html::getUniqueId.

Seems using Html::getUniqueId($block->getPluginId())) makes the ID block-system-menu-blockmy-menu vs block-system-menu-block-my-menu due to line 230 of Html.php

But updated tests for using getUniqueId

This looks great. Look forward to seeing this bug fixed.

Addressed feedback.

larowlan → changed the visibility of the branch 3083181-10.1.x to hidden.

Everything looks good to me now. Thanks for the updates

Are we sure that the Html::getUniqueId() is necessary here - we're going to call that again in template_preprocess_block() on the #id

Also I think it'd be great to work out the root cause so that we can add a warning or something if another system places a menu block in the same way as layout builder.

Sounds like more work is needed.

So the cause is the twig template - block--system-menu-block.html.twig

{% set heading_id = attributes.id ~ '-menu'|clean_id %}
<nav role="navigation" aria-labelledby="{{ heading_id }}"{{ attributes|without('role', 'aria-labelledby') }}>
  {# Label. If not displayed, we still provide it for screen readers. #}
  {% if not configuration.label_display %}
    {% set title_attributes = title_attributes.addClass('visually-hidden') %}
  {% endif %}
  {{ title_prefix }}
  <h2{{ title_attributes.setAttribute('id', heading_id) }}>{{ configuration.label }}</h2>
  {{ title_suffix }}

This code pretty much requires attributes.id to be set. I think the fix there is correct - we should be adding an ID to all blocks placed by the layout builder.

I was wondering the use-case of placing blocks without an ID. It's definitely not mandated by the system as we have code like:

  if (!empty($variables['elements']['#id'])) {
    $suggestions[] = 'block__' . $variables['elements']['#id'];
  }

But this brings up something else interesting. We definitely should not be using getUniqueId() here as that results in different theme suggestions being applied. The ID we're adding here result in the following suggestions:

• block__system
• block__system_menu_block
• block__system_menu_block__account
• block__system-menu-blockaccount

whereas the menu block placed by the block system results in:

1. block__system
2. block__system_menu_block
3. block__system_menu_block__account
4. block__olivero_account_menu

So calling Html::cleanCssIdentifier() and Html::getUniqueId() is wrong. Because the mix of _ and - in theme suggestion seems very wrong. But also we need to do something to remove the : from the plugin ID. I guess we should be inspired by the code in block_theme_suggestions_block()...

  $parts = explode(':', $variables['elements']['#plugin_id']);
  $suggestion = 'block';
  while ($part = array_shift($parts)) {
    $suggestions[] = $suggestion .= '__' . strtr($part, '-', '_');
  }

We don't need to prefix the ID with block as that happens automatically but maybe something like:

'#id' => 'layout_builder__' . str_replace(':', '__', $block->getPluginId());

will work. But I'm very unsure about the 'layout_builder__' . part.