In #966396: Decide how to include WAI-ARIA landmark roles we discuss how WAI-ARIA roles and aria-* attributes should be applied in Core. For more information see WAI-ARIA Roles

Here is a list of Core blocks and the roles which I think should be assigned. If we get anything close to agreement I can role a patch.

I left the block_test blocks without roles, since we are designing the roles to be easily removed, and removing from block_test would also mean a test failing on removal (assuming we write a test for the role). Let me know what you thing about a test for role being set on block attributes.

Also, user defined (custom) blocks do not get a role, we really need to sort that out, but in another issue and later.

aggregator
- Category latest items (complementary)
- Feed latest items (complementary)
Blocks
- Custom blocks (OMG who knows)
Block example
- Empty (complementary)
- Configurable text string (complementary)
Block test (N/A)
Blog
- Recent (navigation)
Book
- Navigation (navigation)
Comment
- Recent (navigation)
Devel
- Execute PHP (complementary)
- Switch user (complementary)
Devel node access
- Devel node access (complementary)
- Devel node access by user (complementary)
Forum
- Active topics (navigation)
- New topics (navigation)
Local
- Language switcher (complementary)
Menu
- menu (navigation)
Node
- Syndicate (complementary)
- Recent (navigation)
Poll
-Recent (complementary)
Profile
- Author information (complementary)
Search
- Search form (search)
Shortcut
- Shortcuts (navigation)
Statistics
- Popular content (navigation)
System
- main (main)
- powered by(complementary)
- System help (complementary)
- System menus (navigation)
User
- login (form)
- Who's new (complementary)
- Who's online (complementary)

Files: 
CommentFileSizeAuthor
#113 aria-roles-1183042.112.fail_.patch1.24 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 55,998 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#113 aria-roles-1183042.112.pass_.patch2.01 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,903 pass(es).
[ View ]
#111 aria-roles-1183042.111.fail_.patch1.24 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#111 aria-roles-1183042.111.pass_.patch2.01 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#106 1183042-block_roles-helponly-106.patch788 bytesmgifford
PASSED: [[SimpleTest]]: [MySQL] 57,139 pass(es).
[ View ]
#93 1183042-block_roles-helponly-93.patch1.05 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 56,211 pass(es).
[ View ]
#86 1183042-block_roles-helponly-86.patch530 bytesmgifford
PASSED: [[SimpleTest]]: [MySQL] 35,066 pass(es).
[ View ]
#66 1183042-block_roles-66.patch9.19 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 34,560 pass(es).
[ View ]
#45 1183042-block_roles-45.patch9.19 KBEverett Zufelt
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1183042-block_roles-45.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#37 1183042-block_roles-37.patch9.03 KBEverett Zufelt
PASSED: [[SimpleTest]]: [MySQL] 34,283 pass(es).
[ View ]
#33 1183042-block_roles-33.patch9.04 KBEverett Zufelt
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1183042-block_roles-33.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#31 1183042-block_roles-31.patch8.9 KBEverett Zufelt
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/search/search.module.
[ View ]
#16 1183042-blockroleattributes.patch14.13 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 33,303 pass(es).
[ View ]
#14 blockattributes.patch13.56 KBRobLoach
PASSED: [[SimpleTest]]: [MySQL] 33,303 pass(es).
[ View ]
#5 block_roles.patch9.75 KBEverett Zufelt
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block_roles_8.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#2 block_roles.patch10.08 KBEverett Zufelt
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in block_roles_7.patch.
[ View ]

Comments

mgifford’s picture

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.

Everett Zufelt’s picture

Assigned:Everett Zufelt» Unassigned
Status:Active» Needs review
StatusFileSize
new10.08 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid patch format in block_roles_7.patch.
[ View ]

* main role is applied to block, node title is outside of block
* Did not apply roles to block_test.
* Did not write tests (thoughts?).
* Did not write tests for devel or examples (should write a patch for example module

Status:Needs review» Needs work

The last submitted patch, block_roles.patch, failed testing.

Everett Zufelt’s picture

Everett Zufelt’s picture

Status:Needs work» Needs review
StatusFileSize
new9.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block_roles_8.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Try again.

mgifford’s picture

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!

casey’s picture

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

Everett Zufelt’s picture

@Casey

I did think about this when picking roles.

1. All core blocks have received roles, so it would be strange to exclude this one. The choices were between complementary and form. Given the option I wen with form.
2. The form role is not a widget role, i.e. it is not used to add semantics to an element, but to act as a navigational aid for users. If it were to add semantics, then I would agree that it would not be necessary, as the <form> already does this.

My only real concernsares:

1. Did I miss any blocks in Core, or misrepresent any blocks with the wrong role?
2. What do we do with main from system.module? role=main can only be used once, and it would seem to make more sense on the content region, not on the main block. That is, there can be more main content (think block that comes before contact form) in the content region than what appears in the main block. This would of course require a special case for the main content block, but given the benefits I would be comfortable w/ that.

Jeff Burnz’s picture

Just subscribing for now.

mgifford’s picture

Issue tags:-accessibility, -html5

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

Status:Needs review» Needs work
Issue tags:+accessibility, +html5

The last submitted patch, block_roles.patch, failed testing.

RobLoach’s picture

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

Everett Zufelt’s picture

@Rob

Can you please give an example of how to add an attribute to a block wrapper w/o a preprocess function?

The original intent was to ensure:

1. That all blocks get a role attribute.
2. That the attribute is easy to remove by contrib themes (not sure if we care about this anymore).
3. That a good example is set for contrib blocks.

We also still need to decide how to treat the main content block, which is getting role="main" but where this is actually likely better set on the content region wrapper.

RobLoach’s picture

Status:Needs work» Needs review
Issue tags:+hook_block_view
StatusFileSize
new13.56 KB
PASSED: [[SimpleTest]]: [MySQL] 33,303 pass(es).
[ View ]
  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!!!
RobLoach’s picture

Status:Needs review» Needs work

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.

RobLoach’s picture

Status:Needs work» Needs review
StatusFileSize
new14.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,303 pass(es).
[ View ]

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

Jeff Burnz’s picture

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.

RobLoach’s picture

Status:Needs review» Needs work

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.

RobLoach’s picture

Status:Needs work» Needs review

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().

Everett Zufelt’s picture

I'm pretty strongly opposed to an ARIA module. If there were to be such a module it would need to be Core required.

ARIA isn't add-on, it is essential to providing an accessible experience. I think that it should be added as close to the element it is attributing as possible, however, I will stay out of the preprocess / block_view discussion, and leave that to people with a better understanding of the direction these APIs have been taking.

Jacine’s picture

Status:Needs review» Needs work

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.

RobLoach’s picture

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

<?php
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:

<?php
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.

Jacine’s picture

@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

Owen Barton’s picture

Subscribe

mgifford’s picture

Issue tags:+aria

adding tag

indytechcook’s picture

JohnAlbin’s picture

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.

Everett Zufelt’s picture

@JohnAlbin

Agreed.

Commented on #1189822: Convert maintenance-page.html.twig to HTML5 to have this rolled into that patch. page.tpl.php has already been converted to HTML5, so we should handle that as a followup to #1077578: [Followup] Convert bartiks page.tpl.php to HTML5.

Jacine’s picture

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

Everett Zufelt’s picture

Status:Needs work» Needs review
StatusFileSize
new8.9 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/search/search.module.
[ View ]

- Reroll for core/
- Removes the role=main for main system content block.
- Removes chunks for profile.module and blog.module (no longer in 8.x)

Status:Needs review» Needs work

The last submitted patch, 1183042-block_roles-31.patch, failed testing.

Everett Zufelt’s picture

Status:Needs work» Needs review
StatusFileSize
new9.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1183042-block_roles-33.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Ooops, this time let's have only one function declaration for search_preprocess_block().

mgifford’s picture

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.

Everett Zufelt’s picture

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

Status:Needs review» Needs work
Issue tags:+accessibility, +aria, +html5, +hook_block_view

The last submitted patch, 1183042-block_roles-33.patch, failed testing.

Everett Zufelt’s picture

Status:Needs work» Needs review
StatusFileSize
new9.03 KB
PASSED: [[SimpleTest]]: [MySQL] 34,283 pass(es).
[ View ]

Rerolled so that patch will apply.

Jacine’s picture

Status:Needs review» Needs work
+++ 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.

Everett Zufelt’s picture

@Jacine

I am okay with navigation on every menu block, only considering that the alternative is no role on any menu block, and the need to build a UI.

Can you think of an alternative, other than the all or nothing that I described?

Jacine’s picture

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.

bowersox’s picture

Sub

Jacine’s picture

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.

Everett Zufelt’s picture

@Jacine

I don't see what block you are referencing in locale ( http://api.drupal.org/api/drupal/core--modules--locale--locale.module/fu... ). All I see are dynamically generated blocks. Do you mean these should all be navigation? Just trying to clarify.

Jacine’s picture

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.

Everett Zufelt’s picture

Status:Needs work» Needs review
StatusFileSize
new9.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1183042-block_roles-45.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Updated to catch head, changed locale language switcher to 'navigation' from 'complementary'.

Jacine’s picture

Issue tags:+sprint

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.

Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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.

Everett Zufelt’s picture

@Gábor

I originally had thoughts of adding a 'type' to the block definition in hook_block_info, mapping to WAI-ARIA roles. Which could then be used in block.tpl.php. It would then open things up for future Core, or Contrib, to add a UI, or to extend the block config UI, to allow admins to set a 'type' for blocks other than the type in code.

This would also allow for setting the type (role) of custom blocks, or blocks provided by contrib modules that have not provided their own role. I'm not sure what persuaded me not to go this path, perhaps someone could point out any potential downsides?

Jacine’s picture

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.

Everett Zufelt’s picture

After reading @Gábor and @Jacine's comments I think that this is good to go.

The current approach seems best. The * only * reason to add role to a block is to create a ARIA landmark, which is a theming thing, and should be done on the theme layer.

Jacine’s picture

Status:Needs review» Reviewed & tested by the community

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

Everett Zufelt’s picture

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

Status:Reviewed & tested by the community» Needs work
Issue tags:+accessibility, +aria, +html5, +hook_block_view, +sprint

The last submitted patch, 1183042-block_roles-45.patch, failed testing.

Everett Zufelt’s picture

Issue tags:+Novice

Haven't looked, but would strongly suspect that #1301040: Move language listing functionality from locale.module to a new language.module is what is causing this to need a re-roll.

Everett Zufelt’s picture

Status:Needs work» Needs review

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

Everett Zufelt’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC, Head was broken.

catch’s picture

Status:Reviewed & tested by the community» Needs review

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.

Jacine’s picture

Status:Needs review» Needs work
Issue tags:-Novice

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.

catch’s picture

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.

Jacine’s picture

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.

catch’s picture

Status:Needs work» Reviewed & tested by the community

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.

Jacine’s picture

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. :)

catch’s picture

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

Status:Reviewed & tested by the community» Needs work
Issue tags:+accessibility, +aria, +html5, +hook_block_view, +sprint

The last submitted patch, 1183042-block_roles-45.patch, failed testing.

Everett Zufelt’s picture

Status:Needs work» Needs review
StatusFileSize
new9.19 KB
PASSED: [[SimpleTest]]: [MySQL] 34,560 pass(es).
[ View ]

Re-rolling so that patch applies.

Everett Zufelt’s picture

Status:Needs review» Reviewed & tested by the community

Setting back to RTBC as the change between #45 and #66 was only to catch up with head.

catch’s picture

Status:Reviewed & tested by the community» Fixed

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.

Everett Zufelt’s picture

Title:Add WAI-ARIA roles to Core blocks» Change notice: Add WAI-ARIA roles to Core blocks
Priority:Normal» Critical
Status:Fixed» Active

Regardless where the change notice is posted (alone or in a meta node with all HTML5) it needs to not be forgotten. I have no preference, and will post a change notice once someone gives me direction of the preferred path.

David_Rothstein’s picture

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?

Everett Zufelt’s picture

@David

I like the approach. I think that it should be opened as a feature request / task and that this patch should not be reverted. I'm happy to help with coding the suggested implementation in a new issue, if there is consensus.

This also means that preprocess functions could remove / alter the attribute if desired, and that hook_block_info_alter() could also alter the type / role. It would also make it easier for Core / Contrib to expose Role / Type through a UI for custom / Contrib blocks. I would suggest that along with the landmark roles, that the 'region' document structure role be included (for discussion in a new issue, but here so that I remember).

catch’s picture

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

David_Rothstein’s picture

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.

Eric_A’s picture

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

Everett Zufelt’s picture

** This is not the issue in which to discuss possible changes. The community has decided to commit this approach to adding ARIA roles to blocks. If someone wants to see a change, please open a new issue.

Eric_A’s picture

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().

David_Rothstein’s picture

xjm’s picture

Issue tags:+Needs change record

Tagging.

Cottser’s picture

Assigned:Unassigned» Cottser

I'm going to work on a change notification.

Cottser’s picture

Status:Active» Needs review
Issue tags:-Needs change record

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

xjm’s picture

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!

Everett Zufelt’s picture

Priority:Critical» Normal
Status:Needs review» Reviewed & tested by the community

Looks good to me.

Gábor Hojtsy’s picture

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Closed (fixed)

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

sun’s picture

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

mgifford’s picture

Status:Closed (fixed)» Needs review
StatusFileSize
new530 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,066 pass(es).
[ View ]

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

sun’s picture

Status:Needs review» Reviewed & tested by the community

Like that. Thanks!

catch’s picture

Title:Change notice: Add WAI-ARIA roles to Core blocks» Add WAI-ARIA roles to Core blocks
Status:Reviewed & tested by the community» Fixed

Thanks. Committed/pushed to 8.x.

Status:Fixed» Closed (fixed)

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

effulgentsia’s picture

Title:Add WAI-ARIA roles to Core blocks» Regression: Add WAI-ARIA roles to Core blocks
Status:Closed (fixed)» Active
Issue tags:+Needs tests, +Blocks-Layouts

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)?

David_Rothstein’s picture

Issue tags:-Needs tests, -Blocks-Layouts

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:

<?php
  $variables
['attributes_array']['role'] = 'complementary';
?>

Whereas most other implementations do this:

<?php
  $variables
['attributes']['role'] = 'complementary';
?>

So perhaps it has something to do with that.

David_Rothstein’s picture

Issue tags:+Needs tests, +Blocks-Layouts

Hm... disappearing tags.

mgifford’s picture

Status:Active» Needs review
StatusFileSize
new1.05 KB
PASSED: [[SimpleTest]]: [MySQL] 56,211 pass(es).
[ View ]

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.

Cottser’s picture

Assigned:Cottser» Unassigned
mgifford’s picture

mgifford’s picture

mgifford’s picture

mgifford’s picture

mgifford’s picture

joelpittet’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+Quick fix

#93 Thanks for the cleanup @mgifford

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

mgifford’s picture

Thanks Joel for the RTBC!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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

joelpittet’s picture

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

webchick’s picture

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?

joelpittet’s picture

Issue tags:-sprint+Needs reroll

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

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new788 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,139 pass(es).
[ View ]

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.

jessebeach’s picture

Status:Needs review» Reviewed & tested by the community

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.

larowlan’s picture

/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

larowlan’s picture

Status:Reviewed & tested by the community» Needs work

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.

larowlan’s picture

Assigned:Unassigned» larowlan

Adding tests

larowlan’s picture

Assigned:larowlan» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.01 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

larowlan’s picture

Issue tags:-Needs tests, -Needs reroll

updating tags

larowlan’s picture

Issue tags:+Needs tests, +Needs reroll
StatusFileSize
new2.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,903 pass(es).
[ View ]
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] 55,998 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

joelpittet’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests, -Needs reroll

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.
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed c36b54c and pushed to 8.x. Thanks!

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