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
Files: 
CommentFileSizeAuthor
#84 2052473-84.patch803 bytesrpayanm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,692 pass(es).
[ View ]
#76 interdiff-2052473-63-76.txt1.31 KBRade
#76 2052473-76.patch803 bytesRade
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2052473-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#63 2052473-63.patch855 bytesmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2052473-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,290 pass(es).
[ View ]
#50 2052473-50.patch753 bytesricky.middaugh
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,069 pass(es).
[ View ]
#45 2052473-45.patch8.19 KBricky.middaugh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,750 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#43 2052473-43.patch7.74 KBricky.middaugh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,753 pass(es), 0 fail(s), and 6 exception(s).
[ View ]
#39 2052473-39.patch8.38 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,267 pass(es).
[ View ]
#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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,272 pass(es).
[ View ]
#36 interdiff.txt673 byteslokapujya
#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

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
StatusFileSize
new2.93 KB
PASSED: [[SimpleTest]]: [MySQL] 59,226 pass(es).
[ View ]

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

fixing tag.

falcon03’s picture

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 NavigationBook 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.

<?php
/**
 * 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

StatusFileSize
new615 bytes
new2.93 KB
PASSED: [[SimpleTest]]: [MySQL] 59,044 pass(es).
[ View ]

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

StatusFileSize
new6.51 KB
new5.93 KB

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

Issue summary:View changes
StatusFileSize
new6.36 KB

Patch in #20 didn't apply any longer.

mgifford’s picture

StatusFileSize
new7.09 KB

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

StatusFileSize
new7.11 KB

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
StatusFileSize
new8.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,742 pass(es).
[ View ]

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
StatusFileSize
new8.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,114 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new673 bytes
new8.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,272 pass(es).
[ View ]
mgifford’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new97.75 KB
new101.05 KB
new80.38 KB
new101.33 KB
new116.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

StatusFileSize
new8.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,267 pass(es).
[ View ]

@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
StatusFileSize
new7.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,753 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

Here's the rerolled patch.

Status:Needs review» Needs work

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

ricky.middaugh’s picture

StatusFileSize
new8.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,750 pass(es), 0 fail(s), and 6 exception(s).
[ View ]

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
StatusFileSize
new753 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,069 pass(es).
[ View ]

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

StatusFileSize
new8.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,290 pass(es).
[ View ]

@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
StatusFileSize
new58.73 KB
new62.83 KB
new112.99 KB
new102.79 KB
new57.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

StatusFileSize
new855 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2052473-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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
StatusFileSize
new803 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2052473-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.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
StatusFileSize
new803 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,692 pass(es).
[ View ]