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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

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

casey’s picture

Status: Active » Needs review
FileSize
4.33 KB
casey’s picture

FileSize
4.56 KB

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

mgifford’s picture

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?

casey’s picture

Everett Zufelt’s picture

Priority: Normal » Critical

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

mgifford’s picture

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

Jeff Burnz’s picture

Priority: Critical » Normal

This is not critical.

cwgordon7’s picture

Status: Needs review » Reviewed & tested by the community

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

casey’s picture

Status: Reviewed & tested by the community » Needs work

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

Everett Zufelt’s picture

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.

sun’s picture

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

mgifford’s picture

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

sun’s picture

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.

mgifford’s picture

bumping this issue by adding a tag.

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

mgifford’s picture

Status: Needs work » Needs review

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

mgifford’s picture

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.

sun.core’s picture

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

Comment in #12 still stands.

Everett Zufelt’s picture

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.

mgifford’s picture

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

mgifford’s picture

Status: Needs work » Needs review

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

Everett Zufelt’s picture

@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');
+          },
Everett Zufelt’s picture

Priority: Major » Critical

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

EvanDonovan’s picture

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.

Jeff Burnz’s picture

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

webchick’s picture

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.

mgifford’s picture

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

Jacine’s picture

The patch no longer applies.

Everett Zufelt’s picture

Status: Needs work » Needs review

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

Jacine’s picture

FileSize
58.51 KB
4.93 KB

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.

mgifford’s picture

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.

Jacine’s picture

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.

sun’s picture

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?

Jacine’s picture

Status: Needs review » Needs work

Still needs to be fixed.

mgifford’s picture

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.

mgifford’s picture

Issue tags: +CSS, +overlay

tagging

mgifford’s picture

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

making a d8 issue.

casey’s picture

Status: Needs work » Needs review
FileSize
7.77 KB

Some code to work from.

Status: Needs review » Needs work

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

mgifford’s picture

Status: Needs work » Active

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

Probably needs to be completely reviewed.

Bojhan’s picture

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.

mgifford’s picture

Issue tags: +a11ySprint

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

Mirabuck’s picture

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

Mirabuck’s picture

Status: Active » Needs review
FileSize
7.86 KB

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

Mirabuck’s picture

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.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +CSS, +overlay, +Accessibility, +Needs JavaScript review, +Needs backport to D7, +a11ySprint
mgifford’s picture

Status: Needs work » Needs review
FileSize
8.42 KB

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

Mirabuck’s picture

Issue tags: +2012mtlsprint

Tagging for DrupalCamp Montreal 2012 Sprint

Mirabuck’s picture

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

delmarr’s picture

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

ry5n’s picture

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

Everett Zufelt’s picture

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?

Mirabuck’s picture

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.

mgifford’s picture

jessebeach’s picture

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.

mgifford’s picture

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.

mgifford’s picture

mgifford’s picture

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.

attiks’s picture

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

jessebeach’s picture

Rerolled on latest HEAD (bb91b33df84609af587a7673a9a5a8fe0b0e6cbe).

mgifford’s picture

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?

jessebeach’s picture

I'm working on this this morning.

Wim Leers’s picture

Assigned: Unassigned » jessebeach
jessebeach’s picture

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.

webchick’s picture

Issue tags: +JavaScript

Tagging w/ JavaScript for that.

jessebeach’s picture

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.

sun’s picture

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

jessebeach’s picture

Good point. Added the comment.

mgifford’s picture

FileSize
102.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...

Gábor Hojtsy’s picture

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

mgifford’s picture

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.

jessebeach’s picture

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!

webchick’s picture

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!

echoz’s picture

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

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.

dcam’s picture

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.

star-szr’s picture

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.

ehansin’s picture

Issue summary: View changes

Initial migration to new Issue Summary template.

  • webchick committed 8eefa57 on 8.3.x
    Issue #849926 by jessebeach, Jacine, mgifford, casey, Mirabuck: Fixed...

  • webchick committed 8eefa57 on 8.3.x
    Issue #849926 by jessebeach, Jacine, mgifford, casey, Mirabuck: Fixed...

  • webchick committed 8eefa57 on 8.4.x
    Issue #849926 by jessebeach, Jacine, mgifford, casey, Mirabuck: Fixed...

  • webchick committed 8eefa57 on 8.4.x
    Issue #849926 by jessebeach, Jacine, mgifford, casey, Mirabuck: Fixed...
steinmb’s picture