Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I have a client that is willing to pay for me to add some accessibility improvements to Panopoly Spotlight. Before I start, I'd like some feedback as to whether it is worth it to put time into Spotlight or if I should be looking at alternative slideshow plugins.
Here's what my client and I have discussed:
(non-accessibility) Make is so that spotlight image actually links to the content (not just the title).- (accessibility) Add a visible "pause" button.
- (accessibility) Add keyboard shortcuts to control the slideshow with (or ensure that controls are accessible via tabbing).
- (accessibility) Add "alt" tags to Spotlight images.
Ideally, to know if we've accomplished the accessibility goals, we should be able to:
- Turn off JavaScript on our browsers and still have access to the slideshow content.
- Control the slideshow content without the mouse.
- Control the pacing of the slideshow content with the mouse.
Thoughts?
Thanks,
-mike
Comment | File | Size | Author |
---|---|---|---|
#13 | spotlight_accessibility-2229919-13.patch | 10.96 KB | ultimike |
#12 | Selection_067.png | 17.08 KB | dsnopek |
Comments
Comment #1
dsnopekFor the most part, this all sounds good!
The one thing I'm not crazy about is the "visible pause" button. Would it be possible to allow pausing by either: (1) hovering over the spotlight with the mouse or (2) focussing on the spotlight (ie. by tabbing there or otherwise selecting it via a screenreader)?
Although, I'm not an accessibility expert by any means. :-) If that's not workable, that's fine.
If you have any examples of a spotlight/slider/courasel on a site somewhere that does accessibility really well, I'd love to see them! I have a screenreader I use (more rarely than I should) to test accessibility and it'd be really cool to try using a widget that gets it right.
Anyway, thanks for stepping up to tackle this!
Comment #2
ultimikeHere's the first crack at these improvements. This patch adds an "alt" field to the Spotlight items.
-mike
Comment #3
dsnopekI haven't tested the patch, but just reading through it, it's looking good so far. :-)
Are planing to do this in several small patches for the individual pieces or all at once? If you're going to do several small patches, then you'll also need to make seperate issues. It's up to you, but if you're going to be doing this all over a relatively short period of time, I'd prefer to just have one big issue/patch to review.
Comment #4
ultimikeComment #5
ultimikeComment #6
ultimikeI went ahead and removed the non-accessibility item from the original issue. I'll open a separate issue for that one.
@dsnopek - I'll keep all the accessibility updates in this one issue, so we have just one patch.
Thanks,
-mike
Comment #7
ultimikeHere's a more complete accessibility patch for the Spotlight widget. There are a number of things included:
Regarding the visible pause button - just about everything I've read indicates that having a visible pause button is the way to do it properly...
Overall, I think this patch is a good start at making the Spotlight widget accessible, as I think I've accomplished the goals outlined above.
Thoughts?
Thanks,
-mike
Comment #8
ultimikeFixed a z-index CSS issue.
-mike
Comment #9
ultimikeFixed a tabindex issue.
-mike
Comment #10
ultimikeMoved pause/play button to same line as slide numbers.
-mike
Comment #11
ultimikeComment #12
dsnopekI don't know if you were ready for review (because the status was "Active", not "Needs review") but I've gone ahead and done a quick review anyway. :-)
Functionally, it works great! I've done some reading on accessible image carousels/sliders and I've come to terms with the Play/Pause button. And where it's placed with the default styles looks quite nice! I've attached a screenshot for others who may be following along.
I've just got a few comments and requested changes to the code before I can commit:
Extra space in front of 'var' pushing out indent too far.
Should use
Drupal.t('Pause')
so that text can be translated.And
Drupal.t('Play')
here too.Even though it'd be crazy, multiple spotlights can appear on the same page. So, rather than
id="panopoly-spotlight-pause-play"
, this should be aclass="..."
. This will affect the Javascript too.I think the 'alt' field should appear below the 'fid' field, rather than below 'title'. It makes sense for the title/link to be close (because they affect the output of the same thing) and the fid/alt to be close (for the same reason).
This adds a new variable. This should be removed in hook_uninstall() in panopoly_widgets.install.
Anyway, thanks for working on this! Looking awesome so far. :-)
Comment #13
ultimikeThanks for the review - sorry I forgot to update the status earlier...
Anyway, thanks for the feedback, I went ahead and made the changes and have attached an updated patch.
One note - I didn't see a variable_del for "'panopoly_widgets_spotlight_rotation_time" in hook_uninstall(), so I went ahead and added that one as well.
thanks,
-mike
Comment #15
dsnopekAwesome, thanks!
Great catch! Honestly, the spotlight code could really use some overall clean-up and review. :-)
I've committed your changes:
http://drupalcode.org/project/panopoly_widgets.git/commit/c0e64a89908856...
Thanks again!
Comment #16
ultimikeSuperduper - thanks for reviewing and committing so quickly. I'll likely be pushing this out to a client's site in the next few days, so I'll be sure to report back (likely with a patch) if we find anything unexpected.
Thanks,
-mike