Broken aria-labelledby ID reference in menu block and breadcrumb templates

@jo.st Thanks for the patch and steps-to-reproduce.

This problem also affects the equivalent template from system.module, and you can replicate the problem using the Stark theme. Running the Chrome #a11y audits with the Stark theme tells us that this problem affects all 4 menu blocks (on homepage, after a standard install, then enabling Stark).

It looks like this was broken by #2407715: Remove classes from system templates b*.html.twig. That purpose of that issue was to remove some classes but it stripped some ID attributes out as well. The stable theme just inherited the problem when the templates were copied from the core modules just before the 8.0.0 release.

git show af53fb6 says another nav heading ID was stripped from the breadcrumb template at the same time.

The patch in comment #1 is the right approach, however we should also fix it in the core system.module templates. Affected templates:

• core/modules/system/templates/block--system-menu-block.html.twig
• core/modules/system/templates/breadcrumb.html.twig
• core/themes/stable/templates/block/block--system-menu-block.html.twig
• core/themes/stable/templates/block/breadcrumb.html.twig

Updating the title as it affects several templates.

The bug should be fixed in the system.module.

Tagging so stable theme maintainer still sees it. The stable theme templates aren't supposed to change, however this bug might qualify. We're restoring some functional (non-styling) ID attributes that shouldn't have been removed.

This is a bug fix, so eligible for 8.1.x I think.

Thanks for the tag @andrewmacpherson I agree this seems like the type of thing we would want to fix in Stable and I see little to no risk in making this change.

@cottser Great, thanks for confirming this.

@jo.st

Good news - we have sign-off from the maintainer of the stable theme.

The patches from #7 and #8 should be combined - can you prepare a new patch?

Combined #7 and #8 patch.

Thanks!!

Looks good! The patch from #13 fixes the correct templates, and produces the expected markup with the heading ID attributes restored, and the aria-labelledby references now work as expected.

For testing, I used the latest versions of Firefox and NVDA screenreader on Windows 7. I enabled the Stark theme, which uses the templates from the system module.

NVDA provides a tool to browse ARIA landmark roles. Here are some screenshots of the landmark information before and after the patch from comment #13.

Before the patch, NVDA knows about the landmark roles, but they don't have distinct labels. There are 4 on the page, but all are simply listed as "navigation".

After the patch, NVDA reports the landmarks with their associated labels. The 4 navigation landmarks each have a different label: Main navigation, User Account Menu, Breadcrumb, and Footer menu.

Note that both of these screenshots show some other un-labelled landmark regions: banner, main, complementary. This is expected, as these landmarks don't have any aria-labbelledby attributes. This issue is just about fixing the broken ID references for the navigation landmarks which do have aria-labelledby attributes.

I also tested these with the latest ChromeVox screen reader, and it's working fine there too.

Good work! This is a nice win for screen reader / browser combinations which support this feature.

@cottser,

The patch #13 paased in php 5.6 mysql 5.5

Please review.

Thanks!!

Tests are green again, restoring RTBC.

HI @cottser,

Can you please find the update patch,

Thanks,

Re #21: This patch is a regression from the patch in #13.

I've attached an interdiff to show what changed between #13 and #21...

1. The breadcrumb navigation landmark no longer gets a label, because the ID referenced by aria-labelledby is not found. This is the very problem we are trying to fix in this issue. Here is a screenshot of the landmark regions list from NVDA screen reader, after applying the patch from #21. There are 4 navigation landmarks, but only 3 have labels.

   

   Compare this with the earlier screenshot (in comment #15) which shows that the breadcrumb landmark label was fixed by patch #13.

2. The patch in #21 also introduces some whitespace around the dot in title_attributes . setAttribute, which we don't seem to use for any other twig variables.

The patch in #13 was previously RTBC, and previously all test were passing. We get intermittent test failures, unrelated to the changes in the patch. (I don't really understand why, but it usually involves the tests for the hook_update_N mechanism. It varies from day to day).

I've queued another test for the patch in #13.

Another thing to note. The screenshots I included use the Stark theme, which demonstrates that the templates from system.module have been fixed.

I have also been testing this manually with the Stable theme, which we are also fixing. Although it is a "hidden" theme which cannot be enabled via admin/appearance, it can be enabled via drush config-edit system.theme. I'm happy that the patch from comment #13 fixes the stable theme templates.

The tests are currently green :-)

Restoring RTBC for the patch in comment #13 (not the patch from #21)

For the patch provided in #21,
Even the id is different.

Committing to 8.2.x only for now since it's a markup change. (https://www.drupal.org/core/d8-allowed-changes#minor)

Committed 551dcc0 and pushed to 8.2.x. Thanks!

Hi @Cottser ,

Thanks for commiting the patch.
But I was not credited for this issue.May I know the reason why?

Thanks,

@pashupathi nath gajawada:

In short I don't think #21 moved the issue forward and that's what I usually have in mind when determining who should get credit: did this person move the issue forward? Unfortunately I don't think your comments/patch here moved the issue forward so I made the decision not to give you an issue credit. Sometimes we'll give leeway when we can see someone is really trying but I didn't get that impression here based on what I can see. We also try not to enable those who might try to game the issue credit system. In more detail:

1. After looking at the patch both @andrewmacpherson and myself could only see things being broken, not fixed. Both coding standards and functionality.
2. There was no explanation given for the changes. This makes it really hard to tell what your intention was in uploading that patch. I understand language can be a barrier but any attempted explanation is better than nothing.

For the above reasons your patch and comment could be interpreted as an attempt to get an issue credit with little to no effort, similar to re-uploading a patch for no reason. I sincerely hope that's not the case here and I also hope that if you're just trying to learn you can continue to do so in other issues. A few lessons you might learn from this issue:

1. Always write a comment to summarize what your patch does and/or what it's changing.
2. Get in the habit of uploading interdiffs so that others can easily see what you are changing (@andrewmacpherson helpfully generated one in #22).
3. Test the code yourself whenever possible, the issue summary of this issue describes how to test it.

@pashupathi nath gajawada I just went and looked at some of the other issues you've worked on recently and your habits there seem to be better than what is shown here. So please continue to do what you've been doing in the other issues where you're describing the changes you're incorporating. Thanks!

I'm sorry for that @cottser,

That was not my intention.

Sure and thanks for your advice's, I would make sure I would follow the 3 steps which you have suggested while posting.

Thanks,