System Menu blocks placed via Layout Builder can generate duplicate, invalid HTML ID attributes

Here's a patch. Not really helpful for anyone, as it uses a UUID. But at least the markup is valid.

Note that this is really a bug in block module for wrongly assuming there will be an ID available...

Review Original patch https://www.drupal.org/files/issues/2019-08-12/3073895-id-2.patch it was written in wrong way to attached ID attributes with Block.
New Patch applied & tested with drupal 8.9.x , 9.0.x & 9.1.x versions
its working fine

Please review and confirm

Upload Interdiff for last patch with new patch

On Code base its working but as Test cases fail so maybe we need to change on Test case

Correct me if I'm wrong.

Need Review on patch & test cases

Fixing test cases file.

Hi,
I have missed this function testContextAwareBlock in the previous patch. Updated the patch again.

Fixing the test cases on section render test file.

Fixing code standards.

Fixing the coding standard.

NW for the missing test coverage.
Also #21 was the opposite, there should be spaces there

Apologies for the late reply @tim.plunket was bit occupied. I guess the test cases were already present. I didn't get your comment about test cases. Can you please tell me which test cases you are talking about.

Should this perhaps be merged with #3020876: Contextual links of reusable content blocks are not displayed when rendering entities built via Layout Builder?

Simple re-roll of the patch from #21 so it applies cleanly with composer-patches on 9.3.x and 9.4.x. No interdiff because no code lines changed, only surrounding context.

latest patch doesn't apply to 9.5

Patch for 9.5.x.

the patch in #32 applies cleanly and the functionality has returned. thanks!

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

For the specific test coverage still needed.

Also I see that the tests were updated is that because this attribute is now required? Won't this break existing sites that don't have it?

Opened an MR with a slightly different approach.

The existing patch will add an ID to every since block that is added via LB, which feels a bit overkill to solve this. Instead I've added a default ID that will be applied to system_menu_blocks if one does not exist. If people like this approach I'll add some tests

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Seems a few other block--system-menu-block.html.twig in core. Is there a place we can add an assertion to?

This appears no longer to be an issue as of 11.3.x. Placing a menu block on the page successfully generates unique ID attributes; placing a second instance of the same menu block confirms that the ID attributes and corresponding aria-labelledby attributes are unique. I haven't tracked down the core commit responsible for this yet, but will set this to "Postponed" given the above.

Example markup of first instance of menu block:

<nav id="block-olivero-mainnavigation" class="contextual-region primary-nav block block-menu navigation menu--main" aria-labelledby="block-olivero-mainnavigation-menu" role="navigation">
      
  <h2 class="block__title" id="block-olivero-mainnavigation-menu">Main navigation</h2>

Example markup of second instance of menu block:

<nav id="block-olivero-mainnavigation-2" class="contextual-region primary-nav block block-menu navigation menu--main" aria-labelledby="block-olivero-mainnavigation-2-menu" role="navigation">
      
  <h2 class="block__title" id="block-olivero-mainnavigation-2-menu">Main navigation</h2>

My previous comment was wrong. This is still an issue in Drupal core, though it only manifests in the context of placing a menu block using Layout Builder.

Seems a few other block--system-menu-block.html.twig in core. Is there a place we can add an assertion to?

Agreed that modifying the Twig templates is not the best approach here. Instead, we can supply the ID attribute to all templates through the SystemMenuBlock::build() method. The rendering logic for a unique ID can be modeled exactly on block/src/Hook/BlockHooks::preprocessBlock(), per below:

    // Create a valid HTML ID and make sure it is unique.
    if (!empty($variables['elements']['#id'])) {
      $variables['attributes']['id'] = Html::getUniqueId('block-' . $variables['elements']['#id']);
    }

I've created an alternate merge request, at https://git.drupalcode.org/project/drupal/-/merge_requests/12664

The latest patch appears to work. This is a screenshot of the code output when two unique menu blocks are inserted on the same page via layout builder.

Nice! Glad we found a non template approach.

Moving to NW for the test coverage also issue summary may need some love to include proposed solution and maybe screenshots.

Thanks!

+1 on the approach from the latest MR. Looks like the newly added test coverage is failing to find the newly added ID though. I expect it will be a quick fix.

Updated to accommodate tests! Ready for further review.

Nice. Given it a (hopefully) last review. No issues, reckon this can be RTBC.

I'm not sure the fix is appropriate.

IDs are not a backend concern, so it seems strange to handle that in the backend when the frontend can deal with it, we try not to rely on ids for styling and JS so the id could be random and it'd still work.

The MR https://git.drupalcode.org/project/drupal/-/merge_requests/6965 looks promising need to use |clean_unique_id to make sure things don't break when you're dealing with ajax stuff.

we try not to rely on ids for styling and JS so the id could be random and it'd still work.

Indeed, separate from styling and JS targets, HTML ID attributes, especially, as in this case, when associated with HTML heading tags, are used as in-page anchor links. Having a "permalink" through a consistent HTML ID, therefore, is preferable to a random one.

The approach in https://git.drupalcode.org/project/drupal/-/merge_requests/6965 would seem to provide a consistent ID, so if the Drupal core practice is to register HTML IDs in Twig templates, then MR 6965 would seem to be the right approach.

The disadvantage, for the sake of stating it explicitly, of updating (multiple) core Twig templates is that any sites that have defined their own block--system-menu-block templates will not receive this change. In my experience in Drupal development, the menu block template is frequently modified on a per site basis.

I updated https://git.drupalcode.org/project/drupal/-/merge_requests/6965 to use |clean_unique_id twig filter in the block and updated test coverage.

Original concern of updating the the twig templates directly is back. We will need a CR as contrib and custom themes won't have that change.

If we are going to go the template route think we need a CR to least alert others that have overridden this template.

Hello,
I wanted to warn that https://git.drupalcode.org/project/drupal/-/merge_requests/6965 is acting only on the heading_id (which is used to "label" the navigation via aria-labelledby), but it's not acting on the block id itself (attributes.id).
It seems to me that https://git.drupalcode.org/project/drupal/-/merge_requests/12806 is acting at the right place, before Twig rendering, and really on the block id itself : I agree with https://www.drupal.org/project/drupal/issues/3073895#comment-16254500 about the necessary separating of concerns.