Coming from #2045039-7: Cleanup HTML heading structure @terrill:

That said, I still think we should add aria-label attributes to all elements. There are many of these on each page, and without a label they're each identified by screen readers as "Navigation region" - there's no means of clarifying which navigation region is which. If we add aria-label, screen readers use the value of aria-label when identifying the region (e.g., "Toolbar Items Navigation Region", "Breadcrumb Navigation Region", and the like)

@falcon03 and later on:

Well, I think that hearing messages like "entering navigation", "entering main" and so on doesn't give a real "context". So, as a blind user, I'm completely in favor of adding aria-labels.

@pratikp1 and finally:

Having an effective label is essential. I would like some discussion about that for D8 or D9. Let's see if we can make it more understandable. It will go a long way toward having users understand that landmarks can actually be useful.

I don't know why there isn't any in Seven, but in Bartik, we can find the <nav> element in:
core/themes/bartik/templates/comment.html.twig
core/themes/bartik/templates/page.html.twig

And in modules it's in:
core/modules/book/templates/book-all-books-block.html.twig
core/modules/book/templates/book-navigation.html.twig
core/modules/system/templates/breadcrumb.html.twig
core/modules/system/templates/page.html.twig
core/modules/toolbar/toolbar.module

Where else do we find them?

Some more info on using aria-labels:
http://www.last-child.com/debugging-aria-label/

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Yes Instructions
Manually test the patch Yes Instructions
Add automated tests Instructions
CommentFileSizeAuthor
#84 2052473-84.patch803 bytesrpayanm
#76 interdiff-2052473-63-76.txt1.31 KBRade
#76 2052473-76.patch803 bytesRade
#63 2052473-63.patch855 bytesmgifford
#54 Book-nav-aria.png57.12 KBmgifford
#54 Toolbar-nav-aria.png102.79 KBmgifford
#54 Secondary-nav-aria.png112.99 KBmgifford
#54 Tabs-nav-aria.png62.83 KBmgifford
#54 Breadcrumb-nav-aria.png58.73 KBmgifford
#53 2052473-53.patch8.39 KBricky.middaugh
#50 2052473-50.patch753 bytesricky.middaugh
#45 2052473-45.patch8.19 KBricky.middaugh
#43 2052473-43.patch7.74 KBricky.middaugh
#39 2052473-39.patch8.38 KBmgifford
#37 Label-Toolbar.png116.1 KBmgifford
#37 Labelledby-2ndary-Navigation.png101.33 KBmgifford
#37 Labelledby-Breadcrumb.png80.38 KBmgifford
#37 Labelledby-Book-Nav.png101.05 KBmgifford
#37 Label-Book-Navigation.png97.75 KBmgifford
#36 2052473-36.patch8.39 KBlokapujya
#36 interdiff.txt673 byteslokapujya
#34 aria_labels-2052473-34.patch8.38 KBJalandhar
#30 aria_labels-2052473-30.patch8.38 KBmgifford
#28 aria_labels-2052473-27-do-not-test_0.patch7.11 KBmgifford
#26 aria_labels-2052473-26-do-not-test_0.patch7.09 KBmgifford
#25 aria_labels-2052473-24-do-not-test_0.patch6.36 KBmgifford
#20 interdiff.txt5.93 KBjessebeach
#20 aria_labels-2052473-12-do-not-test.patch6.51 KBjessebeach
#12 aria_labels-2052473-12.patch2.93 KBBarisW
#12 interdiff-2-12.txt615 bytesBarisW
#10 Screen Shot 2013-09-18 at 11.18.56 PM.png163.4 KBmgifford
#10 Screen Shot 2013-09-18 at 11.24.14 PM.png170.33 KBmgifford
#2 aria_labels-first_attempt.patch2.93 KBfalcon03
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

falcon03’s picture

Assigned: Unassigned » falcon03
Priority: Normal » Major

So basically, we want to get a result like this: http://www.qcsalon.net

Ok, I admit that it is not a *really* technical website, but it offer a *stellar* user experience for a screen reader user by combining *old, traditional* techniques with the more recent ARIA ones.

I guess that if all the theme_functions->twig conversions & twig templates consolidation had been completed this task would have been easier. Anyways, let me give this a stab! :-)

As a screen reader user our current situation could be a pain in production; adjusting priority accordingly! :-)

falcon03’s picture

Status: Active » Needs review
FileSize
2.93 KB

Here's a first stab at it (untested patch). In this comment you will find some remarks, let's get the discussion about them going! :-)

Regarding core/themes/bartik/templates/comment.html.twig: it uses a "nag" to wrap comment links, but the core-provided template does not. This is something to fix -- we have to avoid inconsistencies! But then the thing is -- What could an appropriate label be for the comment links? It is neither printed inside an heading, so maybe the better solution would be to completely remove the wrapper around comment links.

In the breadcrumb template, "You are here" in the heading makes sense, but does it make sense using it as a label as well? If not, we should use another string -- But I have no ideas! :-)

In core/modules/system/templates/page.html.twig the nag element is a wrapper for the main and the secondary menu. Maybe we'd better want to have a single nag for each menu. I'd vote for removing the nag container as it is now, since it is useless and print out each of both menus using a container. Also, tabs and action links are not wrapped in a container, but they should be -- Can we fix both these issues in this patch or do we need to split them in follow ups?

I'm inclined to deal with the toolbar in [1800614]. Are you fine with that?

I'm still convinced that theme_functions->Twig conversions and twig templates consolidation will reveal many other places to add aria-labels to and some other inconsistencies!

falcon03’s picture

Issue tags: +d8ux, +Bartik HTML5, +Twig

Tagging -- we strongly need themers feedback on this issue! :-)

falcon03’s picture

Issue tags: -Bartik HTML5, -Twig +html5, +drupal-at-twig

fixing tag.

falcon03’s picture

Issue tags: -html5, -drupal-at-twig +Bartik HTML5, +Twig

I can't stand d.o tags autocompletion any longer. Seriously! :-)

falcon03’s picture

Issue tags: -Bartik HTML5 +html5
jessebeach’s picture

As far as code goes, it looks great.

bowersox’s picture

The Drupal 8 accessibility testing sessions in Minneapolis proved that this is important--users were confused that Drupal has so many role=Navigation regions, and it is not clear which is which. So thanks for the work on this important issue!

falcon03’s picture

@bowersox: yeah, I really agree -- we absolutely need to solve this issue before releasing D8. I changed priority to major, but feel free to set it to critical if you want to. :-)

I can finish the patch easily, but I need feedback for remarks in #2 first. :-)

mgifford’s picture

These are numbered responses from #2 above. I've quoted them for simplicity.

1) Here's a first stab at it (untested patch).
It seems to be working where I have tested it. I've added screen shots.

2) Regarding core/themes/bartik/templates/comment.html.twig: it uses a "nag" to wrap comment links, but the core-provided template does not. This is something to fix -- we have to avoid inconsistencies! But then the thing is -- What could an appropriate label be for the comment links? It is neither printed inside an heading, so maybe the better solution would be to completely remove the wrapper around comment links.
I'm for simplifying things if we can. If we are just repeating text, I don't know that it really is going to be adding much if any context by adding the aria-label. Wouldn't it just get annoying to hear them twice all the time?

3) In the breadcrumb template, "You are here" in the heading makes sense, but does it make sense using it as a label as well? If not, we should use another string

Can we skip it in that case? We don't want to remove the heading as that is used for navigation, but is there a good reason to paraphrase or duplicate it everywhere? If so maybe we have to dig a bit deeper to figure out what the function of these links is. Alternatively, maybe some of these will have to be defined per-site and we should simply make it easier for the developer to add them after the fact.

4) In core/modules/system/templates/page.html.twig the nag element is a wrapper for the main and the secondary menu. Maybe we'd better want to have a single nag for each menu. I'd vote for removing the nag container as it is now, since it is useless and print out each of both menus using a container. Also, tabs and action links are not wrapped in a container, but they should be -- Can we fix both these issues in this patch or do we need to split them in follow ups?
I'm in favour of anything we can do to make it simpler.

5) I'm inclined to deal with the toolbar in #1800614: Improve the responsive toolbar accessibility. Are you fine with that?
Yes. I noted that on the other issue too.

6) I'm still convinced that theme_functions->Twig conversions and twig templates consolidation will reveal many other places to add aria-labels to and some other inconsistencies!
Agreed. It's all going to have to be reviewed many more times I'm afraid.

So far, pulling out the proposed changes we have:

+  <nav id="book-block-menu-{{ book_id }}" class="book-block-menu" aria-label="{{ 'Book outlines'|t }}">
+  <nav id="book-navigation-{{ book_id }}" class="book-navigation" aria-label="{{ 'Book Navigation'|t }}">
+  <nav class="breadcrumb" role="navigation" aria-label="{{ 'You are here'|t }}">
+      <nav id="secondary-menu" class="navigation" role="navigation" aria-label="{{ 'Secondary menu'|t }}">
+      <nav id ="main-menu" class="navigation" role="navigation" aria-label="{{ 'Main menu'|t }}">
+          <nav class="tabs" role="navigation" aria-label="{{ 'Tabs'|t }}">

Home Page Navigation

Book Page Navigation

agileadam’s picture

For what it's worth, you can add these attributes using preprocess functions as necessary.
For example, my nav elements are typically menu_block and nice_menus blocks.
Here's how I add the aria-label attributes to these blocks.

/**
 * Override or insert variables into the block templates.
 */
function custom_preprocess_block(&$vars) {
  if (isset($vars['block']->bid)) {
    switch ($vars['block']->bid) {
      case 'nice_menus-1':
        $vars['attributes_array']['aria-label'][] = 'Utility Navigation';
        break;
      case 'tb_megamenu-main-menu':
        $vars['attributes_array']['aria-label'][] = 'Primary Navigation';
        break;
      case 'menu_block-1':
        $vars['attributes_array']['aria-label'][] = 'Tertiary Main Navigation';
        break;
      case 'menu_block-2':
        $vars['attributes_array']['aria-label'][] = 'Secondary Navigation';
        break;
    }
  }
}

You can do similar for other elements in themename_preprocess_page() and elsewhere. I set a breadcrumb aria-label in my mytheme_breadcrumb() override function by modifying the HTML within that function.

Anyways, sorry to muddy-up this issue; I just figured other people would land here looking to do similar.

BarisW’s picture

I really like this idea. The patch in #2 works fine, I've only changed made the casing of the first and second words equal (like Book navigation instead of Book Navigation).

mgifford’s picture

@BarisW what do you think of my response to @falcon03's points in #2. Where else do we need more clarification than what I provided in comment #10?

BarisW’s picture

Apologies, haven't read you comment properly. This issue needs an updated description ;)
I'll come back to this issue later, I agree with most of your points.

BarisW’s picture

Ok, so having read this all over, I believe we make a mistake. As far as I understand from the specs, the aria-label should be used to give information to a section in cases when there's no visible label. The discussion here is the meaning of 'visible'. We use the 'visually-hidden' class in favor of the 'element-invisible' class exactly for this reason already; so that screen readers will still read out the labels.

In the example of the OP, the only places where aria-labels were added are the regions, like Main Content, or places where the list (breadcrumbs) didn't have a heading already.

I think that adding labels on places were we already output headings would only end up in having speach readers repeating themselves. Better would be to use the aria-labelledby attribute, but that would result in having to add heading ID's all over the place.

Would it work if we add the aria-labels like in the current patch, but replace the visually-hidden with element-invisible again? So that speach readers will only read out the aria-label element?

So we need some input from Everett?

mgifford’s picture

We definitely don't need to wait for Everett (as much as I do really value his insights)..

But let's see if we can get some real world experience chipping in on how to apply this properly so that it works right! I've reached out to a few folks in the accessibility community and hope to get some input.

mgifford’s picture

#12: aria_labels-2052473-12.patch queued for re-testing.

mgifford’s picture

I think adding aria-labelledby tags to heading tags with ID's would be acceptable. We'd just need to build a strong case for it. I think that would be probably better than the duplication.

jessebeach’s picture

3) In the breadcrumb template, "You are here" in the heading makes sense, but does it make sense using it as a label as well? If not, we should use another string

Using a string like this results in the output: "You are here navigation". It strikes me that we should be more explicit about what this navigation element is. Maybe just Breadcrumb resulting in: "Breadcrumb navigation".

The way I've been approaching these labels is to think "given an out-of-context list of landmarks, what kind of label will inform me quickly about the purpose of this region? What label will make me understand if I need to click into it or just ignore it given my goal at the moment?"

jessebeach’s picture

I'd really like to see us be as specific as possible with labels. If something has a title, then that title should be used in the label or heading. The label "Secondary menu" is not very helpful. A label like "Account actions" is helpful because it describes what types of links are might be found in the menu. So, when we have something like this:

<nav id="secondary-menu" class="navigation" role="navigation" aria-label="Secondary menu">
  <h2 class="visually-hidden">Secondary menu</h2>
  <ul id="secondary-menu-links" class="links inline clearfix">
    <li class="menu-3 odd first"><a href="/user">My account</a></li>
    <li class="menu-14 even last"><a href="/user/logout">Log out</a></li>
  </ul>
</nav>

The utility of the label is very low. I'm not criticizing the work that has been done already. No no. What I'm doing is challenging us to make these labels really useful; I don't want to simply be checking off accessibility boxes here. Let's make great interfaces!

So instead, what we really want is HTML like this:

<nav id="secondary-menu" class="navigation" role="navigation" aria-labelledby="secondary-menu-header">
  <h2 id="secondary-menu-header" class="visually-hidden">Account actions</h2>
  <ul id="secondary-menu-links" class="links inline clearfix">
    <li class="menu-3 odd first"><a href="/user">My account</a></li>
    <li class="menu-14 even last"><a href="/user/logout">Log out</a></li>
  </ul>
</nav>

The label pertains to the particular menu being printed here.

Can we do this? Yes, I think we can. I've gone through the Book module and made similar changes in the attached patch.

The goal we're aim for is, can someone reasonably understand the landmark labels absent of a specific content? Enough to choose between them with some degree of confidence?

This patch also contains a small fix, represented in #2124287: Book module's BookNavigationBlock passes poorly formatted variable data to the book-all-books-block template, so when that gets committed, we'll have a tiny conflict to resolve.

jessebeach’s picture

Status: Needs review » Needs work

This patch still needs work. Let's make it awesome!

jessebeach’s picture

Category: feature » task

This also is no feature request. This is UI work. Non-visual UI work.

jessebeach’s picture

from #15:

I think that adding labels on places were we already output headings would only end up in having speach readers repeating themselves.

My experience using VoiceOver is that headings and aria labels are two parallel systems. Either one is navigating by headings or one is navigating by landmarks. I don't run into duplicated output. I do prefer to refer to the same strings, so where we can, we should be using aria-labelelledby so that strings don't diverge over time.

mgifford’s picture

This is really great Jesse, thanks. I really like where this is going.

I was looking at the new Drupal.org and wondered if it might be possible to apply some of these ideas to the theme there so that we can get more feedback from users in a real world environment. Not that we should hold off on that, but it would be a great case study.

I like the use of aria-labelelledby, simply so that there is less translation required. The only concern was the re-introduction of ID's.

mgifford’s picture

Patch in #20 didn't apply any longer.

mgifford’s picture

The semantic relationship between the title and the navigation should be established by default if it exists. I think this will work if inersted at the bottom of template_preprocess_block().

  // Add aria-describedby if possible to improve accessibility.
  if ($variables['block']->subject && !empty($variables['attributes']['role'])) {
    $variables['title_attributes']['id'] = drupal_html_id($variables['block']->subject);
    $variables['attributes']['aria-describedby'] =  $variables['title_attributes_array']['id'];
  }
mgifford’s picture

Ok, this needs more thought... Can't just move it over from AdaptiveTheme as I'd hoped.

Notice: Undefined index: block in template_preprocess_block() (line 535 of core/modules/block/block.module).
Notice: Trying to get property of non-object in template_preprocess_block() (line 535 of core/modules/block/block.module).

mgifford’s picture

Sorry for the noise.. Think I needed to use $variables['elements']['#block']

mgifford’s picture

Nope.. Bit closer, but i have to get my laptop back from the shop so I can test this easier....

Notice: Undefined property: Drupal\block\Entity\Block::$subject in template_preprocess_block() (line 535 of core/modules/block/block.module).

mgifford’s picture

Status: Needs work » Needs review
FileSize
8.38 KB

I'd like to use to more aria-labelledby simply so that there is less redundant code and less processing.

Anyways, this patch addresses the notice error. I haven't found an example where my efforts to proactively add aria-describedby where possible actually worked mind you. There's been a real effort to remove ID's and so lots of places to add them back in if we're going to use this approach.

mimes’s picture

30: aria_labels-2052473-30.patch queued for re-testing.

mgifford’s picture

30: aria_labels-2052473-30.patch queued for re-testing.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Novice
Jalandhar’s picture

Assigned: falcon03 » Unassigned
Status: Needs work » Needs review
FileSize
8.38 KB

Updating patch with reroll. Please review.

Status: Needs review » Needs work

The last submitted patch, 34: aria_labels-2052473-34.patch, failed testing.

lokapujya’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
97.75 KB
101.05 KB
80.38 KB
101.33 KB
116.1 KB

Patch looks nice. The bot likes it too. There are no UI changes. I attached some screen shots.

jessebeach’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/block/block.module
@@ -402,6 +402,13 @@ function template_preprocess_block(&$variables) {
   }
+
+  // Proactively add aria-describedby if possible to improve accessibility.
+  if (isset($variables['elements']['#block']->subject) && isset($variables['attributes']['role'])) {
+    $variables['title_attributes']['id'] = drupal_html_id($variables['elements']['#block']->subject);
+    $variables['attributes']['aria-describedby'] =  $variables['title_attributes_array']['id'];
+  }
+
 }

Could someone quickly explain why the key in the $variables array is 'title_attributes' on one line and then 'title_attributes_array' on the next line. Is this some kind of $variables array magic?

mgifford’s picture

FileSize
8.38 KB

@jessebeach - I'd say it was definitely troll magic....

Thanks for catching that. Here's a patch with the correction.

LewisNyman’s picture

Issue tags: +frontend
dcam’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Updating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....

#39 does not apply to HEAD and need a reroll.

ricky.middaugh’s picture

Assigned: Unassigned » ricky.middaugh

Will reroll the latest patch.

ricky.middaugh’s picture

Assigned: ricky.middaugh » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.74 KB

Here's the rerolled patch.

Status: Needs review » Needs work

The last submitted patch, 43: 2052473-43.patch, failed testing.

ricky.middaugh’s picture

FileSize
8.19 KB

It looks like the problem had to do with missing a node title when trying to generate a block for the current book.

This should fix the failing tests.

ricky.middaugh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 45: 2052473-45.patch, failed testing.

ricky.middaugh’s picture

Assigned: Unassigned » ricky.middaugh

Dangit! I thought that would do it. I'll keep trying to look at this.

dcam’s picture

Hi ricky.middaugh! Thanks for helping out with this issue! You've done a great job so far. I think I've figured out where the problems are.

First, the change to core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php doesn't look quite right. It should only have one change, the second one, and it should be one line lower, outside that closing curly bracket.

Second, your patches are missing the changes to core/modules/system/templates/breadcrumb.html.twig.

I would suggest starting from scratch and trying a new reroll of #39, but you can build on your earlier work if you prefer. The instructions for using the Git command line to reroll patches can be found at https://drupal.org/patch/reroll.

Whenever I reroll a patch I like to look at the original patch and my rerolled version side by side. Then I compare them line by line. This helps me make sure that I haven't missed or accidentally added anything. In the case of a patch like this, the two should be nearly identical except for the filepaths, as most or all of the reason the reroll was needed was due to the change in directory structure. The surrounding code probably hasn't changed at all.

Let me know if you need any help!

ricky.middaugh’s picture

Assigned: ricky.middaugh » Unassigned
Status: Needs work » Needs review
FileSize
753 bytes

Ugh, figured it was something simple like that.

Here's another reroll for up to the newest commit.

mgifford’s picture

@ricky.middaugh this passes, which is good, but aria-labelledby="links__system_main_menu" needs to point to an ID (and I don't think it does right now.).

I'm also not sure what happened to all of the other nav elements which were highlighted above...

mgifford’s picture

Title: Add aria-label attributes to all <nav> elements » Add aria-label or aria-describedby attributes to all <nav> elements
ricky.middaugh’s picture

FileSize
8.39 KB

@mgifford The links__system_main_menu id is set it core/includes/theme.inc in template_preprocess_page(), set on the main menu (Same with the secondary menu in the same fashion).

That patch was from a merge conflict I ran into, this one should include all the changes.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
58.73 KB
62.83 KB
112.99 KB
102.79 KB
57.12 KB

Ok, this looks good to me. Here are the screenshots:

Breadcrumbs
Breadcrumbs screenshot

Tabs
Tabs screenshot

Secondary Nav
Secondary Nav screenshot

Toolbar
Toolbar screenshot

Book Module
Book Module screenshot

joelpittet’s picture

Added a quick follow up to this to remove that deprecated 'class' attribute on header for #theme links.

#2285493: Remove deprecated 'class' property from #theme 'links' and #theme 'menu_tree' heading arrays

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, great!

Committed and pushed to 8.x. a11y for the win. :)

  • Commit ed310e1 on 8.x by webchick:
    Issue #2052473 by ricky.middaugh, lokapujya, Jalandhar, mgifford,...
mgifford’s picture

Excellent! Glad it's in.

mgifford’s picture

tim.plunkett’s picture

Status: Fixed » Needs work
Issue tags: +Needs tests
+++ b/core/modules/block/block.module
@@ -382,6 +382,13 @@ function template_preprocess_block(&$variables) {
+  if (isset($variables['elements']['#block']->subject) && isset($variables['attributes']['role'])) {

$block->subject has been gone since #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer, so this neither works nor has test coverage.

joelpittet’s picture

Thanks for pointing that out @tim.plunkett and just to keep y'all are in the loop there are attempts to change that to title instead of label in the theme layer to keep some consistency with title_suffix/prefix and title_attributes.
#1939224: Change block "label" so that it's called a title like everything else in the template file (and all other template files)
#1939234: Change node "label" so that it's called a title like everything else in the template file (and all other template files)

Berdir’s picture

Posted a possible fix for this in #1989568: Remove block config ID from being used as an HTML ID or template suggestion, but still missing tests. Also didn't find any 'role' attributes in core, so I don't think this would be printed anywhere even with the right check.

mgifford’s picture

FileSize
855 bytes

Ok, so there's this change here (which is applied to HEAD now). And we need tests.

It's not clear to me how we should proceed. Tests are always good, but I'm no good at writing them.

@tim.plunkett - thoughts on how to proceed?

@joelpittet & @Berdir - always good to know about related issues.

tim.plunkett’s picture

There is no $block->admin_label either. That's part of the block's plugin definition.

Manually testing this would be a start, since this 100% does not work.

Writing a web test for it is the next step.

lauriii’s picture

Issue summary: View changes
mgifford’s picture

Status: Needs work » Needs review

Probably the bigger issue is that the block module doesn't include any <nav> code. In which case, looks like there is some code which can be removed. It could be that it should be addressed elsewhere, but this is focused on this one HTML tag.

core/themes/bartik/templates/comment.html.twig

This should probably be labelled as "Related links" or something like that:

      <nav>
        {{ content.links }}
      </nav>

core/modules/system/templates/page.html.twig

Looking at this page, I'm wondering if this might be a mistake too. There is no consideration for if there is no main_menu but just a secondary_menu.

  {% if main_menu or secondary_menu %}
    <nav role="navigation" aria-labelledby="links__system_main_menu">
      {{ main_menu }}
      {{ secondary_menu }}
    </nav>
  {% endif %}

This should probably also be identified as "Local actions" but this could be clarified.

      {% if action_links %}
        <nav class="action-links">{{ action_links }}</nav>
      {% endif %}

Status: Needs review » Needs work

The last submitted patch, 63: 2052473-63.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll, +TwinCities
mgifford’s picture

Issue tags: -TwinCities +TCDrupal 2014

Status: Needs work » Needs review

mrjmd queued 63: 2052473-63.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 63: 2052473-63.patch, failed testing.

mgifford’s picture

Issue tags: -TCDrupal 2014 +dcamsa11y
lauriii’s picture

I think this issue is at least some how related to this #1869476: Convert global menus (primary links, secondary links) into blocks

mgifford’s picture

LewisNyman’s picture

Issue tags: -Novice +Amsterdam2014

No longer novice

Rade’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
803 bytes
1.31 KB

I rerolled the patch from #63 following the instructions from "Re-rolling patches". Fixed the conflict manually.

I don't think this new patch makes much sense, still it's using the admin_label that according to @tim.plunkett doesn't exist anymore, but at least this patch applies cleanly, and maybe it's easier to pick up from.

Thorough manual review needed!

joelpittet’s picture

Issue tags: +Needs manual testing

Thanks @rade tagging as needs manual testing

lauriii queued 76: 2052473-76.patch for re-testing.

mgifford queued 76: 2052473-76.patch for re-testing.

mgifford’s picture

On the home page with a default install I can't see any change to the number of aria-label references before or after this patch.

There are blocks enabled on the home page, but I can't see where this patch is changing the output at the moment.

Is there an instance where <nav> is defined that doesn't have an aria attribute associated with it that we know of?

Perhaps we should mark this issue postponed until we find a good way to test it.

mgifford queued 76: 2052473-76.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 76: 2052473-76.patch, failed testing.

mgifford’s picture

Issue tags: +Needs reroll
rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
803 bytes

LewisNyman queued 84: 2052473-84.patch for re-testing.

leandro713’s picture

the patch works for me.
i can see nav tags with the attribute aria-labelledby for the user menu and every block

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgifford’s picture

@leandro713 looking at the patch this is about proactively adding aria-describedby if possible. Not aria-label.

@rpayanm do you have any sense as to where we would see this in action?

  • webchick committed ed310e1 on 8.3.x
    Issue #2052473 by ricky.middaugh, lokapujya, Jalandhar, mgifford,...

  • webchick committed ed310e1 on 8.3.x
    Issue #2052473 by ricky.middaugh, lokapujya, Jalandhar, mgifford,...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

BarisW’s picture

I have no idea what this piece of code does. On most places (menu blocks, navigation, breadcrumb and pager) the labelled-by attribute is added in the twig files.

<?php
  // Proactively add aria-describedby if possible to improve accessibility.
  if ($variables['label'] && isset($variables['attributes']['role'])) {
    $variables['title_attributes']['id'] = Html::getUniqueId($variables['label']);
    $variables['attributes']['aria-describedby'] = $variables['title_attributes']['id'];
  }
?>

So what is the purpose of this piece of code anyway?

mgifford’s picture

The purpose of it (from what i recall) is to make sure that we're associating related elements with aria-describedby in a programmatic way. Theoretically we'd see it in the label tags where there is a description element.

As I said in #88, I don't know where this appears, if anywhere. It may not appear and as you stated, we may be doing this in twig files anyways.

Hope that's clear.

Thew’s picture

Is this issue still needs review or manually testing?
I tested the patch from #88 in many sections and pages and see there is no different before and after apply a patch.

mgifford’s picture

Status: Needs review » Needs work

I think we need a few examples where aria-label or aria-describedby attributes aren't added to elements first.

This issue might just be too generic.

Looking through the code I could only find two instances from the templates:

$ grep -ir '<nav>' core/*
core/modules/system/templates/block--local-actions-block.html.twig:    <nav>{{ content }}</nav>
core/themes/stable/templates/block/block--local-actions-block.html.twig:    <nav>{{ content }}</nav>

Thanks @Threw

oscar_wong67’s picture

Category: Task » Feature request

@mgifford, is aria-label/aria-describedby always meant to be added as the first attribute on a tag according to the Drupal code style? Could it just be enforced and maybe noted somewhere?

Could this maybe be simplified by automatically creating aria-label attributes when none are specified, based on text? I know this was done with the Angular Material project (https://github.com/angular/material)

mgifford’s picture

I'm pretty confident that the Drupal code style doesn't define this.

I don't think we can just run with the first attribute on a tag approach.

How does the Angular Material automatically create the aria-label and is it always accurate?

It is unclear to me that there will be a single purpose for the <nav> in block--local-actions-block.html.twig

  • webchick committed ed310e1 on 8.4.x
    Issue #2052473 by ricky.middaugh, lokapujya, Jalandhar, mgifford,...

  • webchick committed ed310e1 on 8.4.x
    Issue #2052473 by ricky.middaugh, lokapujya, Jalandhar, mgifford,...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Needs work » Postponed (maintainer needs more info)

I'm following up on issues that have been committed and re-opened.

This issue was committed to Drupal 8.x in Jun 2014 and re-opened in #60 due to needing tests and a question about the implementation. The last discussions were 5 years ago.

Is there anything still to do here? If so, update the Issue Summary and add a comment.

Thanks

Edit: fix typo

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Postponed (maintainer needs more info) » Fixed

There hasn't been any further information provided so I am restoring the Fixed status, originally set in #56 at the time of the commit to Drupal 8.

Cheers.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.