Coming from #2045039-7: Cleanup HTML heading structure @terrill:
That said, I still think we should add aria-label attributes to all elements. There are many of these on each page, and without a label they're each identified by screen readers as "Navigation region" - there's no means of clarifying which navigation region is which. If we add aria-label, screen readers use the value of aria-label when identifying the region (e.g., "Toolbar Items Navigation Region", "Breadcrumb Navigation Region", and the like)
@falcon03 and later on:
Well, I think that hearing messages like "entering navigation", "entering main" and so on doesn't give a real "context". So, as a blind user, I'm completely in favor of adding aria-labels.
@pratikp1 and finally:
Having an effective label is essential. I would like some discussion about that for D8 or D9. Let's see if we can make it more understandable. It will go a long way toward having users understand that landmarks can actually be useful.
I don't know why there isn't any in Seven, but in Bartik, we can find the <nav>
element in:
core/themes/bartik/templates/comment.html.twig
core/themes/bartik/templates/page.html.twig
And in modules it's in:
core/modules/book/templates/book-all-books-block.html.twig
core/modules/book/templates/book-navigation.html.twig
core/modules/system/templates/breadcrumb.html.twig
core/modules/system/templates/page.html.twig
core/modules/toolbar/toolbar.module
Where else do we find them?
Some more info on using aria-labels:
http://www.last-child.com/debugging-aria-label/
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Reroll the patch if it no longer applies. | Yes | Instructions | |
Manually test the patch | Yes | Instructions | |
Add automated tests | Instructions |
Comment | File | Size | Author |
---|---|---|---|
#84 | 2052473-84.patch | 803 bytes | rpayanm |
Comments
Comment #1
falcon03 CreditAttribution: falcon03 commentedSo basically, we want to get a result like this: http://www.qcsalon.net
Ok, I admit that it is not a *really* technical website, but it offer a *stellar* user experience for a screen reader user by combining *old, traditional* techniques with the more recent ARIA ones.
I guess that if all the theme_functions->twig conversions & twig templates consolidation had been completed this task would have been easier. Anyways, let me give this a stab! :-)
As a screen reader user our current situation could be a pain in production; adjusting priority accordingly! :-)
Comment #2
falcon03 CreditAttribution: falcon03 commentedHere's a first stab at it (untested patch). In this comment you will find some remarks, let's get the discussion about them going! :-)
Regarding core/themes/bartik/templates/comment.html.twig: it uses a "nag" to wrap comment links, but the core-provided template does not. This is something to fix -- we have to avoid inconsistencies! But then the thing is -- What could an appropriate label be for the comment links? It is neither printed inside an heading, so maybe the better solution would be to completely remove the wrapper around comment links.
In the breadcrumb template, "You are here" in the heading makes sense, but does it make sense using it as a label as well? If not, we should use another string -- But I have no ideas! :-)
In core/modules/system/templates/page.html.twig the nag element is a wrapper for the main and the secondary menu. Maybe we'd better want to have a single nag for each menu. I'd vote for removing the nag container as it is now, since it is useless and print out each of both menus using a container. Also, tabs and action links are not wrapped in a container, but they should be -- Can we fix both these issues in this patch or do we need to split them in follow ups?
I'm inclined to deal with the toolbar in [1800614]. Are you fine with that?
I'm still convinced that theme_functions->Twig conversions and twig templates consolidation will reveal many other places to add aria-labels to and some other inconsistencies!
Comment #3
falcon03 CreditAttribution: falcon03 commentedTagging -- we strongly need themers feedback on this issue! :-)
Comment #4
falcon03 CreditAttribution: falcon03 commentedfixing tag.
Comment #5
falcon03 CreditAttribution: falcon03 commentedI can't stand d.o tags autocompletion any longer. Seriously! :-)
Comment #6
falcon03 CreditAttribution: falcon03 commentedComment #7
jessebeach CreditAttribution: jessebeach commentedAs far as code goes, it looks great.
Comment #8
bowersox CreditAttribution: bowersox commentedThe Drupal 8 accessibility testing sessions in Minneapolis proved that this is important--users were confused that Drupal has so many role=Navigation regions, and it is not clear which is which. So thanks for the work on this important issue!
Comment #9
falcon03 CreditAttribution: falcon03 commented@bowersox: yeah, I really agree -- we absolutely need to solve this issue before releasing D8. I changed priority to major, but feel free to set it to critical if you want to. :-)
I can finish the patch easily, but I need feedback for remarks in #2 first. :-)
Comment #10
mgiffordThese are numbered responses from #2 above. I've quoted them for simplicity.
1) Here's a first stab at it (untested patch).
It seems to be working where I have tested it. I've added screen shots.
2) Regarding core/themes/bartik/templates/comment.html.twig: it uses a "nag" to wrap comment links, but the core-provided template does not. This is something to fix -- we have to avoid inconsistencies! But then the thing is -- What could an appropriate label be for the comment links? It is neither printed inside an heading, so maybe the better solution would be to completely remove the wrapper around comment links.
I'm for simplifying things if we can. If we are just repeating text, I don't know that it really is going to be adding much if any context by adding the aria-label. Wouldn't it just get annoying to hear them twice all the time?
3) In the breadcrumb template, "You are here" in the heading makes sense, but does it make sense using it as a label as well? If not, we should use another string
Can we skip it in that case? We don't want to remove the heading as that is used for navigation, but is there a good reason to paraphrase or duplicate it everywhere? If so maybe we have to dig a bit deeper to figure out what the function of these links is. Alternatively, maybe some of these will have to be defined per-site and we should simply make it easier for the developer to add them after the fact.
4) In core/modules/system/templates/page.html.twig the nag element is a wrapper for the main and the secondary menu. Maybe we'd better want to have a single nag for each menu. I'd vote for removing the nag container as it is now, since it is useless and print out each of both menus using a container. Also, tabs and action links are not wrapped in a container, but they should be -- Can we fix both these issues in this patch or do we need to split them in follow ups?
I'm in favour of anything we can do to make it simpler.
5) I'm inclined to deal with the toolbar in #1800614: Improve the responsive toolbar accessibility. Are you fine with that?
Yes. I noted that on the other issue too.
6) I'm still convinced that theme_functions->Twig conversions and twig templates consolidation will reveal many other places to add aria-labels to and some other inconsistencies!
Agreed. It's all going to have to be reviewed many more times I'm afraid.
So far, pulling out the proposed changes we have:
Comment #11
agileadamFor what it's worth, you can add these attributes using preprocess functions as necessary.
For example, my nav elements are typically menu_block and nice_menus blocks.
Here's how I add the aria-label attributes to these blocks.
You can do similar for other elements in themename_preprocess_page() and elsewhere. I set a breadcrumb aria-label in my mytheme_breadcrumb() override function by modifying the HTML within that function.
Anyways, sorry to muddy-up this issue; I just figured other people would land here looking to do similar.
Comment #12
BarisW CreditAttribution: BarisW commentedI really like this idea. The patch in #2 works fine, I've only changed made the casing of the first and second words equal (like Book navigation instead of Book Navigation).
Comment #13
mgifford@BarisW what do you think of my response to @falcon03's points in #2. Where else do we need more clarification than what I provided in comment #10?
Comment #14
BarisW CreditAttribution: BarisW commentedApologies, haven't read you comment properly. This issue needs an updated description ;)
I'll come back to this issue later, I agree with most of your points.
Comment #15
BarisW CreditAttribution: BarisW commentedOk, so having read this all over, I believe we make a mistake. As far as I understand from the specs, the aria-label should be used to give information to a section in cases when there's no visible label. The discussion here is the meaning of 'visible'. We use the 'visually-hidden' class in favor of the 'element-invisible' class exactly for this reason already; so that screen readers will still read out the labels.
In the example of the OP, the only places where aria-labels were added are the regions, like Main Content, or places where the list (breadcrumbs) didn't have a heading already.
I think that adding labels on places were we already output headings would only end up in having speach readers repeating themselves. Better would be to use the aria-labelledby attribute, but that would result in having to add heading ID's all over the place.
Would it work if we add the aria-labels like in the current patch, but replace the visually-hidden with element-invisible again? So that speach readers will only read out the aria-label element?
So we need some input from Everett?
Comment #16
mgiffordWe definitely don't need to wait for Everett (as much as I do really value his insights)..
But let's see if we can get some real world experience chipping in on how to apply this properly so that it works right! I've reached out to a few folks in the accessibility community and hope to get some input.
Comment #17
mgifford#12: aria_labels-2052473-12.patch queued for re-testing.
Comment #18
mgiffordI think adding aria-labelledby tags to heading tags with ID's would be acceptable. We'd just need to build a strong case for it. I think that would be probably better than the duplication.
Comment #19
jessebeach CreditAttribution: jessebeach commentedUsing a string like this results in the output: "You are here navigation". It strikes me that we should be more explicit about what this navigation element is. Maybe just Breadcrumb resulting in: "Breadcrumb navigation".
The way I've been approaching these labels is to think "given an out-of-context list of landmarks, what kind of label will inform me quickly about the purpose of this region? What label will make me understand if I need to click into it or just ignore it given my goal at the moment?"
Comment #20
jessebeach CreditAttribution: jessebeach commentedI'd really like to see us be as specific as possible with labels. If something has a title, then that title should be used in the label or heading. The label "Secondary menu" is not very helpful. A label like "Account actions" is helpful because it describes what types of links are might be found in the menu. So, when we have something like this:
The utility of the label is very low. I'm not criticizing the work that has been done already. No no. What I'm doing is challenging us to make these labels really useful; I don't want to simply be checking off accessibility boxes here. Let's make great interfaces!
So instead, what we really want is HTML like this:
The label pertains to the particular menu being printed here.
Can we do this? Yes, I think we can. I've gone through the Book module and made similar changes in the attached patch.
The goal we're aim for is, can someone reasonably understand the landmark labels absent of a specific content? Enough to choose between them with some degree of confidence?
This patch also contains a small fix, represented in #2124287: Book module's BookNavigationBlock passes poorly formatted variable data to the book-all-books-block template, so when that gets committed, we'll have a tiny conflict to resolve.
Comment #21
jessebeach CreditAttribution: jessebeach commentedThis patch still needs work. Let's make it awesome!
Comment #22
jessebeach CreditAttribution: jessebeach commentedThis also is no feature request. This is UI work. Non-visual UI work.
Comment #23
jessebeach CreditAttribution: jessebeach commentedfrom #15:
My experience using VoiceOver is that headings and aria labels are two parallel systems. Either one is navigating by headings or one is navigating by landmarks. I don't run into duplicated output. I do prefer to refer to the same strings, so where we can, we should be using
aria-labelelledby
so that strings don't diverge over time.Comment #24
mgiffordThis is really great Jesse, thanks. I really like where this is going.
I was looking at the new Drupal.org and wondered if it might be possible to apply some of these ideas to the theme there so that we can get more feedback from users in a real world environment. Not that we should hold off on that, but it would be a great case study.
I like the use of aria-labelelledby, simply so that there is less translation required. The only concern was the re-introduction of ID's.
Comment #25
mgiffordPatch in #20 didn't apply any longer.
Comment #26
mgiffordThe semantic relationship between the title and the navigation should be established by default if it exists. I think this will work if inersted at the bottom of template_preprocess_block().
Comment #27
mgiffordOk, this needs more thought... Can't just move it over from AdaptiveTheme as I'd hoped.
Comment #28
mgiffordSorry for the noise.. Think I needed to use $variables['elements']['#block']
Comment #29
mgiffordNope.. Bit closer, but i have to get my laptop back from the shop so I can test this easier....
Comment #30
mgiffordI'd like to use to more
aria-labelledby
simply so that there is less redundant code and less processing.Anyways, this patch addresses the notice error. I haven't found an example where my efforts to proactively add
aria-describedby
where possible actually worked mind you. There's been a real effort to remove ID's and so lots of places to add them back in if we're going to use this approach.Comment #31
mimes CreditAttribution: mimes commented30: aria_labels-2052473-30.patch queued for re-testing.
Comment #32
mgifford30: aria_labels-2052473-30.patch queued for re-testing.
Comment #33
mgiffordComment #34
Jalandhar CreditAttribution: Jalandhar commentedUpdating patch with reroll. Please review.
Comment #36
lokapujyaComment #37
mgiffordPatch looks nice. The bot likes it too. There are no UI changes. I attached some screen shots.
Comment #38
jessebeach CreditAttribution: jessebeach commentedCould someone quickly explain why the key in the $variables array is 'title_attributes' on one line and then 'title_attributes_array' on the next line. Is this some kind of $variables array magic?
Comment #39
mgifford@jessebeach - I'd say it was definitely troll magic....
Thanks for catching that. Here's a patch with the correction.
Comment #40
LewisNyman CreditAttribution: LewisNyman commentedComment #41
dcam CreditAttribution: dcam commentedUpdating in preparation for DrupalCon Austin sprints. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead....
#39 does not apply to HEAD and need a reroll.
Comment #42
ricky.middaugh CreditAttribution: ricky.middaugh commentedWill reroll the latest patch.
Comment #43
ricky.middaugh CreditAttribution: ricky.middaugh commentedHere's the rerolled patch.
Comment #45
ricky.middaugh CreditAttribution: ricky.middaugh commentedIt looks like the problem had to do with missing a node title when trying to generate a block for the current book.
This should fix the failing tests.
Comment #46
ricky.middaugh CreditAttribution: ricky.middaugh commentedComment #48
ricky.middaugh CreditAttribution: ricky.middaugh commentedDangit! I thought that would do it. I'll keep trying to look at this.
Comment #49
dcam CreditAttribution: dcam commentedHi ricky.middaugh! Thanks for helping out with this issue! You've done a great job so far. I think I've figured out where the problems are.
First, the change to core/modules/book/lib/Drupal/book/Plugin/Block/BookNavigationBlock.php doesn't look quite right. It should only have one change, the second one, and it should be one line lower, outside that closing curly bracket.
Second, your patches are missing the changes to core/modules/system/templates/breadcrumb.html.twig.
I would suggest starting from scratch and trying a new reroll of #39, but you can build on your earlier work if you prefer. The instructions for using the Git command line to reroll patches can be found at https://drupal.org/patch/reroll.
Whenever I reroll a patch I like to look at the original patch and my rerolled version side by side. Then I compare them line by line. This helps me make sure that I haven't missed or accidentally added anything. In the case of a patch like this, the two should be nearly identical except for the filepaths, as most or all of the reason the reroll was needed was due to the change in directory structure. The surrounding code probably hasn't changed at all.
Let me know if you need any help!
Comment #50
ricky.middaugh CreditAttribution: ricky.middaugh commentedUgh, figured it was something simple like that.
Here's another reroll for up to the newest commit.
Comment #51
mgifford@ricky.middaugh this passes, which is good, but
aria-labelledby="links__system_main_menu"
needs to point to an ID (and I don't think it does right now.).I'm also not sure what happened to all of the other
nav
elements which were highlighted above...Comment #52
mgiffordComment #53
ricky.middaugh CreditAttribution: ricky.middaugh commented@mgifford The
links__system_main_menu
id is set it core/includes/theme.inc intemplate_preprocess_page()
, set on the main menu (Same with the secondary menu in the same fashion).That patch was from a merge conflict I ran into, this one should include all the changes.
Comment #54
mgiffordOk, this looks good to me. Here are the screenshots:
Breadcrumbs
Tabs
Secondary Nav
Toolbar
Book Module
Comment #55
joelpittetAdded a quick follow up to this to remove that deprecated 'class' attribute on header for #theme links.
#2285493: Remove deprecated 'class' property from #theme 'links' and #theme 'menu_tree' heading arrays
Comment #56
webchickOk, great!
Committed and pushed to 8.x. a11y for the win. :)
Comment #58
mgiffordExcellent! Glad it's in.
Comment #59
mgiffordComment #60
tim.plunkett$block->subject has been gone since #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer, so this neither works nor has test coverage.
Comment #61
joelpittetThanks for pointing that out @tim.plunkett and just to keep y'all are in the loop there are attempts to change that to title instead of label in the theme layer to keep some consistency with title_suffix/prefix and title_attributes.
#1939224: Change block "label" so that it's called a title like everything else in the template file (and all other template files)
#1939234: Change node "label" so that it's called a title like everything else in the template file (and all other template files)
Comment #62
BerdirPosted a possible fix for this in #1989568: Remove block config ID from being used as an HTML ID or template suggestion, but still missing tests. Also didn't find any 'role' attributes in core, so I don't think this would be printed anywhere even with the right check.
Comment #63
mgiffordOk, so there's this change here (which is applied to HEAD now). And we need tests.
It's not clear to me how we should proceed. Tests are always good, but I'm no good at writing them.
@tim.plunkett - thoughts on how to proceed?
@joelpittet & @Berdir - always good to know about related issues.
Comment #64
tim.plunkettThere is no $block->admin_label either. That's part of the block's plugin definition.
Manually testing this would be a start, since this 100% does not work.
Writing a web test for it is the next step.
Comment #65
lauriiiComment #66
mgiffordProbably the bigger issue is that the block module doesn't include any
<nav>
code. In which case, looks like there is some code which can be removed. It could be that it should be addressed elsewhere, but this is focused on this one HTML tag.core/themes/bartik/templates/comment.html.twig
This should probably be labelled as "Related links" or something like that:
core/modules/system/templates/page.html.twig
Looking at this page, I'm wondering if this might be a mistake too. There is no consideration for if there is no main_menu but just a secondary_menu.
This should probably also be identified as "Local actions" but this could be clarified.
Comment #68
mgiffordComment #69
mgiffordComment #72
mgiffordComment #73
lauriiiI think this issue is at least some how related to this #1869476: Convert global menus (primary links, secondary links) into blocks
Comment #74
mgiffordWorth looking into...
Comment #75
LewisNyman CreditAttribution: LewisNyman commentedNo longer novice
Comment #76
Rade CreditAttribution: Rade commentedI rerolled the patch from #63 following the instructions from "Re-rolling patches". Fixed the conflict manually.
I don't think this new patch makes much sense, still it's using the admin_label that according to @tim.plunkett doesn't exist anymore, but at least this patch applies cleanly, and maybe it's easier to pick up from.
Thorough manual review needed!
Comment #77
joelpittetThanks @rade tagging as needs manual testing
Comment #80
mgiffordOn the home page with a default install I can't see any change to the number of aria-label references before or after this patch.
There are blocks enabled on the home page, but I can't see where this patch is changing the output at the moment.
Is there an instance where
<nav>
is defined that doesn't have an aria attribute associated with it that we know of?Perhaps we should mark this issue postponed until we find a good way to test it.
Comment #83
mgiffordComment #84
rpayanmComment #86
leandro713 CreditAttribution: leandro713 as a volunteer commentedthe patch works for me.
i can see nav tags with the attribute aria-labelledby for the user menu and every block
Comment #88
mgifford@leandro713 looking at the patch this is about proactively adding aria-describedby if possible. Not aria-label.
@rpayanm do you have any sense as to where we would see this in action?
Comment #92
BarisW CreditAttribution: BarisW at LimoenGroen commentedI have no idea what this piece of code does. On most places (menu blocks, navigation, breadcrumb and pager) the labelled-by attribute is added in the twig files.
So what is the purpose of this piece of code anyway?
Comment #93
mgiffordThe purpose of it (from what i recall) is to make sure that we're associating related elements with aria-describedby in a programmatic way. Theoretically we'd see it in the label tags where there is a description element.
As I said in #88, I don't know where this appears, if anywhere. It may not appear and as you stated, we may be doing this in twig files anyways.
Hope that's clear.
Comment #94
Thew CreditAttribution: Thew at Google Code-In commentedIs this issue still needs review or manually testing?
I tested the patch from #88 in many sections and pages and see there is no different before and after apply a patch.
Comment #95
mgiffordI think we need a few examples where aria-label or aria-describedby attributes aren't added to elements first.
This issue might just be too generic.
Looking through the code I could only find two instances from the templates:
Thanks @Threw
Comment #96
oscar_wong67 CreditAttribution: oscar_wong67 as a volunteer commented@mgifford, is aria-label/aria-describedby always meant to be added as the first attribute on a tag according to the Drupal code style? Could it just be enforced and maybe noted somewhere?
Could this maybe be simplified by automatically creating aria-label attributes when none are specified, based on text? I know this was done with the Angular Material project (https://github.com/angular/material)
Comment #97
mgiffordI'm pretty confident that the Drupal code style doesn't define this.
I don't think we can just run with the first attribute on a tag approach.
How does the Angular Material automatically create the
aria-label
and is it always accurate?It is unclear to me that there will be a single purpose for the
<nav>
in block--local-actions-block.html.twigComment #106
andypostFeatures should go to 9.1 now
Comment #110
quietone CreditAttribution: quietone at PreviousNext commentedI'm following up on issues that have been committed and re-opened.
This issue was committed to Drupal 8.x in Jun 2014 and re-opened in #60 due to needing tests and a question about the implementation. The last discussions were 5 years ago.
Is there anything still to do here? If so, update the Issue Summary and add a comment.
Thanks
Edit: fix typo
Comment #112
quietone CreditAttribution: quietone at PreviousNext commentedThere hasn't been any further information provided so I am restoring the Fixed status, originally set in #56 at the time of the commit to Drupal 8.
Cheers.