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:
- 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.
- 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.
- Do nothing, as per #1, since "We use preprocess functions to add attributes all over the place in core."
Remaining tasks
- When this issue is resolved, update the change notification at: http://drupal.org/node/1430892
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:
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#55 | interdiff.txt | 2.32 KB | alansaviolobo |
#55 | allow_modules_to-1402250-55.patch | 9.02 KB | alansaviolobo |
#52 | block-classify-1402250-52.patch | 10.73 KB | lahoosascoots |
#49 | block-classify-1402250-49.patch | 10.47 KB | lahoosascoots |
#47 | block-classify-1402250-47.patch | 10.47 KB | lahoosascoots |
Comments
Comment #1
JacineWhy 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.
Comment #2
catchBumping 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).
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commentedPros 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.
Comment #4
Everett Zufelt CreditAttribution: Everett Zufelt commentedNote, 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.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedJacine: 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?
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.
Comment #6
Eric_A CreditAttribution: Eric_A commentedA) 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.
Comment #7
xjmA 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.
Comment #7.0
xjmUpdated issue summary.
Comment #8
tim.plunkettThere 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.
Comment #9
sunThe 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?
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedUm, 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:
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.
Comment #12
tim.plunketthook_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.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedI 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).
Comment #14
sunHm. 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.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedMy 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.
Comment #16
mgiffordAgreed 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.
Comment #17
mgifford#13: block-classify-1402250-13.patch queued for re-testing.
Comment #19
mgiffordMostly a re-roll, but what happened to plugin.core.block.seven.login.yml & plugin.core.block.bartik.login.yml?
Comment #20
mgifford#19: block-classify-1402250-19.patch queued for re-testing.
Comment #21
mgifford#19: block-classify-1402250-19.patch queued for re-testing.
Comment #22
mgifford#19: block-classify-1402250-19.patch queued for re-testing.
Comment #23
mgifford#19: block-classify-1402250-19.patch queued for re-testing.
Comment #24
mgiffordAn Interdiff failed to apply cleanly. This was the rejected code between 13 & 19:
Comment #25
mgifford#19: block-classify-1402250-19.patch queued for re-testing.
Comment #27
mgiffordJust 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.
Comment #28
mgifford#27: block-classify-1402250-27.patch queued for re-testing.
Comment #30
mgiffordre-rolled
Comment #31
shanly CreditAttribution: shanly commented#30: block-classify-1402250-30.patch queued for re-testing.
Comment #33
mgiffordTemplate was renamed for twig.
Comment #35
mgifford#33: block-classify-1402250-33.patch queued for re-testing.
Comment #37
mgifford#33: block-classify-1402250-33.patch queued for re-testing.
Comment #38
mgifford#33: block-classify-1402250-33.patch queued for re-testing.
Comment #40
mgiffordre-roll as diff in UserLoginBlock.php was rejected as existing function was dropped.
Comment #41
mgifford#40: block-classify-1402250-40.patch queued for re-testing.
Comment #42.0
(not verified) CreditAttribution: commentedAdded a third option
Comment #43
mgifford40: block-classify-1402250-40.patch queued for re-testing.
Comment #45
mgiffordreroll
Comment #46
mgiffordComment #47
lahoosascoots CreditAttribution: lahoosascoots commentedRe-rolled.
Comment #49
lahoosascoots CreditAttribution: lahoosascoots commentedAdding in missing Xss use.
Comment #50
lahoosascoots CreditAttribution: lahoosascoots commentedComment #52
lahoosascoots CreditAttribution: lahoosascoots commentedForgot to commit before diffing XD.
Comment #54
lahoosascoots CreditAttribution: lahoosascoots commentedBlockConfigSchema 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?
Comment #55
alansaviolobo CreditAttribution: alansaviolobo commentedComment #57
mgiffordWhat manual tests need to be done before this is marked RTBC?
Comment #58
mgiffordIn 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.
Comment #59
googletorp CreditAttribution: googletorp as a volunteer commentedComment #60
EclipseGc CreditAttribution: EclipseGc commentedThis 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
Comment #63
tim.plunkettNot actively part of the Blocks-Layouts work.
Comment #68
xeM8VfDh CreditAttribution: xeM8VfDh commentedis this still being worked on?
Comment #69
mgifford@xeM8VfDh I don't think someone is working on this now. Want to assign this to yourself and work on the patch from #55?
Comment #70
xeM8VfDh CreditAttribution: xeM8VfDh commentedIf i get time I will look into it!
Comment #71
xeM8VfDh CreditAttribution: xeM8VfDh commentedmaybe this is a solution? https://www.drupal.org/project/block_aria_landmark_roles
Comment #80
mgiffordtagging for https://www.w3.org/WAI/WCAG21/Understanding/name-role-value.html