Problem/Motivation

See #1183042-70: Regression: Add WAI-ARIA roles to Core blocks and the next few comments that follow it. (@todo: It would be good to summarize those points here.)

Proposed resolution

One of the following:

  1. Use hook_block_view(). This would rely on #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks and the modules would add the WAI-ARIA attributes directly.
  2. Classify blocks in hook_block_info(), so that the metadata is available in the correct place, and then have template_preprocess_block() add the WAI-ARIA attributes based on that. This would be straightforward also, except the hook_block_info() 'properties' element is currently not stored in the database so it's not available when a block is loaded. We might need to put it in the database for this to work. That's probably a good idea anyway, since currently almost everything in hook_block_info() lives in the database, and it's inconsistent that this one isn't.
  3. Do nothing, as per #1, since "We use preprocess functions to add attributes all over the place in core."

Remaining tasks

Original report by David_Rothstein

For background on this issue, read #1183042-70: Regression: Add WAI-ARIA roles to Core blocks and the next few comments that follow it.

Here, we should do one of the following:

  1. Use hook_block_view(). This would rely on #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks and the modules would add the WAI-ARIA attributes directly.
  2. Classify blocks in hook_block_info(), so that the metadata is available in the correct place, and then have template_preprocess_block() add the WAI-ARIA attributes based on that. This would be straightforward also, except the hook_block_info() 'properties' element is currently not stored in the database so it's not available when a block is loaded. We might need to put it in the database for this to work. That's probably a good idea anyway, since currently almost everything in hook_block_info() lives in the database, and it's inconsistent that this one isn't.

I prefer option #2, but either is preferable to the current situation where each module has to implement a theme preprocess function to add the role attribute to its blocks.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jacine’s picture

Why are we singling out the "role" attribute here? These are just attributes, and this is not just something that is done for blocks. I said this like 10 times in the referenced issues, but I guess it's falling on deaf ears.

Anyway, what I care about here is consistency. We use preprocess functions to add attributes all over the place in core. As a themer, I know to look in these preprocess functions to find out what's going on without having to print out endless arrays and refer to a crap load of functions, and I would really, really hate to see us introduce a completely different way of doing it just for blocks, and even worse just for a single attribute. So, please, whatever you guys decide here, please take that into account.

catch’s picture

Priority: Normal » Major

Bumping this to major. The preprocess stuff was acknowledged as a hack when it went in (but unfortunately no worse than what we're doing in core for many other things), but we should try to clean that up if at all possible.

Following on from the other issue, having the 'role' in block API (but not the actual HTML attribute) does seem like a reasonable approach to me if you look at it from the point of metadata. However there'd need to be clear docs about what the valid values for that are (I guess that's no worse with the current approach though).

Everett Zufelt’s picture

Pros to using hook_block_info()

- Classifying the type, or semantic, of a block.
- Can be altered
- Can be presented through a UI (not part of this issue) so that the type of a block can be altered, or in the case of custom blocks added
- The role property (meta data about the block) will be added in template_preprocess_block (this is where a themer could look to see that info->property->role is being added to the attributes array, and could override / alter it.

All WAI-ARIA landmark roles can be used freely (no limit on where / how many times they are used) other than "main". The "main" role is currently being applied, in Core, to the "content" region wrapper, and should be omitted. The document structure "region" role should also be added (better name perhaps required). Using "region" with the aria-labelledby attribute allows for a block to be identified as a unique region of a document (page), and for that region to be labelled.

<div role="region" aria-labelledby="block-id">
  <h2 id="block-id">Donations</h2>
  Stuff about how you should donate to my awesome project...
</div>
Everett Zufelt’s picture

Note, I'm pretty flexible on adding "region" it is recommended that landmark roles be used, leaving "region" as a last resort where the landmark roles don't carry the necessary semantics. But, it is confusing, and for those who require it "region" could be added using custom hook_block_info_alter() implementations, or a custom addition to block UI.

David_Rothstein’s picture

Jacine: Nothing you ever say should fall on deaf ears :) I think the disagreement is about whether this is "just" an attribute. It appears in the HTML as an attribute, but I think it's really a classification scheme that module authors are applying to their blocks, so that screen readers can interpret it (and various parts of Drupal could in theory interpret it too). Like <input type="text"...>.... The 'type' is an attribute but a module author won't ever put it in the attributes array directly, because it has much more meaning than that, and Drupal (not to mention web browsers) uses this meaning in all sorts of fundamental ways.

It sounds like between the two options I posted above, you're objecting to option #1 but not option #2 basically... if in the end template_preprocess_block() is where the HTML attribute is actually added, that should be fine, no?

...and this is not just something that is done for blocks...

Aren't blocks the only place where a module author would ever interact with this, though? It looks like there are couple places in page.tpl.php and similar where the WAI-ARIA landmark roles are hardcoded (some of which are things that should ideally be converted to blocks anyway, others of which are special cases) but since blocks are really the only way we have for module authors to provide content for regions of the page, I think this is probably the only place they'd ever need to deal with it.

Eric_A’s picture

A) Option #1 does not imply not populating $attributes_array in template_preprocess_block().
B) Block module builds a renderable array for every block. Bypassing that for only some of your HTML attributes would be... strange. Block module can easily add new data to the existing render element.
So, yes, indeed! Right now the easiest place to start the life of attributes is in hook_block_info(). Simply because hook_block_view() does not have full access to the render element right now while _block_get_renderable_region() can do so much more.

xjm’s picture

A change notice for #1183042: Regression: Add WAI-ARIA roles to Core blocks is here:
http://drupal.org/node/1430892

So, eventually, we'll want to update that change notice based on the outcome of this issue. I'll add a note to the summary here. Feel free to also update the summary based on the discussion.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Priority: Major » Normal

There doesn't seem to be any strong consensus here, and as #1 points out, this is a pattern used elsewhere in core and there may be nothing to do at all, so I'm bumping this down.

sun’s picture

Priority: Normal » Major

The lack of consensus has no effect on the severity of a problem.

As others have noted already, it doesn't make sense to have to implement a theme preprocess function just to add an HTML attribute.

However, @David_Rothstein can you clarify how this issue differs from #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks? Isn't it a duplicate?

David_Rothstein’s picture

I would say option #2 is either of a duplicate of that issue or dependent on it (I'm not sure which).

However, option #1 is definitely different.

David_Rothstein’s picture

Title: Use hook_block_info() or hook_block_view() to automatically add WAI-ARIA roles to blocks » Allow modules to classify blocks when defining them, so WAI-ARIA roles can automatically be added in the theme layer
Priority: Major » Normal

Um, I managed to get that completely backwards... Option #1 is the duplicate/dependent one, and option #2 is new :)

Anyway, given that, let's refocus this issue on option #2. I think Everett's summary in comment #3 nicely explains why this is a good idea, in particular:

Pros to using hook_block_info()

- Classifying the type, or semantic, of a block.
....
- Can be presented through a UI (not part of this issue) so that the type of a block can be altered, or in the case of custom blocks added

I haven't seen anyone even try to rebut those points, so unless someone does, this is the direction we should go in here.

To clarify, this is about allowing module authors to classify blocks based on their purpose; the mapping between that classification and WAI-ARIA role attributes (even if it turns out to be one-to-one mapping) still happens in the render/theme layer. So this issue will not result in HTML attributes being defined in unholy places where they shouldn't be.

However... I think there is a good argument that says this issue and #1302482: Ensure render arrays properly handle #attributes and add them there instead of preprocess hooks should not both be major priority (since they were marked major for essentially the same reason). And I think we should give "major" to that one, since it's more far-reaching.

tim.plunkett’s picture

hook_block_info and hook_block_view are gone, so the issue summary is out-of-date.

I wonder if we could add something to the annotation. It'd be available in hook_block_alter(), and that's all available everywhere.

David_Rothstein’s picture

Status: Active » Needs review
FileSize
4.19 KB

I was thinking something like this (patch contains a couple example conversions). The fact that I had to repeat the definition for the user login block in the install profile in order to get it to work at all is... interesting. So perhaps I'm doing something wrong.

The problem with putting it in the annotation is that modules couldn't define it dynamically (or at least, couldn't without hook_block_alter()-ing their own annotations). Also somewhat goes against #1683644-74: Use Annotations for plugin discovery, although I guess it could be considered something you'd want to know about a block even during "discovery" (i.e. even when the block is disabled/not being instantiated).

sun’s picture

Hm. I don't think that this classification is configuration — at least not the default classification that lives in code.

A user-supplied override might be configuration, but the in-code definition is definitely not. We really need to be careful to not save stuff into configuration that wasn't actually configured.

I wonder whether it really makes sense to call this "category". The available + valid list of roles is pretty abstract and while it is a NIH vocabulary, I'm entirely not sure whether this is an appropriate vocabulary to base the categorization on:
http://www.w3.org/TR/wai-aria/roles#role_definitions

I also don't know whether it is a good idea to have a hard binding between internal/administrative meta categories and (some seemingly random) WAI-ARIA roles that only apply to the output/theming/machine-reading layer. The hard binding has the consequence that the categorization can never diverge between the two frontiers.

David_Rothstein’s picture

My understanding is that we'd be limiting this to "landmark" roles only: http://drupal.org/node/1179668 (which is a much friendlier-looking list).

We could also invent our own classification categories if we really wanted to, as long as there was some way to map them to the WAI-ARIA roles in the end.

mgifford’s picture

Issue tags: +Accessibility, +aria

Agreed that this was only focused on the landmark roles at least in my understanding.

Also, would be good to have the roles be defined so that the user can easily make sense of them. As they are defined right now the proposed list: application, banner, complementary, contentinfo, form, main, navigation & search - isn't necessarily the easiest way to guide a user who isn't already familiar with ARIA. It should be more functional with a link to the full technical details & best practices as defined in WAI-ARIA.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Accessibility, +aria, +Blocks-Layouts

The last submitted patch, block-classify-1402250-13.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
3.27 KB

Mostly a re-roll, but what happened to plugin.core.block.seven.login.yml & plugin.core.block.bartik.login.yml?

mgifford’s picture

mgifford’s picture

#19: block-classify-1402250-19.patch queued for re-testing.

mgifford’s picture

#19: block-classify-1402250-19.patch queued for re-testing.

mgifford’s picture

mgifford’s picture

FileSize
1.07 KB

An Interdiff failed to apply cleanly. This was the rejected code between 13 & 19:

*************** public function blockAccess() {
*** 30,35 ****
    }
  
    /**
     * Implements \Drupal\block\BlockBase::blockBuild().
     */
    public function blockBuild() {
--- 30,44 ----
    }
  
    /**
+    * Overrides \Drupal\block\BlockBase::blockSettings().
+    */
+   public function blockSettings() {
+     return array(
+       'category' => 'form',
+     );
+   }
+ 
+   /**
     * Implements \Drupal\block\BlockBase::blockBuild().
     */
    public function blockBuild() {
mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Accessibility, +aria, +Blocks-Layouts

The last submitted patch, block-classify-1402250-19.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

Just a re-roll. Had an issue with core/modules/user/lib/Drupal/user/Plugin/block/block/UserLoginBlock.php but just dropped the function back in though.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Accessibility, +aria, +Blocks-Layouts

The last submitted patch, block-classify-1402250-27.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

re-rolled

shanly’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Accessibility, +aria, +Blocks-Layouts

The last submitted patch, block-classify-1402250-30.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

Template was renamed for twig.

The last submitted patch, block-classify-1402250-33.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

#33: block-classify-1402250-33.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, block-classify-1402250-33.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

#33: block-classify-1402250-33.patch queued for re-testing.

mgifford’s picture

#33: block-classify-1402250-33.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Accessibility, +aria, +Blocks-Layouts

The last submitted patch, block-classify-1402250-33.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
3.27 KB

re-roll as diff in UserLoginBlock.php was rejected as existing function was dropped.

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, block-classify-1402250-40.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Added a third option

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 40: block-classify-1402250-40.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
10.63 KB

reroll

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
lahoosascoots’s picture

Status: Needs work » Needs review
FileSize
10.47 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 47: block-classify-1402250-47.patch, failed testing.

lahoosascoots’s picture

Adding in missing Xss use.

lahoosascoots’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 49: block-classify-1402250-49.patch, failed testing.

lahoosascoots’s picture

Status: Needs work » Needs review
FileSize
10.73 KB

Forgot to commit before diffing XD.

Status: Needs review » Needs work

The last submitted patch, 52: block-classify-1402250-52.patch, failed testing.

lahoosascoots’s picture

BlockConfigSchema test is tripping me up here. Spent a good couple hours on it and couldn't quite figure it out. Maybe someone can give me a hand?

alansaviolobo’s picture

Status: Needs work » Needs review
FileSize
9.02 KB
2.32 KB

Status: Needs review » Needs work

The last submitted patch, 55: allow_modules_to-1402250-55.patch, failed testing.

mgifford’s picture

What manual tests need to be done before this is marked RTBC?

mgifford’s picture

In looking to re-roll this patch (that no longer applies), I was unsure what happened to core/modules/field_ui/src/Form/FieldEditForm.php

It seems to be broken up now into a few other files. I am not sure how to update this patch. It looks like it could be in the buildForm from any or all of these:

core/modules/field_ui/src/Form/FieldStorageAddForm.php: public function buildForm(array $form, FormStateInterface $form_state, $entity_type_id = NULL, $bundle = NULL) {
core/modules/field_ui/src/Form/EntityDisplayModeAddForm.php: public function buildForm(array $form, FormStateInterface $form_state, $entity_type_id = NULL) {
core/modules/field_ui/src/Form/EntityDisplayModeAddForm.php: $form = parent::buildForm($form, $form_state);
core/modules/field_ui/src/Form/FieldStorageConfigEditForm.php: public function buildForm(array $form, FormStateInterface $form_state, $field_config = NULL) {

Someone who understands this better needs to re-roll the patch.

googletorp’s picture

Issue tags: -Needs reroll
EclipseGc’s picture

This definitely appears to be Annotation level information. I don't know why the current patch attempts to make this a configuration form element, but looking at the W3 specification for this, it does NOT appear to be something that is intended to be changeable. I recall this being in the annotations when we first did the blocks as plugins transition. Why was it moved out and hardcoded into preprocess functions? I am not sure, but we should likely move it back and make sure it's in the variables before preprocess functions ever fire.

Leaving this as needs work, though I could argue for postponed since this is already pretty static in its current form and that seems preferable.

Eclipse

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

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

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

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

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

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

tim.plunkett’s picture

Issue tags: -Blocks-Layouts

Not actively part of the Blocks-Layouts work.

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

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

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

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

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

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

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

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

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

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

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

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

xeM8VfDh’s picture

is this still being worked on?

mgifford’s picture

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

@xeM8VfDh I don't think someone is working on this now. Want to assign this to yourself and work on the patch from #55?

xeM8VfDh’s picture

If i get time I will look into it!

xeM8VfDh’s picture

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

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

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

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

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

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

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

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

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mgifford’s picture