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

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

Files: 
CommentFileSizeAuthor
#71 OpenConfigOPtions.png102.9 KBmgifford
#70 contextual-contextual_links_keyboard_accessibility-849926-70.patch14.47 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 49,317 pass(es).
[ View ]
#68 contextual-contextual_links_keyboard_accessibility-849926-68.patch14.46 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 49,448 pass(es).
[ View ]
#68 interdiff_66-68.txt1.13 KBjessebeach
#66 contextual-contextual_links_keyboard_accessibility-849926-66.patch15.29 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 49,340 pass(es).
[ View ]
#66 interdiff_60-66.txt5.03 KBjessebeach
#62 contextual-contextual_links_keyboard_accessibility-849926-60.patch13.44 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es).
[ View ]
#58 Screen Shot 2013-01-02 at 10.20.28 AM.png63.57 KBmgifford
#57 contextual-contextual_links_keyboard_accessibility-849926-57.patch13.4 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 50,689 pass(es).
[ View ]
#49 contextual-contextual_links_keyboard_accessibility-849926-49.patch8.42 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 48,120 pass(es).
[ View ]
#46 contextual-contextual_links_keyboard_accessibility-849926-46.patch8.15 KBMirabuck
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch contextual-contextual_links_keyboard_accessibility-849926-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 contextual-contextual_links_keyboard_accessibility-849926-45.patch7.86 KBMirabuck
PASSED: [[SimpleTest]]: [MySQL] 37,004 pass(es).
[ View ]
#39 contextual.patch7.77 KBcasey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch contextual_1.patch. See the log in the details link for more information.
[ View ]
#31 849926-31.patch4.93 KBJacine
PASSED: [[SimpleTest]]: [MySQL] 27,633 pass(es).
[ View ]
#31 contextual-links.png58.51 KBJacine
#3 contextual-keyboard.patch4.56 KBcasey
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch contextual-keyboard_0.patch.
[ View ]
#2 contextual-keyboard.patch4.33 KBcasey
PASSED: [[SimpleTest]]: [MySQL] 22,128 pass(es).
[ View ]

Comments

Cut/Paste error, that should continue by saying "... nowhere to be found on the page"

Status:Active» Needs review
StatusFileSize
new4.33 KB
PASSED: [[SimpleTest]]: [MySQL] 22,128 pass(es).
[ View ]

StatusFileSize
new4.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch contextual-keyboard_0.patch.
[ View ]

Forgot :active and :focus on "ul.contextual-links li a" in contextual.css.

I'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?

Priority:Normal» Critical

Setting to critical as certain users cannot use links wrapped in .contextual-links

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

Priority:Critical» Normal

This is not critical.

Status:Needs review» Reviewed & tested by the community

Patch works, the javascript looks fine to me, and nothing breaks, so this is rtbc.

Status:Reviewed & tested by the community» Needs work

On second thoughts the patch is quite buggy... working on it.

Priority:Normal» Critical

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

+++ modules/contextual/contextual.js 10 Jul 2010 14:04:08 -0000
@@ -12,33 +12,40 @@
+        .focus(function () { $region.addClass('contextual-links-region-active'); })
...
+          function () { $region.addClass('contextual-links-region-active'); },
+          function () { $region.removeClass('contextual-links-region-active'); }

These should use proper formatting, i.e., be on separate lines.

Powered by Dreditor.

@casey any comments on how it's buggy other than @sun's comments?

Priority:Critical» Major

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

bumping this issue by adding a tag.

Regardless of whether or not it is a critical or major issue it should be dealt with.

Status:Needs work» Needs review

#3: contextual-keyboard.patch queued for re-testing.

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

Priority:Major» Normal
Status:Needs review» Needs work

Comment in #12 still stands.

Priority:Normal» Major

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

Can someone re-work the original patch to fix @sun's formatting concerns?

Status:Needs work» Needs review

#3: contextual-keyboard.patch queued for re-testing.

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

+          function () {
+            $region.addClass('contextual-links-region-active');
+          },

Priority:Major» Critical

Bumping to critical as per Webchick's direction in beta3 release notes.

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

@22, regarding Everetts coding standard example - of the JS I've looked at in core the standard appears to be as per your example.

Priority:Critical» Normal

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

#3: contextual-keyboard.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+accessibility, +needs JavaScript review

The last submitted patch, contextual-keyboard.patch, failed testing.

The patch no longer applies.

Status:Needs work» Needs review

#3: contextual-keyboard.patch queued for re-testing.

StatusFileSize
new58.51 KB
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 27,633 pass(es).
[ View ]

Here'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.

Status:Needs review» Reviewed & tested by the community

This looks great Jacine. Looks like the outstanding concerns have been addressed.

It works great. I tested this in FF, Chrome & Safari.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review

@jacine: Is the additional bug you mentioned in #31 already fixed in the patch of #31 or does it still need to be fixed?

Status:Needs review» Needs work

Still needs to be fixed.

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

Issue tags:+CSS, +overlay

tagging

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

making a d8 issue.

Status:Needs work» Needs review
StatusFileSize
new7.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch contextual_1.patch. See the log in the details link for more information.
[ View ]

Some code to work from.

Status:Needs review» Needs work

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

Status:Needs work» Active

Let's just say this module has changed a whole lot since September.

Probably needs to be completely reviewed.

Priority:Normal» Major

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

Issue tags:+a11ySprint

Tagging. Hopefully we can get someone to re-roll this weekend.

Making some progress re-rolling a patch at the Montreal sprint. hope to have something to post shortly.

Status:Active» Needs review
StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 37,004 pass(es).
[ View ]

iwayMan and I have re-rolled a patch using casey's #39 code as a starting point.

StatusFileSize
new8.15 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch contextual-contextual_links_keyboard_accessibility-849926-46.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work
Issue tags:+CSS, +overlay, +accessibility, +needs JavaScript review, +needs backport to D7, +a11ySprint

The last submitted patch, contextual-contextual_links_keyboard_accessibility-849926-46.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.42 KB
PASSED: [[SimpleTest]]: [MySQL] 48,120 pass(es).
[ View ]

Here's a re-roll of the patch above.

Issue tags:+2012mtlsprint

Tagging for DrupalCamp Montreal 2012 Sprint

I've tested the keyboard functionality and verified that it's working as expected.

Status:Needs review» Reviewed & tested by the community

Tested 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

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

Status:Reviewed & tested by the community» Needs review

Tested with the voiceover utility in mac osx
* When tabbed to the gear - audio announces you are on a closed menu list

Is there context for exactly what this means? What happens when this is activated?

Is there context for exactly what this means? What happens when this is activated?

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

StatusFileSize
new13.4 KB
PASSED: [[SimpleTest]]: [MySQL] 50,689 pass(es).
[ View ]

This 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 and element-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:

// Bind behaviors through delegation.
var highlightRegion = $.proxy(this.highlightRegion, this);
this.$region
  .on('click.contextual', '.trigger', $.proxy(this.triggerClickHandler, this))
  .on('mouseenter.contextual', {highlight: true}, highlightRegion)
  .on('mouseleave.contextual', {highlight: false}, highlightRegion)
  .on('mouseleave.contextual', '.contextual', {show: false}, $.proxy(this.triggerLeaveHandler, this))
  .on('focusin.contextual', '.contextual a, .trigger', {highlight: true}, highlightRegion)
  .on('focusout.contextual', '.contextual a, .trigger', {highlight: false}, highlightRegion);

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.

Issue tags:+keyboard focus
StatusFileSize
new63.57 KB

I'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.

I'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.

I only had a quick look

+++ b/core/modules/contextual/contextual.jsundefined
@@ -3,55 +3,141 @@
+ * If the state of a contextual region is toggled to inactive, the links

looks like some of the comment is missing

StatusFileSize
new13.44 KB
PASSED: [[SimpleTest]]: [MySQL] 49,282 pass(es).
[ View ]

Rerolled on latest HEAD (bb91b33df84609af587a7673a9a5a8fe0b0e6cbe).

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

I'm working on this this morning.

Assigned:Unassigned» jessebeach

StatusFileSize
new5.03 KB
new15.29 KB
PASSED: [[SimpleTest]]: [MySQL] 49,340 pass(es).
[ View ]

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

Issue tags:+JavaScript

Tagging w/ JavaScript for that.

StatusFileSize
new1.13 KB
new14.46 KB
PASSED: [[SimpleTest]]: [MySQL] 49,448 pass(es).
[ View ]

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

re: #68: The details you just provided in the comment actually belong into the code as an inline comment. ;)

StatusFileSize
new14.47 KB
PASSED: [[SimpleTest]]: [MySQL] 49,317 pass(es).
[ View ]

Good point. Added the comment.

StatusFileSize
new102.9 KB

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

Status:Needs review» Reviewed & tested by the community

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

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

I'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!

Status:Reviewed & tested by the community» Fixed
Issue tags:+Spark

Yeah, 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!

Status:Fixed» Closed (fixed)

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

Version:8.x-dev» 7.x-dev
Status:Closed (fixed)» Patch (to be ported)

This doesn't look like it ever made it into Drupal 7. Hopefully there's a good way to backport it.

http://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.

Assigned:jessebeach» Unassigned

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

Issue summary:View changes

Initial migration to new Issue Summary template.