Spotlight accessibility regressions

Thanks for reporting this! Marking as Critical as this represents a regression.

Looking at the diff from when #2229919: Spotlight Accessibility Improvements was committed to now, there's so many changes to the CSS and Javascript, it's hard to see a smoking gun just from the diff. :-/

Something I just wanted to throw out there quickly: If we could somehow create Behat tests for this functionality, we'd never regress this again! However, testing the spotlight widget via Behat at all is hard, doing it entirely from the keyboard would be even harder. But if we could do it, it'd be brilliant!

I did some testing on older versions of Panopoly: I can't reproduce the regression in Panopoly 1.22, but I can in 1.23. I suspect it was the update of jquery_update that broke it, since that updated jQuery UI from 1.8 to 1.10. :-/

Referencing the issue: #2235081: Update jquery_update to version 2.5

If the slide number buttons are not programmatically focusable while the slideshow is playing, that should be testable. I don't think Selenium can execute a click on something that's not programmatically focusable. If they are focusable programmatically but not manually then that's the problem, but it'll be harder to test.

@cboyden: Sure, we could try that. However, I'd prefer that the tests not be so specifically targeted at this regression, because the next regression will probably be different. It'd be best if test could walk through basically using the spotlight via the keyboard, such that anything that broke the normal flow would be caught!

So... The Spotlight is implemented using jQuery UI tabs: the slide numbers are the tabs themselves, and then the image is the content of the tab. #1 from the issue summary appears to be by design:

1. When a spotlight is playing (not paused), it is no longer possible to use the tab key to access each of the spotlight pager numbers. Only the active spotlight pager is accessible by tabbing.

jQuery UI wants you to use the right/left arrow keys to change which tab is selected. That kinda makes sense for normal tabs, but is totally a WTF for our spotlight.

You can try a demo of normal jQuery 1.10 tabs here:

http://api.jqueryui.com/1.10/tabs/#entry-examples

2. Only when the spotlight is paused are all the spotlight pager numbers accessible from by tabbing.

So, this appears to be a bug somewhere. I'm honestly not sure why it even works! I would have expected that #1 would still have blocked this, but the tabs (the <li>) can't be focused, the links inside of them can. So, when the spotlight is paused, you get two tab stops on the selected slide number, and only one on all the rest.

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

I haven't been able to pin this point down to anything in the jQuery UI code or reproduce on vanilla jQuery UI, but I suspect this is just an oversight. I don't think they expect the tab to change without someone clicking or keyboarding their way to the tab (which would correct the focus).

Anyway, I'm personally starting to feel like the root of the problem here is just that we're trying to abuse jQuery UI tabs to implement this! I really don't see how to correct jQuery UI's behavior to match our expectations. :-/ Maybe the solution here is switching to some other plugin to this, like cycle2? Or some pure custom code?

Some more researching / thinking / soul searching will be required to make a decision on that...

@dsnopek - For the eventual test that will verify it's working again, Behat provides a keypress step for form fields:

Given I press the :char key in the :field field

But not a generic keypress step. Probably the thing to do would be write three custom steps using Selenium functions. One step would programmatically focus an element by ID. The next would execute a keypress a specified number of times. And the third would check which element had focus. So the test could focus on a known element (to avoid tabbing through the whole page), then tab a specified number of times, then check that it was focused on the correct element. And then if there's an element of keyboard interactivity to check, the test could execute an Enter or Space keypress and verify the results.

Here's the first pass at a patch that doesn't use jQuery UI. It seems to work fine if there is only one spotlight on the page, but bad things happen if there two. I haven't figured that out yet and haven't done a review of the accessibility yet, but just wanted to post the in-progress patch.

Ok, here's a patch that's actually ready for review!

I fixed the issue with multiple Spotlight widgets on the same page, fixed a bug with the "Mini pager" style, and moved all the CSS for the spotlight into it's own CSS file (in a similar vein to other similar Spotlight refactors: #2526330: Split spotlight JS into it's own file). The patch was already changing every line of the Spotlight CSS, so I'm not really worried about scope creep. :-)

Also, I did some testing with the 3rd problem in the issue summary:

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

And this was a problem even on Panopoly 1.21, which is before the jQuery UI update. So, this part I don't think is a regression. We should still totally solve it! But, if a solution is going to take a while, in order to fix this critical quickly, we might have to spin that out into a follow-up issue.

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

And this was a problem even on Panopoly 1.21, which is before the jQuery UI update. So, this part I don't think is a regression. We should still totally solve it! But, if a solution is going to take a while, in order to fix this critical quickly, we might have to spin that out into a follow-up issue.

This is happening for me on Chrome, but not Firefox.

Thanks for the patch! I and a screenreader-using colleague tested it. As far as accessibility goes, it is equivalent to the version before the regressions introduced by the new jQuery UI version.

When I applied the patch I had to do a feature revert of panopoly_widgets.field_instance and a cache clear. Without them, the image did not display and there were a bunch of PHP notices. So this needs an update hook. Once that's in place, this should be good to go.

@cboyden: Thanks for testing!

The Feature being overridden turned out to have nothing to do with this issue - I fixed that on this issue #2532908: Panopoly Widgets overridden after upgrade to -dev

Here's a new patch (against latest -dev, which has changed per #2532908) that adds a hook_update_N() and fixes the issue where the pager would be positioned incorrectly under some themes.

I tested this by:

1. Installing a new Panopoly 1.25 site
2. Creating a Spotlight on the front page
3. Replacing panopoly_widgets with the latest -dev
4. Running only drush updb -y
5. Refreshing the page and seeing that it still worked, then inspecting the DOM to make sure it's not using jQuery UI anymore

It worked in my testing!

Oh, and I also created some follow-up issues so we can get this one committed as fast as possible:

#2532886: When slide in Spotlight has keyboard focus, all focus is lost when the slide changes (under Chrome)

#2532884: Audit and simplify the panopoly-widgets-spotlight.css

This all works as advertised. I may file some more accessibility enhancements later, but this patch restores the previous level of function.

Just to be safe, here's a Travis-CI build with this patch:

https://travis-ci.org/panopoly/panopoly/builds/70972670

I actually just realized that we do have some minimal Spotlight tests, so this will at least see if anything explodes. :-)

The tests have passed! It would have been great to get confirmation from @ultimike that this fixed it for him, but since this is Critical, I'm going to commit right away. Please feel free to test the latest -dev and report back here if there are problems!

@ultimike: Awesome! Thanks for testing. :-)