From 3cb0fd4156e7ef304871961811aabf6bcbbfbae2 Mon Sep 17 00:00:00 2001
From: Ari Rizzitano <arizzitano@edx.org>
Date: Tue, 21 Mar 2017 10:30:41 -0400
Subject: [PATCH] adjust focus order and fix duplicate ids in video player
 (TNL-6361, AC-587)

move focus to slider on caption select (TNL-6361)

toggle video playback on spacebar press

sr text for spacebar explanation

focus on slider container instead of handle

linter issues

cleanup

quality

more quality

ugh wrong var name

hack to get around linter

more quality

pls work this time

fix transcript height test

hopefully last one

fix some ie/ff issues

fix dupe ids in video sr instructions. [AC-587]

fix test
---
 .../js/spec/video/video_caption_spec.js       |  6 ++-
 .../spec/video/video_progress_slider_spec.js  |  2 +-
 .../xmodule/js/src/video/03_video_player.js   |  2 +
 .../js/src/video/06_video_progress_slider.js  | 48 +++++++++++++++----
 .../js/src/video/07_video_volume_control.js   | 11 +++--
 .../js/src/video/08_video_speed_control.js    |  9 +++-
 .../xmodule/js/src/video/09_video_caption.js  | 17 ++++---
 7 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js
index 7ddda1c092..0a0cdd3dbe 100644
--- a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js
+++ b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js
@@ -986,9 +986,11 @@
                     videoWrapperHeight = $('.video-wrapper').height();
                     progressSliderHeight = state.el.find('.slider').height();
                     controlHeight = state.el.find('.video-controls').height();
-                    shouldBeHeight = videoWrapperHeight -
+                    shouldBeHeight = parseInt((
+                        videoWrapperHeight -
                         0.5 * progressSliderHeight -
-                        controlHeight;
+                        controlHeight
+                    ), 10);
 
                     expect(realHeight).toBe(shouldBeHeight);
                 });
diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js
index 1f3d5f728d..f5149926f3 100644
--- a/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js
+++ b/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js
@@ -44,7 +44,7 @@
 
                     expect(timeControl).toHaveAttrs({
                         'role': 'slider',
-                        'aria-label': 'Video position',
+                        'aria-label': 'Video position. Press space to toggle playback',
                         'aria-disabled': 'false'
                     });
 
diff --git a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js
index 6eecd3df86..52b2633099 100644
--- a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js
+++ b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js
@@ -405,6 +405,8 @@ function(HTML5Video, Resizer) {
         this.videoPlayer.goToStartTime = false;
 
         this.videoPlayer.seekTo(time);
+        this.trigger('videoProgressSlider.focusSlider');
+
         this.el.trigger('seek', [time, oldTime, type]);
     }
 
diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js
index b79a17fefe..8c87b6b387 100644
--- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js
+++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js
@@ -12,7 +12,9 @@ mind, or whether to act, and in acting, to live."
 [],
 function() {
     var template = [
-        '<div class="slider" title="', gettext('Video position'), '"></div>'
+        '<div class="slider" role="application" title="',
+        gettext('Video position. Press space to toggle playback'),
+        '"></div>'
     ].join('');
 
     // VideoProgressSlider() function - what this module "exports".
@@ -35,6 +37,8 @@ function() {
     //
     //     Functions which will be accessible via 'state' object. When called,
     //     these functions will get the 'state' object as a context.
+
+    /* eslint-disable no-use-before-define */
     function _makeFunctionsPublic(state) {
         var methodsDict = {
             destroy: destroy,
@@ -45,7 +49,8 @@ function() {
             updatePlayTime: updatePlayTime,
             updateStartEndTimeRegion: updateStartEndTimeRegion,
             notifyThroughHandleEnd: notifyThroughHandleEnd,
-            getTimeDescription: getTimeDescription
+            getTimeDescription: getTimeDescription,
+            focusSlider: focusSlider
         };
 
         state.bindTo(methodsDict, state.videoProgressSlider, state);
@@ -57,6 +62,12 @@ function() {
         delete this.videoProgressSlider;
     }
 
+    function bindHandlers(state) {
+        state.videoProgressSlider.el.on('keypress', sliderToggle.bind(state));
+        state.el.on('destroy', state.videoProgressSlider.destroy);
+    }
+    /* eslint-enable no-use-before-define */
+
     // function _renderElements(state)
     //
     //     Create any necessary DOM elements, attach them, and set their
@@ -69,6 +80,7 @@ function() {
         state.el.find('.video-controls').prepend(state.videoProgressSlider.el);
         state.videoProgressSlider.buildSlider();
         _buildHandle(state);
+        bindHandlers(state);
     }
 
     function _buildHandle(state) {
@@ -77,7 +89,10 @@ function() {
 
         // ARIA
         // We just want the knob to be selectable with keyboard
-        state.videoProgressSlider.el.attr('tabindex', -1);
+        state.videoProgressSlider.el.attr({
+            tabindex: -1
+        });
+
         // Let screen readers know that this div, representing the slider
         // handle, behaves as a slider named 'video position'.
         state.videoProgressSlider.handle.attr({
@@ -89,10 +104,8 @@ function() {
             'aria-valuemin': '0',
             'aria-valuenow': state.videoPlayer.currentTime,
             'tabindex': '0',
-            'aria-label': gettext('Video position')
+            'aria-label': gettext('Video position. Press space to toggle playback')
         });
-
-        state.el.on('destroy', state.videoProgressSlider.destroy);
     }
 
     // ***************************************************************
@@ -103,8 +116,11 @@ function() {
     // ***************************************************************
 
     function buildSlider() {
-        this.videoProgressSlider.el
-            .append('<div class="ui-slider-handle progress-handle"></div>');
+        var sliderContents = edx.HtmlUtils.joinHtml(
+            edx.HtmlUtils.HTML('<div class="ui-slider-handle progress-handle"></div>')
+        );
+
+        this.videoProgressSlider.el.append(sliderContents.text);
 
         this.videoProgressSlider.slider = this.videoProgressSlider.el
             .slider({
@@ -328,5 +344,21 @@ function() {
 
         return i18n(seconds, 'second');
     }
+
+    // Shift focus to the progress slider container element.
+    function focusSlider() {
+        this.videoProgressSlider.handle.attr(
+            'aria-valuetext', getTimeDescription(this.videoPlayer.currentTime)
+        );
+        this.videoProgressSlider.el.trigger('focus');
+    }
+
+    // Toggle video playback when the spacebar is pushed.
+    function sliderToggle(e) {
+        if (e.which === 32) {
+            e.preventDefault();
+            this.videoCommands.execute('togglePlayback');
+        }
+    }
 });
 }(RequireJS.requirejs, RequireJS.require, RequireJS.define));
diff --git a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js
index 9ac4fa3d6b..63d7b7ee35 100644
--- a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js
+++ b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js
@@ -40,10 +40,10 @@ function(HtmlUtils) {
         videoVolumeControlHtml: HtmlUtils.interpolateHtml(
             HtmlUtils.HTML([
                 '<div class="volume" role="application">',
-                '<p class="sr instructions" id="volume-instructions">',
+                '<p class="sr instructions">',
                 '{volumeInstructions}',
                 '</p>',
-                '<button class="control" aria-disabled="false" aria-describedby="volume-instructions"',
+                '<button class="control" aria-disabled="false"',
                 '" aria-expanded="false" title="',
                 '{adjustVideoVolume}',
                 '">',
@@ -129,7 +129,8 @@ function(HtmlUtils) {
          * initial configuration.
          */
         render: function() {
-            var container = this.el.find('.volume-slider');
+            var container = this.el.find('.volume-slider'),
+                instructionsId = 'volume-instructions-' + this.state.id;
 
             HtmlUtils.append(container, HtmlUtils.HTML('<div class="ui-slider-handle volume-handle"></div>'));
 
@@ -146,6 +147,10 @@ function(HtmlUtils) {
             // order.
             container.find('.volume-handle').attr('tabindex', -1);
             this.state.el.find('.secondary-controls').append(this.el);
+
+            // set dynamic id for instruction element to avoid collisions
+            this.el.find('.instructions').attr('id', instructionsId);
+            this.button.attr('aria-describedby', instructionsId);
         },
 
         /** Bind any necessary function callbacks to DOM events. */
diff --git a/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js b/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js
index cfc328dea0..f953f1d772 100644
--- a/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js
+++ b/common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js
@@ -31,13 +31,13 @@
     SpeedControl.prototype = {
         template: [
             '<div class="speeds menu-container" role="application">',
-            '<p class="sr instructions" id="speed-instructions">',
+            '<p class="sr instructions">',
                     gettext('Press UP to enter the speed menu then use the UP and DOWN arrow keys to navigate the different speeds, then press ENTER to change to the selected speed.'),  // eslint-disable-line max-len, indent
             '</p>',
             '<button class="control speed-button" aria-disabled="false" aria-expanded="false"',
             'title="',
             gettext('Adjust video speed'),
-            '" aria-describedby="speed-instructions">',
+            '">',
             '<span>',
             '<span class="icon fa fa-caret-right" aria-hidden="true"></span>',
             '</span>',
@@ -98,6 +98,7 @@
         render: function(speeds, currentSpeed) {
             var speedsContainer = this.speedsContainer,
                 reversedSpeeds = speeds.concat().reverse(),
+                instructionsId = 'speed-instructions-' + this.state.id,
                 speedsList = $.map(reversedSpeeds, function(speed) {
                     return HtmlUtils.interpolateHtml(
                         HtmlUtils.HTML(
@@ -125,6 +126,10 @@
                 HtmlUtils.HTML(this.el)
             );
             this.setActiveSpeed(currentSpeed);
+
+            // set dynamic id for instruction element to avoid collisions
+            this.el.find('.instructions').attr('id', instructionsId);
+            this.speedButton.attr('aria-describedby', instructionsId);
         },
 
         /**
diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js
index a99cecbccd..2183ece093 100644
--- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js
+++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js
@@ -88,20 +88,19 @@
                         '<span class="icon fa fa-quote-left" aria-hidden="true"></span>',
                         '</button>',
                         '<div class="lang menu-container" role="application">',
-                        '<p class="sr instructions" id="lang-instructions"></p>',
+                        '<p class="sr instructions" id="lang-instructions-{courseId}"></p>',
                         '<button class="control language-menu" aria-disabled="false"',
-                        'aria-describedby="lang-instructions" ',
+                        'aria-describedby="lang-instructions-{courseId}" ',
                         'title="{langTitle}">',
                         '<span class="icon fa fa-caret-left" aria-hidden="true"></span>',
                         '</button>',
                         '</div>',
                         '</div>'
-                    ].join(''),
-                        {
-                            langTitle: gettext('Open language menu')
-                        }
-                    )
-
+                    ].join('')),
+                    {
+                        langTitle: gettext('Open language menu'),
+                        courseId: this.state.id
+                    }
                 );
 
                 var subtitlesHtml = HtmlUtils.interpolateHtml(
@@ -109,7 +108,7 @@
                     [
                         '<div class="subtitles" role="region" id="transcript-{courseId}">',
                         '<h3 id="transcript-label-{courseId}" class="transcript-title sr"></h3>',
-                        '<ol id="transcript-captions" class="subtitles-menu" lang="{courseLang}"></ol>',
+                        '<ol id="transcript-captions-{courseId}" class="subtitles-menu" lang="{courseLang}"></ol>',
                         '</div>'
                     ].join('')),
                     {