Add accessible names to theme region complementary landmarks

Ooh, I forgot I filed this!

Thinking about it, this should probably be classed as a bug report, on the basis that the W3C ARIA Authoring Practices says "Naming is necessary when two complementary landmark regions are present on the same page" (emphasisi mine).

This might be a somewhat disruptive for a patch release, depending on how we implement it. We'll see.

We could split this into child issues for each theme if needs be.

minor issue summary updates

I looked over all the variants of page.html.twig in core.

ALL of the complementary landmarks are hard-coded HTML elements with no dynamic twig variables (e.g. attributes) in the tag. This example comes from Bartik, and is typical:

        <aside class="layout-container section clearfix" role="complementary">

This is good news for backwards compatibility - the fact that none of these instances have any twig variables in the tag means that the only way this could have been fixed in an existing website was by overriding (or hacking) the Twig template. I think that means we can introduce an aria-label="First sidebar" attribute without worrying about disrupting existing sites. That includes the Classy, Stable, and Stable9 themes; non-disruptive accessibility fixes have been permitted in these templates in the past.

So the only outstanding question is to figure out what to call a few of the regions.

Here's the full list of complementary landmark regions in theme templates. Output of ack --type=twig --sort-files "complementary":

core/modules/system/templates/install-page.html.twig
38:      <aside class="layout-sidebar-first" role="complementary">
44:      <aside class="layout-sidebar-second" role="complementary">

core/modules/system/templates/maintenance-page.html.twig
45:  <aside role="complementary">
51:  <aside role="complementary">

core/modules/system/templates/page.html.twig
71:      <aside class="layout-sidebar-first" role="complementary">
77:      <aside class="layout-sidebar-second" role="complementary">

core/profiles/demo_umami/themes/umami/templates/classy/layout/maintenance-page.html.twig
48:    <aside class="layout-sidebar-first" role="complementary">
54:    <aside class="layout-sidebar-second" role="complementary">

core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig
115:      <aside class="layout-sidebar" role="complementary">

core/themes/bartik/templates/page.html.twig
64:        <aside class="layout-container section clearfix" role="complementary">
71:        <aside class="featured-top__inner section layout-container clearfix" role="complementary">
87:            <aside class="section" role="complementary">
94:            <aside class="section" role="complementary">
103:        <aside class="layout-container clearfix" role="complementary">

core/themes/claro/templates/install-page.html.twig
26:    <aside class="layout-sidebar-first" role="complementary">
40:    <aside class="layout-sidebar-second" role="complementary">

core/themes/claro/templates/maintenance-page.html.twig
21:    <aside class="layout-sidebar-first" role="complementary">

core/themes/classy/templates/layout/maintenance-page.html.twig
48:    <aside class="layout-sidebar-first" role="complementary">
54:    <aside class="layout-sidebar-second" role="complementary">

core/themes/classy/templates/layout/page.html.twig
68:      <aside class="layout-sidebar-first" role="complementary">
74:      <aside class="layout-sidebar-second" role="complementary">

core/themes/seven/templates/install-page.html.twig
26:    <aside class="layout-sidebar-first" role="complementary">
40:    <aside class="layout-sidebar-second" role="complementary">

core/themes/seven/templates/maintenance-page.html.twig
21:    <aside class="layout-sidebar-first" role="complementary">

core/themes/stable/templates/layout/install-page.html.twig
36:      <aside class="layout-sidebar-first" role="complementary">
42:      <aside class="layout-sidebar-second" role="complementary">

core/themes/stable/templates/layout/maintenance-page.html.twig
43:  <aside role="complementary">
49:  <aside role="complementary">

core/themes/stable/templates/layout/page.html.twig
69:      <aside class="layout-sidebar-first" role="complementary">
75:      <aside class="layout-sidebar-second" role="complementary">

core/themes/stable9/templates/layout/install-page.html.twig
36:      <aside class="layout-sidebar-first" role="complementary">
42:      <aside class="layout-sidebar-second" role="complementary">

core/themes/stable9/templates/layout/maintenance-page.html.twig
43:  <aside role="complementary">
49:  <aside role="complementary">

core/themes/stable9/templates/layout/page.html.twig
69:      <aside class="layout-sidebar-first" role="complementary">
75:      <aside class="layout-sidebar-second" role="complementary">

@maxstarkenburg - thanks for adding your insights here. It was a productive Slack chat.

Minor point, but the in Bartik for featured_bottom_... regions at least wraps all three in one, so a default for that particular one could at least presumably be a simpler "Featured bottom".

I think this pretty much solves that problem! I noticed the same thing when I did the survey in #5.

The discussion about the "first sidebar" and "second sidebar" names was very useful. I think it's definitely worth putting the conditional names in place to avoid this confusion. Bartik already has some code which adds some CSS classes based on the number of sidebars present, so a similar approach should be achievable for the accessible names of the landmark regions.

As Andrew also mentioned in Slack, sites in RTL languages might also add a wrinkle (though maybe other parts of core are already doing a good enough job of not assuming sidebar-first is necessarily on the left?).

Yeah, my idea was to check the language direction so they can be conditionally named "left sidebar" and "right sidebar". I'm less convinced by that idea though, because the sidebars are moved below the main content on narrow viewports. Let's park that idea.

Updating the proposed resolution. Crediting @maxstarkenburg.

Thanks for working on this everybody. I've skim-read the latest patch, but I haven't manually tested each theme yet. I'll give it some manual testing soon.

Partial review of patch #30:

1. I really like the simplicity of this approach: aria-label="{{ page.sidebar_first ? 'Second Sidebar'|t : 'Sidebar'|t }}".

2. 
   diff --git a/core/modules/system/templates/page.html.twig b/core/modules/system/templates/page.html.twig
   index 032b724dfe..701758ad9b 100644
   --- a/core/modules/system/templates/page.html.twig
   +++ b/core/modules/system/templates/page.html.twig
   @@ -68,13 +68,13 @@
        </div>{# /.layout-content #}
   
        {% if page.sidebar_first %}
   -      <aside class="layout-sidebar-first" role="complementary">
   +      <aside class="layout-sidebar-first" role="complementary" aria-label="{{ page.sidebar_second ? 'First Sidebar'|t : 'Sidebar'|t }}}">>

   There's a Twig syntax error in this instance. Notice there are 3 curly braces (}}}) at the end of the aria-label.

3. diff --git a/core/modules/system/templates/maintenance-page.html.twig b/core/modules/system/templates/maintenance-page.html.twig
   index 748ed5a3aa..781e611d80 100644
   --- a/core/modules/system/templates/maintenance-page.html.twig
   +++ b/core/modules/system/templates/maintenance-page.html.twig
   @@ -42,13 +42,13 @@
    </main>
   
    {% if page.sidebar_first %}
   -  <aside role="complementary">
   +  <aside role="complementary" aria-label="{{ page.sidebar_second ? 'First Sidebar'|t : 'Sidebar'|t }}">>

   There's a HTML syntax error here. Notice the 2 closing angle brackets (>>). This is repeated several times in the patch.

Re. #12:

2. classy will be removed from Drupal 9. So think there's no need to do the changes.

Classy hasn't been removed from D9, so the changes are needed there too. (It looks like Classy won't be removed until D10 now - see #3110137: Remove Classy from core and #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility).

Any website which uses the stable or classy themes can automatically benefit from this change, but themes which have overridden the page template won't.

A change record would be good for this issue, to advise themers and distro maintainers to do something similar in their own page templates.

Thanks for working on this @sja112.

I looked at ConfirmClassyCopiesTest to see what we need to do, and I understand it now. Here's a summary of what it does:

• The getClassyHash() helper method has a big list of MD5 checksums for all of the templates in the Classy theme. We're expecting to see these in the following tests.
• The first test (testClassyHashes()) calculates the MD5 checksum for each template in the Classy theme, and confirms it is the expected value from the helper method. If we change a template from the Classy theme (for example maintenance-page.html.twig) then this test will fail.
• The second test (testClassyCopies()) calculates the MD5 checksum for templates in other themes, which are supposed to be exact copies of the template in Classy. If we change the maintenance-page.html.twig template from Umami, this test will fail.
• When both of these tests pass, it means the maintenance-page.html.twig templates in Classy and Umami are identical.

So, if we want to make changes to templates in Classy, we have to update the expected MD5 checksum stored in the ConfirmClassyCopiesTest::getClassyHash(), and make sure exactly the same changes are made to the Classy copies in the other core themes.

The affected templates are these ones. In this patch, I've updated the templates and the MD5 hashes stored in the test.

$ ack  -l --type=twig --sort-files "complementary" | grep classy
core/profiles/demo_umami/themes/umami/templates/classy/layout/maintenance-page.html.twig
core/themes/classy/templates/layout/maintenance-page.html.twig
core/themes/classy/templates/layout/page.html.twig

I did some other remaining work. These page templates have 2 sidebars, but didn't have the conditional name check yet.

• core/themes/claro/templates/install-page.html.twig
• core/themes/seven/templates/install-page.html.twig
• core/themes/stable/templates/layout/install-page.html.twig
• core/themes/stable/templates/layout/maintenance-page.html.twig
• core/themes/stable/templates/layout/page.html.twig
• core/themes/stable9/templates/layout/install-page.html.twig
• core/themes/stable9/templates/layout/maintenance-page.html.twig
• core/themes/stable9/templates/layout/page.html.twig

A good issue for the Accessibility Bug Bash during DrupalCon Global 2020.

You get sidebars to appear by placing blocks in them. The templates check whether there is anything in the sidebar, before creating the HTML wrapper.