Regression: Add WAI-ARIA roles to Core blocks

This is a great list! We should probably see about adding it to http://drupal.org/project/accessible for D7. Thanks for classifying them this way.

Patch applies nicely Everett. I noticed that the Execute PHP Code block didn't have ARIA added but in the scheme of things.

I enabled the blocks and ran them through the WAVE Toolbar to verify. Looks good to me!

I don't think the form role on the login block is necessary; it already contains a form element.

Just subscribing for now.

#5: block_roles.patch queued for re-testing.

Instead of sticking this is preprocess functions, think we should stick it into the actually system itself?

1. Add "attributes" to hook_block_view and block.tpl.php
2. Use the attributes to add in $block['attributes']['role'] for each block
3. This allows alteration of the WAI-ARIA attributes in contrib via hook_block_view_alter
4. ???
5. PROFIT!!!

Should probably also make note of "attributes" in hook_block_view_alter().

And.......

+++ b/modules/system/system.moduleundefined
@@ -2071,16 +2076,6 @@ function system_block_view($delta = '') {
- */
-function system_preprocess_block(&$variables) {
-  // System menu blocks should get the same class as menu module blocks.
-  if ($variables['block']->module == 'system' && in_array($variables['block']->delta, array_keys(menu_list_system_menus()))) {

Move that code comment on top of $block['attibutes']['class'][] = 'block-menu';

-4 days to next Drupal core point release.

Fixed with my comments from above. Mind checking to make sure I got all the roles in there?

Hmmm, I gotta say this feels like the wrong direction to be heading in - aka a tad abusive of hook_block_view and pushing attribute adding into the API just feels wrong. IMO attributes are something added, removed, modified ect in the theme layer, we want cores output to be agnostic and clean, not loaded with more crap, especially when we need to use an alter just to get rid of. I've thought about this approach previously but discounted it on these grounds, make core output clean and without assumptions and let the theme layer handle markup.

Should ARIA support be moved into its own module then? I've always wanted to have "attributes" as part of hook_block_view(), had to hack a bunch into the theme system because of it before. If ARIA support was its own module, than we could implement aria_block_view_alter() and add in the roles cleanly.

The following is a good example of why having "attributes" in hook_block_view() is beneficial. We end up saving ourselves having to write an additional preprocess function.

+++ b/modules/system/system.moduleundefined
@@ -2049,14 +2049,17 @@ function system_block_view($delta = '') {
@@ -2064,6 +2067,9 @@ function system_block_view($delta = '') {

@@ -2064,6 +2067,9 @@ function system_block_view($delta = '') {
       if (isset($system_menus[$delta])) {
         $block['subject'] = t($system_menus[$delta]);
         $block['content'] = menu_tree($delta);
+        $block['attributes']['role'] = 'navigation';
+        // System menu blocks should get the same class as menu module blocks.
+        $block['attributes']['class'][] = 'block-menu';
         return $block;
       }
       break;
@@ -2071,16 +2077,6 @@ function system_block_view($delta = '') {

@@ -2071,16 +2077,6 @@ function system_block_view($delta = '') {
 }
 
 /**
- * Implements hook_preprocess_block().
- */
-function system_preprocess_block(&$variables) {
-  // System menu blocks should get the same class as menu module blocks.
-  if ($variables['block']->module == 'system' && in_array($variables['block']->delta, array_keys(menu_list_system_menus()))) {
-    $variables['classes_array'][] = 'block-menu';
-  }
-}
-

Generally, all template/theme stuff is template/theme stuff and is unique to the theme that you're currently using. ARIA is theme agnostic, meaning it should work across any theme you use. This means that it doesn't belong in the theme layer. Splitting it into its own module would mean that it works across any theme, and is decoupled from the system itself... Thoughts? Would just require an implementation of hook_block_views_alter() then.

Ummm, not needs work. Still applies cleanly, and works as advertised. Modules sometimes need to add "data-" and "class" attributes to their blocks, this is absolutely appropriate for hook_block_view().

Hmm, so this worries me too.

First, I don't support the idea of an ARIA module. ARIA is part of the HTML5 spec and I don't think it should be a quick switch to disable.

The main reasons I take issue with this approach are consistency and complexity. We usually manipulate attributes in preprocess functions. Why would we do it differently here? We need to get at and be able to manipulate this in the theme layer, in a consistent and easy way. Preprocess functions are what themers know. Forcing them into using alter hooks to manipulate attributes would be a mistake IMO. I get that it would be helpful to be able to manipulate this in hook_block_view(), for example, but we have entire sessions and issues in this queue talking about how we've destroyed the theme layer in D7 by making it so complicated, so it's a sore subject and something we need to be careful not to make worse.

On the other hand, this could be a great idea if we can make it work for everyone. I think we should discuss this in a separate issue. If we are going to change how we do this, then it's important that we make sure it works for everyone and that it's done system-wide so that it's consistent, and it shouldn't hold this up.

Right now, if you have a module where you're creating a block where you need a specific block class, you need to use:

mymodule_block_view() {
  $block['subject'] = t('Hi thare!');
  $block['content'] = '<div class="mymodules-specific-class"'>' . $content . '</div>';
  return $block;
}

It makes infinitely more sense to use:

mymodule_block_view() {
  $block['subject'] = t('Hi thare!');
  $block['content'] = $content;
  $block['attributes']['class'][] = 'mymodules-special-class';
  return $block;
}

Should I make a new issue to get this in? Preprocess functions for it still work and apply fine, this just gives module maintainers the freedom to decide how they want the class to be applied to the block. system_preprocess_block is an excellent example of why we want this.

@RobLoach Yeah, I think a separate issue would be better. It really has nothing to with ARIA roles for blocks. Link it back here so we can follow. Maybe it could also kill $vars['classes_array'] at the same time? :P

#1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks

Subscribe

adding tag

related #430886: Make all blocks fieldable entities

I believe role="main" should go in the page.tpl and maintenance-page.tpl because the page content's title is in those templates and is part of the "main" content.

role="main" is in the page template already. No need for follow-ups in those issues. :)

This looks good to me. Would be good to have someone else review it before marking it RTBC, however.

1) it applies nicely to git
2) after applying to a fresh D8 environment with generated content I verified that aria roles were being properly applied.
3) there are no changes that I can see other than improvements to accessibility.

Looks like a big step ahead for D8's accessibility.

+++ b/core/modules/locale/locale.moduleundefined
@@ -1073,6 +1073,15 @@ function locale_block_view($type) {
+function locale_preprocess_block(&$variables) {
+  if ($variables['block']->module == 'locale') {
+    $variables['attributes_array']['role'] = 'complementary';
+  }

The Locale module has a language switcher block that is pretty much a menu. I think you probably want a navigation role on it. Module is "locale" delta is "language."

+++ b/core/modules/menu/menu.moduleundefined
@@ -793,3 +793,12 @@ function menu_get_menus($all = TRUE) {
+function menu_preprocess_block(&$variables) {
+  if ($variables['block']->module == 'menu') {
+    $variables['attributes_array']['role'] = 'navigation';
+  }

So... question: Are you really okay with this on every menu? Because menus can be random lists of links. I use them all the time for things like social media links and basically any list of links that I think an administrator might want to edit.

21 days to next Drupal core point release.

You could apply it to the main and secondary menus directly. You can also apply it to any menu block that is in a "header" region. The header region idea isn't 100% reliable because region names can change, but it's very very common. If you are okay with it, I'm not gonna fight you on it. :) My only concern is that it will actually end up being way less useful if it's overused.

Sub

So... been thinking about this a little more, and applying the navigation role to all menu blocks is probably the best bet. There are definitely going to be cases where it is incorrect, but custom menus are often created and the potential to miss really important menus by just targeting the main and secondary menus is worse, so I take back my comment in #40. :)

I still think you want the navigation role on the locale switcher block, though. After that's done, I think this is ready.

Hmm, I've done many multi-language sites before and have only seen and themed one block: #block-locale-language. It's a block than contains the navigation to change the overall site language context, and yes it is dynamically generated. It's also a bit of a pain in the ass. In order even to see to see it you've got to enable more than 1 language, have at least one piece of content translated, and I think you might even have to have the URLs prefixes setup.

Anyway, looking at that code I can't tell if there is actually is more than one block or why either, so gonna go try and find someone who knows.

Tagging this for the next sprint. Just wanna double check with Gabor that I'm not missing anything here before RTBC'ing, because that code is confusing.

There can be multiple blocks exposed by locale to switch different languages. By Drupal core provides "content" "interface" and "url" language, and blocks are exposed for "content" and "interface" (if they are different). Drupal by default only exposes the interface language selector block. If you install entity_translation module, it turns on the content language selector block (along with possibly different rules for content language). Additionally to that, other modules can define other language types, but I have not yet seen such a module. All-in-all every block exposed by locale is a language switcher block only it applies to different language types.

BTW adding to the preprocess vs. hook_block_view() discussion, I agree with Rob Loach that it looks puzzling that modules implement preprocess hooks based on hard-wired data and only acting on their own blocks. If they do that, it should just be in the block definition, that is it IMHO.

Thanks Gábor!

The downside is the inconsistency in the way the attributes are handled. Everywhere else, except for forms, attributes are added via preprocess functions. See Node module and RDF module. I'd love to see this improved, but want to see consistency and I don't think blocks are special here. Fixing that is not going to be a quick issue and isn't exactly on topic here (this is just a side effect of a larger problem IMO), but we can bikeshed this and discuss here if you'd like. :)

BTW, this is not exactly hard-wired data. Themes have easy access to attributes in preprocess functions.

Yeah, I think this is ready.

The technique "issue" should fixed elsewhere and not just for blocks: #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks

I'd like to see more action in #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks before committing this.

It makes sense for RDF to add attributes in preprocess because it's acting on elements defined by other modules, that's not the same as block modules preprocessing their own stuff. Also I'm not sure why we can't get this with the existing renderable arrays that blocks can already return, at least I think we're closer to that being supported than #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks currently suggests.

Hmm, well I will not support that issue it until it moves beyond blocks. There is no reason blocks are special IMO, and consistency is a lot more important.

I find it disappointing and discouraging that as we attempt to use the theming system we have, we run into these kinds of roadblocks preventing us from getting things done. Here it's because people don't like using preprocess functions, over the in the theme_datetime() issue it's because we're being told we need to now account for #children in theme functions, and in the node template we don't have a properly sectioned title because the proposal was too hackish for some people's taste. Again, the problem is that this is the theme system we have. Now all of the sudden, as we need to actually use that theme system in core modules to implement this stuff, core developers are exposed to it (and apparently horrified by it in some cases), but this is the way we get things done and has been for a while. It is what it is. Until someone comes up with a plan to fix that, these are all piecemeal changes that need to be made as consistently as possible to prevent the problem from getting worse than it already is.

Given #58 and the fact that it has been sitting here for 2 weeks, I'm marking this needs work, because it's clearly not going to be committed the way it is. Unfortunately, I can't provide further direction for what needs to be done, because I do not know, and this has been happening more and more often lately to the point where I sit here wondering what I am even doing in this role.

I do support adding these roles though, and I hope it ends up happening. Hopefully someone else can help.

I didn't say it won't get committed as is, I said I'd like to see the other issue discussed further before taking any action here.

I know, but whatever happens in that issue has very little to do with this IMO. It's tough, when everything is held to a standard of perfection that does not even exist yet. This is the thing... This issue started in June. It follows the example of the existing block, system, node and other modules, not just RDF. All of this type of information is added in preprocess functions right now.

If people don't think that is ideal, fine. But, I don't understand what that has to do with this issue right now. IMO that is a separate issue entirely, and if it's going to be fixed, it will need to go way beyond the scope of this patch, and beyond the scope of the issue you linked as well. If it's decided in that issue or some other issue that this needs to be handled differently, then why can't it be modified in a future patch?

Bottom line is that you don't feel comfortable committing this... and that's fine. I'm not going to argue with you on that. :) If it doesn't need work and you want to see what happens in the other issue then maybe postponing it and increasing the priority of the other issue is more appropriate. I don't know, but, it's been reviewed and tested, so putting it back at needs review doesn't seem appropriate. Encouraging these issues to get up into the 100s of comments is something we should really try to avoid and I think that's exactly leaving it at needs review will do.

Obviously, this frustration is not coming from just this issue (and it's not directed at you), but I have to wonder why this issue (and others) and the people working hard on them have to suffer for simply using what we currently have in place. Obviously there is a level of clean-up involved in the patches we are doing, but the burden of fixing everything that is broken or not ideal with the current system is just too much to bear. We are making it really hard on contributors and it's just not fun at all.

OK I just looked at http://api.drupal.org/api/drupal/core--modules--node--node.module/functi... and got a headache.

While I don't think RDF is a good example because it's preprocessing stuff provided by other modules, that node trainwreck is a good argument that this isn't making an existing problem any worse. Moving back to RTBC although not going to commit it right now because I need food.

Why stop at Node module? LOL. Just kidding. I don't want to give you more of a headache so I will not post links to the others. Anyway, thanks @catch. :)

#45: 1183042-block_roles-45.patch queued for re-testing.

OK. Committed/pushed to 8.x.

I'm marking this fixed because I'm assuming the change notice is covered by the general HTML5 conversion, however if that's wrong please bump to critical/active/task for that.

Sorry, but I think we need to discuss this implementation more. I have some concerns with the patch that went in, and I think we can do better:

1. Performance: This adds a lot of function calls to every Drupal page load, basically "number of blocks on the page" multiplied by "number of enabled modules that provide blocks". Although these particular functions execute quickly, calling functions in PHP isn't cheap. It's not going to be a huge effect, but it happens on every page load so we should be careful. Adding hundreds (or maybe even thousands) of function calls on a typical site just to add a couple HTML attributes seems wasteful.
2. Discoverability: Most module authors are going to do hook_block_info() and hook_block_view() and stop there. Remembering to add a preprocess function is extra work that frankly probably won't happen, because it will be hard to even find the documentation or example core code that says you need to do that. Realistically, with this approach I think the number of contrib modules that add these ARIA roles will be very small.
3. Metadata: These roles are providing important semantic information about the block, but if we bury that in an HTML attribute then we lose the ability to use it. For example, wouldn't it be nice to get a list of all blocks on a site that have the 'form' role?

Putting this information in hook_block_view() or hook_block_info() is the way to solve the above problems. I see that was rejected above, but only because of concerns that you shouldn't randomly put HTML attributes in those places. I agree with that concern, but there's no reason it has to be implemented that way. Instead, the metadata could be added in the hook, and then template_preprocess_block() could take care of turning that metadata into an HTML attribute.

Everett suggested putting this in hook_block_info() in #49 and I think that's ideally the way to go, although the implementation might be a bit harder than hook_block_view(). One good thing about hook_block_info() is that we already have 'properties' there, and it seems like this would fit in perfectly with that.

So for a typical module (using Poll as an example), the preprocess function would go away and instead we'd do something like this:

 function poll_block_info() {
   $blocks['recent']['info'] = t('Most recent poll');
   $blocks['recent']['properties']['administrative'] = TRUE;
+  $blocks['recent']['properties']['role'] = 'complementary';
   return $blocks;
 }

I used 'role' above, but it could easily be 'type' as Everett suggested in #49. Either way, template_preprocess_block() is where this information would get converted into the actual ARIA role attribute.

Thoughts?

@David, I think this is discussion is better had on #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks. I'd personally rather we do this in hook_block_view() since if it's using the existing render array mechanism that'd be more generic than a hook_block_info() key, but either way agree there should be an API for this other than doing it in preprocess.

Happy to move this to a different issue - I just figured I'd raise it here especially since the change notification isn't in yet :)

I'm not sure #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks is the right issue though. That's about adding any old HTML attribute in general, but what we're talking about here is asking module authors to properly classify their blocks. It should be core's job to use that classification to generate accessible markup; we typically don't leave that kind of thing up to module authors to do on their own, at least not when we can help it. For example, we have '#title_display' => 'invisible' rather than asking people to add the 'element-invisible' class directly.

I'd personally rather we do this in hook_block_view()
Are you suggesting we pass the whole container around instead of just the content element? Because I think it's either that or add a second drupal_alter() later on.
The committed patch maps to the theme wrapper level ($attributes in the template), not to the content element level ($content_attributes in the template).

To clarify: I'm not talking about taking away any of the preprocessing possibilities or changing variables or templates. Nothing like that. It's just about *initializing* the attributes in an earlier phase than in the preprocessing hook (in the render element or in hook_info), thereby using less cycles, allowing for using the render cache and earlier opportunities for altering. The $atttributes_array preprocess variable itself would be first populated in template_preprocess_block().

I created #1402250: Allow modules to classify blocks when defining them, so WAI-ARIA roles can automatically be added in the theme layer to follow up.

Tagging.

I'm going to work on a change notification.

Drafted a change notification: http://drupal.org/node/1430892

The change notice looks great to me! I added #1402250: Allow modules to classify blocks when defining them, so WAI-ARIA roles can automatically be added in the theme layer to its list of issues as well, and added a note to that issue to update the change notification as needed based on that issue's outcome.

If one of the people who worked on this issue could review the change notification, that would be excellent. Thanks!

The change notice is there, so we can mark fixed? (There is no committer involved in change notices, so no RTBC needed).

+++ b/core/modules/help/help.module
@@ -61,3 +61,12 @@ function help_help($path, $arg) {
+/**
+ * Implements hook_preprocess_block().
+ */
+function help_preprocess_block(&$variables) {
+  if ($variables['block']->module == 'help') {
+    $variables['attributes_array']['role'] = 'complementary';
+  }
+}

Help module does not implement hook_block_*() itself. The help block is implemented by System module, so the condition is never true.

It needs to be ->module == 'system' && ->delta == 'help'.

Git blamed and pointed out by @tim.plunkett and @EclipseGc in IRC earlier, but not mentioned here.

So like this? Probably should have opened a new issue, but...

Like that. Thanks!

Thanks. Committed/pushed to 8.x.

Were tests ever added for this? It looks like this regressed due to #1535868: Convert all blocks into plugins. Maybe it'll get fixed as part of #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks, but re-opening in the meantime in case not, and to discuss what kind of test coverage we want: (do we want a test for every core block to make sure it outputs the correct role attribute)?

It actually looks like only some blocks are missing the roles now, not all of them. It mostly seems to be System blocks that are missing them (but maybe others too).

system_preprocess_block() does this:

  $variables['attributes_array']['role'] = 'complementary';

Whereas most other implementations do this:

  $variables['attributes']['role'] = 'complementary';

So perhaps it has something to do with that.

Hm... disappearing tags.

Following on @David's note I just decided to grep for this:
$ grep -ir attributes core/* | grep variables | grep role

I think this is all of the instances:
core/modules/aggregator/aggregator.module: $variables['attributes']['role'] = 'complementary';
core/modules/book/book.module: $variables['attributes']['role'] = 'navigation';
core/modules/comment/comment.module: $variables['attributes']['role'] = 'navigation';
core/modules/forum/forum.module: $variables['attributes']['role'] = 'navigation';
core/modules/help/help.module: $variables['attributes']['role'] = 'complementary';
core/modules/language/language.module: $variables['attributes']['role'] = 'navigation';
core/modules/menu/menu.module: $variables['attributes']['role'] = 'navigation';
core/modules/node/node.module: $variables['attributes']['role'] = 'complementary';
core/modules/node/node.module: $variables['attributes']['role'] = 'navigation';
core/modules/node/node.module: $variables['attributes']['role'] = 'article';
core/modules/search/search.module: $variables['attributes']['role'] = 'search';
core/modules/shortcut/shortcut.module: $variables['attributes']['role'] = 'navigation';
core/modules/statistics/statistics.module: $variables['attributes']['role'] = 'navigation';
core/modules/user/user.module: $variables['attributes']['role'] = 'form';
core/modules/user/user.module: $variables['attributes']['role'] = 'complementary';
core/modules/user/user.module: $variables['attributes']['role'] = 'complementary';

So for the oddballs:
core/modules/system/system.module: $variables['attributes_array']['role'] = 'complementary';
core/modules/system/system.module: $variables['attributes_array']['role'] = 'complementary';
core/modules/system/system.module: $variables['attributes_array']['role'] = 'navigation';

I rolled this patch to help move this issue along. I don't know where the $variables['attributes']['role'] is documented to verify this with the API.

#93: 1183042-block_roles-helponly-93.patch queued for re-testing.

#93: 1183042-block_roles-helponly-93.patch queued for re-testing.

#93: 1183042-block_roles-helponly-93.patch queued for re-testing.

#93: 1183042-block_roles-helponly-93.patch queued for re-testing.

#93: 1183042-block_roles-helponly-93.patch queued for re-testing.

#93 Thanks for the cleanup @mgifford

The rest can be captured over at #1484704: Remove instances of attributes_array

Thanks Joel for the RTBC!

Are we not adding tests so this doesn't get broken again?

@alexpott could you suggest a test that would do that because I can't think of one that would be useful after this gets in.

I can't think of one that would catch a typo like this, no.

However, maybe a test that did some kind of iteration through all the blocks, checked its properties for any ARIA role assignments (so came up with the list in #93), placed them all in some region, and then ensured the roles were rendered out when those blocks are displayed?

Tagging, looks like only $variables['attributes_array']['role'] = 'complementary'; is left in core so needs small reroll and still a test.

Looks like with this new patch we should be able to mark this RTBC as we can't think of a way to test it.

We really need a test for these. It would be easy to lose the ARIA info and not it's gone until another bug is filed.

/me wishes we had behat..... a test would be so easy.... or even better this issue.... #1959660: Replace xpath() with WebTestBase::cssSelect() by leveraging Symfony CssSelector the tests would be a snap then

We really need a test for these. It would be easy to lose the ARIA info and not it's gone until another bug is filed.

That's definitely needs work, sorry guys - I will look at this on Monday unless someone beats me to it.

Adding tests

Hoping first fails, second passes.
Looks like the complementary role is already on the help block but definitely no role on the powered by block so expecting that to fail.

updating tags

Yeah $thid isn't the same as $this.
larowlan--

Great work @larowlan you make it look so easy!

RTBC

• Has test with pass/fail to show that it catch's the the previous regression for WAI-ARIA roles gone missing.
• Checks both plugins 'system_powered_by_block' and 'system_help_block' for their roles.

Committed c36b54c and pushed to 8.x. Thanks!