Originally discovered in #2528924: Spotlight accessibility regressions

When a spotlight item's image or link has tab focus, and then the spotlight moves to the next slide, focus goes immediately back to the top of the page.

CommentFileSizeAuthor
#5 2532886-5-spotlight-focus.patch1.21 KBultimike
#1 2532886-spotlight-focus.patch1.23 KBultimike
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Active » Needs review
FileSize
2532886-spotlight-focus.patch1.23 KB

Here's a patch to take care of this focus issue.

thanks,
-mike

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Needs work

Thanks for the patch!

Some quick review:

  1. +++ b/profiles/panopoly/modules/panopoly/panopoly_widgets/panopoly-widgets-spotlight.js
@@ -58,10 +58,21 @@ Drupal.settings.spotlight_settings = Drupal.settings.spotlight_settings || {};
+              var $keepFocus = false;

    Naming variables with a dollar sign is a coding convention for variables that are wrapped in jQuery. Whereas this variable is just a straight true/false value, so it shouldn't have a dollar sign.

    (Personally, I'm not crazy about this particular convention, but the code here was already using it so I didn't change it when rewriting this.) 

  2. +++ b/profiles/panopoly/modules/panopoly/panopoly_widgets/panopoly-widgets-spotlight.js
@@ -58,10 +58,21 @@ Drupal.settings.spotlight_settings = Drupal.settings.spotlight_settings || {};
+              if ($keepFocus) {
+                $('a', $slides.filter(selector)).focus();
+              }

    What if the next slide doesn't have a link? It seems extreme to completely lose focus in that case - is there somewhere else it can go?

Also, earlier in the week I talked about this issue with @cboyden on IRC, who discussed it with a colleague on the UC Berkeley a11y team (who personally uses a screenreader), and it was their opinion that the best thing to do would be to stop the spotlight altogether when a slide (and maybe the pager too) has focus.

I'll ping @cboyden to chime in here. As the non-accessibility expert in the room, I can really only review the code. :-)

cboyden’s picture

cboyden CreditAttribution: cboyden commented

In case the next slide doesn't have a link, you could add a tabindex of -1 to the image (the only thing that's actually required in a spotlight item). This makes it programmatically focusable but does not put it in the tab order. Then when the slide shifts away from a focused slide you check for a link on the next one and focus there, or fall back to focusing on the image if there's no link.

cboyden’s picture

cboyden CreditAttribution: cboyden commented

The question of whether to keep rotating when a slide has focus is a bigger one. It will affect the UI for everyone. I'm going to think out loud about how it might work...

Do we want it to pause on mouseover? I don't think so, I might have just clicked on a link and my mouse is positioned over the slideshow when the page loads. Then I might click the Play button, but if we don't do something fancy with the pager controls, I'm still moused over the slideshow.

Do we want it to pause when a slide has keyboard focus? My colleague thought that would be a good idea. If so, the Play/Pause button should be put in the Play state when you tab to a slide. What happens if a slide does not have a link? Even if we add tabindex=-1 as recommended above, the slide won't be in focus if you tab to it while it's rotating.

Do we want it to pause when someone selects a specific slide? I think yes. If you select a slide using the pager controls, the rotation should stop and focus should be set to the link or fall back to the image as above.

Do we want it to pause when the pager controls are in focus? Maybe not - if you click the Play button, you're focused on the Pause button, which is in the pager controls. It doesn't make sense to pause it immediately.

Finally, it might be useful to add a Spotlight option to start a slideshow paused. So when the page loads, you see the first slide, there's no rotation, but the Play button is visible.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
2532886-5-spotlight-focus.patch1.21 KB

Ignoring the issue of pausing on focus for now, attached is an updated patch that addresses @dsnopek's comments as well as setting the img tabindex to -1 when a link doesn't exist.

-mike

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Sweet, thanks!

+++ b/panopoly-widgets-spotlight.js
@@ -58,10 +58,26 @@ Drupal.settings.spotlight_settings = Drupal.settings.spotlight_settings || {};
+                  $('img', $slides.filter(selector)).attr('tabindex', -1);

Is this missing a .focus() at the end?

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs work » Needs review
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Needs work

Hm. This isn't working for me under Chrome 43.0.2357.125. Here's what I tried:

  1. Create a Spotlight with two slides, one that has a link and one without
  2. Press TAB until the slide has focus
  3. Wait until the slide changes
  4. Press TAB, and see that focus has gone back to the top of the page

I tried adding .focus() to the end of the line I mentioned in #6, and that didn't help. :-/

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

So, I retested this, exactly the same as I did in #8 (almost 2 years ago!), and this time it worked. However, I don't really understand how? :-) My comment up at #6 still stands: shouldn't we have to actually focus on the image on the next slide? I don't really understand how making it focusable really solves this problem, but it does seem to, at least under Chrome 58

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Works with Firefox too! Hm, it must be me that's missing something...

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Heh, ok, tried without the patch and now I can't reproduce the problem under either Chrome or Firefox. :-) So, maybe it was just fixed in Chrome in a version between 43 and 58?