- Status changed to Needs work
over 2 years ago 9:22pm 13 February 2023 - πΊπΈUnited States smustgrave
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?
- First commit to issue fork.
- Status changed to Needs review
over 1 year ago 6:14am 8 March 2024 - π³πΏNew Zealand danielveza Brisbane, AU
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
- Status changed to Needs work
over 1 year ago 6:24am 8 March 2024 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.)
- Status changed to Needs review
over 1 year ago 6:34am 25 March 2024 - Status changed to Needs work
over 1 year ago 1:34pm 25 March 2024 - πΊπΈUnited States smustgrave
Seems a few other block--system-menu-block.html.twig in core. Is there a place we can add an assertion to?
- First commit to issue fork.
- πΊπΈUnited States mark_fullmer Tucson
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>
- Merge request !12664Issue #3073895 by mark_fullmer, tim.plunkett: System Menu blocks do not have attributes β (Open) created by mark_fullmer
- πΊπΈUnited States mark_fullmer Tucson
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.
- πΊπΈUnited States mark_fullmer Tucson
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 onblock/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