Spotlight Accessibility Improvements

For 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!

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

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

1. +++ b/panopoly-widgets.js
   @@ -39,10 +39,28 @@ Drupal.settings.spotlight_settings = Drupal.settings.spotlight_settings || {};
         if ($('.field-name-field-basic-spotlight-items').length) {
   -     	var rotation_time = Drupal.settings.spotlight_settings.rotation_time;
   +     	 var rotation_time = Drupal.settings.spotlight_settings.rotation_time;
           $('.field-name-field-basic-spotlight-items').tabs().tabs("rotate", rotation_time, true);
   

   Extra space in front of 'var' pushing out indent too far.

2. +++ b/panopoly-widgets.js
   @@ -39,10 +39,28 @@ Drupal.settings.spotlight_settings = Drupal.settings.spotlight_settings || {};
   +           $(this).text('Pause');
   

   Should use Drupal.t('Pause') so that text can be translated.

3. +++ b/panopoly-widgets.js
   @@ -39,10 +39,28 @@ Drupal.settings.spotlight_settings = Drupal.settings.spotlight_settings || {};
   +           $(this).text('Play');
   

   And Drupal.t('Play') here too.

4. +++ b/panopoly_widgets.spotlight.inc
   @@ -157,12 +157,20 @@ function panopoly_widgets_field_formatter_view($entity_type, $entity, $field, $i
   +    $header .= '<a href="#" id="panopoly-spotlight-pause-play">' . t('Pause') . '</a>';
   

   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 a class="...". This will affect the Javascript too.

5. +++ b/panopoly_widgets.spotlight.inc
   @@ -187,6 +195,12 @@ function panopoly_widgets_field_widget_form(&$form, &$form_state, $field, $insta
   +    $element['alt'] = array(
   +      '#title' => t('Alt text'),
   +      '#type' => 'textfield',
   +      '#default_value' => isset($items[$delta]['alt']) ? $items[$delta]['alt'] : NULL,
   +    );
   +
   

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

6. +++ b/panopoly_widgets.module
   @@ -34,13 +34,20 @@ function panopoly_widgets_configure_form($form, &$form_state) {
   +  $form['panopoly_widgets_spotlight_pause_play_buttons'] = array(
   +    '#title' => t('Add pause/play button to all spotlight widgets.'),
   +    '#type' => 'checkbox',
   +    '#default_value' => variable_get('panopoly_widgets_spotlight_pause_play_buttons', 1),
   +    '#description' => t('The button will appear under to the slide numbers. Helps with accessibility.'),
   +  );
   

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

Awesome, thanks!

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.

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!