Updated: Comment #80
Problem/Motivation
(Why the issue was filed, steps to reproduce the problem, etc.)
Proposed resolution
(Description of the proposed solution, the rationale behind it, and workarounds for people who cannot use the patch.)
Remaining tasks
(reviews needed, tests to be written or run, documentation to be written, etc.)
User interface changes
(New or changed features/functionality in the user interface, modules added or removed, changes to URL paths, changes to user interface text.)
API changes
(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate.)
Related Issues
(A list of related issues.)
Original report by mgifford
The links in the various '.contextual-links-wrapper' divs are not accessible at all via keyboard alone, when not using a screen reader. However, when using a screen reader, these links are announced, but not displayed, since they only appear in response to a mouseover Since many screen reader users are also sighted, this can cause confusion when the screen reader is announcing content or links that are nowhere to be found on the page.
Comments
Comment #1
mgiffordCut/Paste error, that should continue by saying "... nowhere to be found on the page"
Comment #2
casey CreditAttribution: casey commentedComment #3
casey CreditAttribution: casey commentedForgot :active and :focus on "ul.contextual-links li a" in contextual.css.
Comment #4
mgiffordI've got #3 applied here - http://drupal7.dev.openconcept.ca/
This was reported to me via @jasonkiss and I haven't actually stumbled across this module until yesterday.
Where would I test this patch?
Comment #5
casey CreditAttribution: casey commentedhttp://www.screencast.com/users/caseyn/folders/Jing/media/2c23e5a4-9b90-...
(Using TAB to navigate and ENTER to open links)
Comment #6
Everett Zufelt CreditAttribution: Everett Zufelt commentedSetting to critical as certain users cannot use links wrapped in .contextual-links
Comment #7
mgifford@casey that's a great screencast. Is that with the applied patch. Thanks for pointing out the contextual links. Now you can navigate with the keyboard (which is good). I also tested this in VoiceOver and it seemed to work fine there too.
This produces no visible changes. I think it should have a review of the javascript though before being marked RTBC.
Comment #8
Jeff Burnz CreditAttribution: Jeff Burnz commentedThis is not critical.
Comment #9
cwgordon7 CreditAttribution: cwgordon7 commentedPatch works, the javascript looks fine to me, and nothing breaks, so this is rtbc.
Comment #10
casey CreditAttribution: casey commentedOn second thoughts the patch is quite buggy... working on it.
Comment #11
Everett Zufelt CreditAttribution: Everett Zufelt commentedSetting back to critical. The inability to use certain links is a critical bug as far as I can tell. If this is not critical I would appreciate an explanation so that I can better understand for the future.
Comment #12
sunThese should use proper formatting, i.e., be on separate lines.
Powered by Dreditor.
Comment #13
mgifford@casey any comments on how it's buggy other than @sun's comments?
Comment #14
sunThis issue cannot be critical, since all of the functionality exposed via contextual links is also available through other means (i.e., without contextual links). For that sake, I believe it's not even major, but will stick to that for now.
Comment #15
mgiffordbumping this issue by adding a tag.
Regardless of whether or not it is a critical or major issue it should be dealt with.
Comment #16
mgifford#3: contextual-keyboard.patch queued for re-testing.
Comment #17
mgiffordCan we bring this into the http://drupal.org/project/accessible module?
Since this type of change improves accessibility for some users by making things easier to accomplish with less clicks, it would be useful. However, I'm not sure we're going to be able to address this change in D7 unless we get more support for this css/js change.
Comment #18
sun.core CreditAttribution: sun.core commentedComment in #12 still stands.
Comment #19
Everett Zufelt CreditAttribution: Everett Zufelt commentedPlease do not change issue priority without a strong rationale.
No keyboard only users can use these links. I consider this to be strong rationale for Major priority. Since the links are not required for Drupal to run it is not Critical.
Furthermore, as an accessibility issue I consider this to be an issue that I have responsibility for. I expect that the priority will not change again,
if it does I expect a strong rationale.
Comment #20
mgiffordCan someone re-work the original patch to fix @sun's formatting concerns?
Comment #21
mgifford#3: contextual-keyboard.patch queued for re-testing.
Comment #22
Everett Zufelt CreditAttribution: Everett Zufelt commented@Sun
I read through the Javascript coding standards and didn't find what you are referring to.
+ function () { $region.addClass('contextual-links-region-active'); },
Can you please confirm if this is correct:
Comment #23
Everett Zufelt CreditAttribution: Everett Zufelt commentedBumping to critical as per Webchick's direction in beta3 release notes.
Comment #24
EvanDonovan CreditAttribution: EvanDonovan commentedI don't think this is actually critical, since, as sun said, the links are also accessible by other means, but I won't bump it back down. webchick's comments in #974066: Friendly reminder: on critical issues and RCs might apply here though.
casey, what about the patch is buggy? Your comment that it was buggy and you were working on it seems to be the last statement made about the patch, other than the formatting issues, which seem minor.
Comment #25
Jeff Burnz CreditAttribution: Jeff Burnz commented@22, regarding Everetts coding standard example - of the JS I've looked at in core the standard appears to be as per your example.
Comment #26
webchickNope. This isn't critical. There is nothing you can do in contextual links you can't do elsewhere in the admin interface. The confusing behaviour sounds like a bug, but a normal one. It doesn't stop anyone from using Drupal.
Comment #27
mgifford#3: contextual-keyboard.patch queued for re-testing.
Comment #29
JacineThe patch no longer applies.
Comment #30
Everett Zufelt CreditAttribution: Everett Zufelt commented#3: contextual-keyboard.patch queued for re-testing.
Comment #31
JacineHere's a reroll that takes @sun's comments in #12 into account.
In testing this patch I found that when clicking (by hitting enter) one of the links, the
.contextual-links-region-active
class does not go away, and the contextual links are visible on top of the overlay. Screenshot of that is attached.I had to reroll manually (and I'm not sure why because it seems like it should have applied), so it's possible I messed something up.
Comment #32
mgiffordThis looks great Jacine. Looks like the outstanding concerns have been addressed.
It works great. I tested this in FF, Chrome & Safari.
Comment #33
Jacine@mgifford Take a look at the screenshot in #31. That shouldn't happen. The
.contextual-links-region-active
class should go away when you click one of the links, so maybe that's what @casey alluding to as being buggy.I'm not sure how to fix it, but it needs to happen before this is RTBC.
Comment #34
sun@jacine: Is the additional bug you mentioned in #31 already fixed in the patch of #31 or does it still need to be fixed?
Comment #35
JacineStill needs to be fixed.
Comment #36
mgiffordAhh.. Really sorry. I didn't read that carefully enough. In my experiments it worked, but I've been able to recreate the problem you mentioned (and showed)..
In both editing a node & a block I can replicate it:
http://drupal7.dev.openconcept.ca/#main-content=&overlay=node/8/edit%3Fd...
http://drupal7.dev.openconcept.ca/#overlay=admin/structure/block/manage/...
Although in editing the node it's just over top of the grey background.
I think this is likely a problem with overlay. I suspect that there's something that's not taking into account that a background item is still in focus. I'm not sure though and not sure how to resolve it.
If we could just change the focus on click then perhaps that would resolve the problem, but I'm not sure how to do that or what problems that might cause.
Comment #37
mgiffordtagging
Comment #38
mgiffordmaking a d8 issue.
Comment #39
casey CreditAttribution: casey commentedSome code to work from.
Comment #41
mgiffordLet's just say this module has changed a whole lot since September.
Probably needs to be completely reviewed.
Comment #42
Bojhan CreditAttribution: Bojhan commentedMarking this back to major, by most of the standards we have applied in the past two years if functionality even in its bare essentials isn't accessible, we mark it major. Here we have a module, that simply cannot be used by keyboard users - to me that is major?
Would love it this could see a reroll.
Comment #43
mgiffordTagging. Hopefully we can get someone to re-roll this weekend.
Comment #44
Mirabuck CreditAttribution: Mirabuck commentedMaking some progress re-rolling a patch at the Montreal sprint. hope to have something to post shortly.
Comment #45
Mirabuck CreditAttribution: Mirabuck commentediwayMan and I have re-rolled a patch using casey's #39 code as a starting point.
Comment #46
Mirabuck CreditAttribution: Mirabuck commentedAfter running through the UI with a screen reader this morning I realized that there wasn't anything indicating to screen readers that the contextual links menu was in an open or closed state. Added some JS to adjust the trigger link's title to reflect this. After speaking to bowersox I've also added some aria attributes to the div with the contextual class that surrounds this trigger in hopes that this will provide more immediate feedback to screenreader users when the title attribute updates.
New patch including everything from #45 attached for review.
Comment #47
mgifford#46: contextual-contextual_links_keyboard_accessibility-849926-46.patch queued for re-testing.
Comment #49
mgiffordHere's a re-roll of the patch above.
Comment #50
Mirabuck CreditAttribution: Mirabuck commentedTagging for DrupalCamp Montreal 2012 Sprint
Comment #51
Mirabuck CreditAttribution: Mirabuck commentedI've tested the keyboard functionality and verified that it's working as expected.
Comment #52
delmarr CreditAttribution: delmarr commentedTested with the voiceover utility in mac osx
* When tabbed to the gear - audio announces you are on a closed menu list
* Configure gear is accessible to screen readers and keyboards
Comment #53
ry5n CreditAttribution: ry5n commented@webchick asked for markup/css review, esp. re: a backport to D7. CSS looks good, plus simpler selectors which is very nice. Rendering is consistent when tested in Chrome and Firefox. There are style rules for IE 6/7 support, so should be easily portable to D7.
Has this been tested by any adept screenreader users? I noticed no barriers to access when using VoiceOver (OS X) in my testing, but there may be opportunities to further enhance this using additional aria attributes designed for dropdown elements.
Comment #54
Everett Zufelt CreditAttribution: Everett Zufelt commentedIs there context for exactly what this means? What happens when this is activated?
Comment #55
Mirabuck CreditAttribution: Mirabuck commentedThe gear icon is used to represent a contextual menu link. As I understand it, the screen reader (voiceover at least) announces that the menu is closed when a user first tabs to it. Voice over then announces that the menu is open when a user uses the mouse or enter key to open the menu. The user then has to tab through the newly opened menu to get to the various contextual options.
Comment #56
mgifford#49: contextual-contextual_links_keyboard_accessibility-849926-49.patch queued for re-testing.
Comment #57
jessebeach CreditAttribution: jessebeach commentedThis is really great work so far. I wanted to go through the JS and put in core patterns that we've been using lately. I've also reduced the amount of code in both the JS and the CSS files. Mirabuck, I followed your example with attaching to focusin and focusout events for the keyboard input. I also used the core
element-invisible
andelement-focusable
classes for hiding the contextual gears rather than using custom CSS in this module for that purpose.I rerolled on the current 8.x HEAD (01567a63), which includes #1138844: Add touch support to contextual links. The merge with this patch wasn't pretty and I'd like to look over it a bit more, but I wanted to get what I have posted so others can have a look.
The most significant change is moving the event binding to the region element and delegating user events from it:
Also,
Drupal.contextual.prototype.highlightRegion
now contains a small timeout delay so a user can user keyboard input between the contextual links trigger and items in the contextual links without the links dismissing and reappearing.I also moved element creation into theme functions.
Theme and base CSS are now completely separated as well. Base contains only enough information for the basic behavior of the contextual links.
Comment #58
mgiffordI'm not going to evaluate your JS/CSS changes, but did a quick review to see if the patch applies & works for a keyboard only user. It does. Finally one can navigate to the contextual links without a mouse.
Comment #59
mgifford#57: contextual-contextual_links_keyboard_accessibility-849926-57.patch queued for re-testing.
Comment #60
mgiffordI've confirmed that this works in Chrome & Safari with VoiceOver. This is ready to RTBC as far as accessibility goes. Will try to get someone to review the JS now.
Comment #61
attiks CreditAttribution: attiks commentedI only had a quick look
looks like some of the comment is missing
Comment #62
jessebeach CreditAttribution: jessebeach commentedRerolled on latest HEAD (bb91b33df84609af587a7673a9a5a8fe0b0e6cbe).
Comment #63
mgiffordAlso occurred to me that it would be better if you also added the HTML5 hidden attribute so that the information was state was passed semantically.
Do you still need an additional JS review?
Comment #64
jessebeach CreditAttribution: jessebeach commentedI'm working on this this morning.
Comment #65
Wim LeersComment #66
jessebeach CreditAttribution: jessebeach commentedThis patch removes the stray comment attiks found in #61.
I added [hidden] information that mgifford #63 and cleaned up the aria attributes on the contextual links. They were making the screen reader a bit too chatty.
I noticed that the color of the contextual links was being set to white in the Bartik footer. I altered the CSS to prevent alteration to the contextual links color.
We probably should have a JS review for this patch, although there isn't too much complicated going on here.
Comment #67
webchickTagging w/ JavaScript for that.
Comment #68
jessebeach CreditAttribution: jessebeach commentedFixed a small issue with
window.setTimeout
that I discovered while integrating this patch with some other features.I needed to clear the timeout, not just the integer representing it, to avoid a infinite loop of mouseleave being called.
Comment #69
sunre: #68: The details you just provided in the comment actually belong into the code as an inline comment. ;)
Comment #70
jessebeach CreditAttribution: jessebeach commentedGood point. Added the comment.
Comment #71
mgiffordIt is RTBC for everything we've asked for, but sadly I'd like to ask for one more thing.
When using VoiceOver to navigate the site there's no way of knowing which box you're gaining admin access to.
By default there is the Tools & Search boxes enabled, but tabbing through the links skips the headings & jumping through the headings doesn't switch the focus so that you can just navigate from there to the contextual links.
I think the way around this is to just add some text that says "Open configuration options button for Tools block" or "Open configuration options button for Search block".
I'm not sure the best way to do this, but identifying the associated block in along with the text would provide context which is otherwise missing.
I'm tempted to just set up a new issue for that given how close we are, but...
Comment #72
Gábor Hojtsy@mgifford: can we really set up a different issue for that? :) This blocks other important work to unify contextual links with in-place editing #1882482: [meta] Unify editing approaches in Drupal and would be important to get in. We'll probably find other tasks especially with the adjustments made through that issue, so things will change again soon. Thanks for your very diligent review :)
Comment #73
mgiffordI just opened #1905340: Contextual Links Needs Title to be Included in "Open configuration options button" to take on this issue.
I think it's acceptable to refer this to another issue for a couple of reasons:
1) It's a basic text change and shouldn't be affected by the feature freeze as I understand it
2) I use VoiceOver in a very basic way and a more experienced user would probably have a few ways to work around this and know more where they are in the page. It's right after a heading for the block after all.
3) I haven't tested it in JAWS so it might work fine there.
4) The problem initially was one of having basic access to this widget. It can be navigated now so although it's not perfect, it's way better than what we shipped in D7.
Comment #74
jessebeach CreditAttribution: jessebeach commentedI'm following the follow-up. I feel like I'm finally reaching a point of comprehension when it comes to building keyboard accessible and aural UIs. I look forward to improving more of Core!
Comment #75
webchickYeah, I agree that solving the major bug report here is important; follow-ups to make it moar betterer are always appreciated, but at least they won't hold back features.
Committed and pushed to 8.x. Thanks!
Comment #76
echoz CreditAttribution: echoz commentedRelated - #1907236: contextual module introduced horizontal scrollbar
Comment #78
David_Rothstein CreditAttribution: David_Rothstein commentedThis doesn't look like it ever made it into Drupal 7. Hopefully there's a good way to backport it.
Comment #79
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #80
star-szrAt a local sprint last Saturday we had some folks looking at backporting this patch. Because contextual.module got refactored quite a bit in D8 this is not a simple backport. That's not to say that it's not doable, but just that it's may not be a good novice task.
Comment #80.0
ehansin CreditAttribution: ehansin commentedInitial migration to new Issue Summary template.
Comment #85
steinmb CreditAttribution: steinmb as a volunteer commented