Merge pull request #659 from edx/valera/bugfix_tabbing_video_controls

Valera/bugfix tabbing video controls

common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js

@@ -61,36 +61,70 @@ function () {
             slide: state.videoVolumeControl.onChange
         });
 
-        // Make sure that we can focus the actual volume slider while Tabing.
-        state.videoVolumeControl.volumeSliderEl.find('a').attr('tabindex', '0');
-
         state.videoVolumeControl.el.toggleClass('muted', state.videoVolumeControl.currentVolume === 0);
     }
 
-    // function _bindHandlers(state)
-    //
-    //     Bind any necessary function callbacks to DOM events (click, mousemove, etc.).
+    /**
+     * @desc Bind any necessary function callbacks to DOM events (click,
+     *     mousemove, etc.).
+     *
+     * @type {function}
+     * @access private
+     *
+     * @param {object} state The object containg the state of the video player.
+     *     All other modules, their parameters, public variables, etc. are
+     *     available via this object.
+     *
+     * @this {object} The global window object.
+     *
+     * @returns {undefined}
+     */
     function _bindHandlers(state) {
-        state.videoVolumeControl.buttonEl.on('click', state.videoVolumeControl.toggleMute);
+        state.videoVolumeControl.buttonEl
+            .on('click', state.videoVolumeControl.toggleMute);
 
         state.videoVolumeControl.el.on('mouseenter', function() {
-            $(this).addClass('open');
-        });
-
-        state.videoVolumeControl.buttonEl.on('focus', function() {
-            $(this).parent().addClass('open');
+            state.videoVolumeControl.el.addClass('open');
         });
 
         state.videoVolumeControl.el.on('mouseleave', function() {
-            $(this).removeClass('open');
+            state.videoVolumeControl.el.removeClass('open');
         });
 
+        // Attach a focus event to the volume button.
         state.videoVolumeControl.buttonEl.on('blur', function() {
+            // If the focus is being trasnfered from the volume slider, then we
+            // don't do anything except for unsetting the special flag.
+            if (state.volumeBlur === true) {
+                state.volumeBlur = false;
+            }
+
+            //If the focus is comming from elsewhere, then we must show the
+            // volume slider and set focus to it.
+            else {
+                state.videoVolumeControl.el.addClass('open');
                 state.videoVolumeControl.volumeSliderEl.find('a').focus();
+            }
         });
 
-        state.videoVolumeControl.volumeSliderEl.find('a').on('blur', function () {
+        // Attach a blur event handler (loss of focus) to the volume slider
+        // element. More specifically, we are attaching to the handle on
+        // the slider with which you can change the volume.
+        state.videoVolumeControl.volumeSliderEl.find('a')
+            .on('blur', function () {
+                // Hide the volume slider. This is done so that we can
+                // continue to the next (or previous) element by tabbing.
+                // Otherwise, after next tab we would come back to the volume
+                // slider because it is the next element visible element that
+                // we can tab to after the volume button.
                 state.videoVolumeControl.el.removeClass('open');
+
+                // Set focus to the volume button.
+                state.videoVolumeControl.buttonEl.focus();
+
+                // We store the fact that previous element that lost focus was
+                // the volume clontrol.
+                state.volumeBlur = true;
             });
     }
 

common/lib/xmodule/xmodule/js/src/video/08_video_speed_control.js

@@ -61,46 +61,116 @@ function () {
         state.videoSpeedControl.setSpeed(state.speed);
     }
 
-    // function _bindHandlers(state)
-    //
-    //     Bind any necessary function callbacks to DOM events (click,
-    //     mousemove, etc.).
+    /**
+     * @desc Bind any necessary function callbacks to DOM events (click,
+     *     mousemove, etc.).
+     *
+     * @type {function}
+     * @access private
+     *
+     * @param {object} state The object containg the state of the video player.
+     *     All other modules, their parameters, public variables, etc. are
+     *     available via this object.
+     *
+     * @this {object} The global window object.
+     *
+     * @returns {undefined}
+     */
     function _bindHandlers(state) {
+        var speedLinks;
+
         state.videoSpeedControl.videoSpeedsEl.find('a')
             .on('click', state.videoSpeedControl.changeVideoSpeed);
 
         if (onTouchBasedDevice()) {
             state.videoSpeedControl.el.on('click', function(event) {
+                // So that you can't highlight this control via a drag
+                // operation, we disable the default browser actions on a
+                // click event.
                 event.preventDefault();
-                $(this).toggleClass('open');
+
+                state.videoSpeedControl.el.toggleClass('open');
             });
         } else {
             state.videoSpeedControl.el
                 .on('mouseenter', function () {
-                    $(this).addClass('open');
+                    state.videoSpeedControl.el.addClass('open');
                 })
                 .on('mouseleave', function () {
-                    $(this).removeClass('open');
+                    state.videoSpeedControl.el.removeClass('open');
                 })
                 .on('click', function (event) {
+                    // So that you can't highlight this control via a drag
+                    // operation, we disable the default browser actions on a
+                    // click event.
                     event.preventDefault();
-                    $(this).removeClass('open');
+
+                    state.videoSpeedControl.el.removeClass('open');
                 });
 
+            // ******************************
+            // Attach 'focus', and 'blur' events to the speed button which
+            // either brings up the speed dialog with individual speed entries,
+            // or closes it.
             state.videoSpeedControl.el.children('a')
                 .on('focus', function () {
-                    $(this).parent().addClass('open');
+                    // If the focus is comming from the first speed entry, this
+                    // means we are tabbing backwards. In this case we have to
+                    // hide the speed entries which will allow us to change the
+                    // focus further backwards.
+                    if (state.firstSpeedBlur === true) {
+                        state.videoSpeedControl.el.removeClass('open');
+
+                        state.firstSpeedBlur = false;
+                    }
+
+                    // If the focus is comming from some other element, show
+                    // the drop down with the speed entries.
+                    else {
+                        state.videoSpeedControl.el.addClass('open');
+                    }
                 })
                 .on('blur', function () {
+                    // When the focus leaves this element, if the speed entries
+                    // dialog is shown (tabbing forwards), then we will set
+                    // focus to the first speed entry.
+                    //
+                    // If the selector does not select anything, then this
+                    // means that the speed entries dialog is closed, and we
+                    // are tabbing backwads. The browser will select the
+                    // previous element to tab to by itself.
                     state.videoSpeedControl.videoSpeedsEl
                         .find('a.speed_link:first')
                         .focus();
                 });
 
-            state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:last')
-                .on('blur', function () {
+
+            // ******************************
+            // Attach 'focus', and 'blur' events to elements which represent
+            // individual speed entries.
+            speedLinks = state.videoSpeedControl.videoSpeedsEl
+                .find('a.speed_link');
+
+            speedLinks.last().on('blur', function () {
+                // If we have reached the last speed entry, and the focus
+                // changes to the next element, we need to hide the speeds
+                // control drop-down.
                 state.videoSpeedControl.el.removeClass('open');
             });
+            speedLinks.first().on('blur', function () {
+                // This flag will indicate that the focus to the next
+                // element that will receive it is comming from the first
+                // speed entry.
+                //
+                // This flag will be used to correctly handle scenario of
+                // tabbing backwards.
+                state.firstSpeedBlur = true;
+            });
+            speedLinks.on('focus', function () {
+                // Clear the flag which is only set when we are un-focusing
+                // (the blur event) from the first speed entry.
+                state.firstSpeedBlur = false;
+            });
         }
     }
 
@@ -151,7 +221,7 @@ function () {
         $.each(this.videoSpeedControl.speeds, function(index, speed) {
             var link, listItem;
 
-            link = '<a href="#">' + speed + 'x</a>';
+            link = '<a class="speed_link" href="#">' + speed + 'x</a>';
 
             listItem = $('<li data-speed="' + speed + '">' + link + '</li>');
 
@@ -162,11 +232,9 @@ function () {
             _this.videoSpeedControl.videoSpeedsEl.prepend(listItem);
         });
 
-        this.videoSpeedControl.videoSpeedsEl.find('a')
-            .on('click', this.videoSpeedControl.changeVideoSpeed);
-
-        // TODO: After the control was re-rendered, we should attach 'focus'
-        // and 'blur' events once more.
+        // Re-attach all events with their appropriate callbacks to the
+        // newly generated elements.
+        _bindHandlers(this);
     }
 
 });

lms/templates/video.html

@@ -39,25 +39,25 @@
 
                 <div>
                     <ul class="vcr">
-                        <li><a class="video_control" href="#" tabindex="0" title="${_('Play')}"></a></li>
+                        <li><a class="video_control" href="#" title="${_('Play')}"></a></li>
                         <li><div class="vidtime">0:00 / 0:00</div></li>
                     </ul>
                     <div class="secondary-controls">
                         <div class="speeds">
-                            <a href="#" tabindex="0" title="Speeds">
-                                <h3 tabindex="-1">${_('Speed')}</h3>
-                                <p tabindex="-1" class="active"></p>
+                            <a href="#" title="Speeds">
+                                <h3>${_('Speed')}</h3>
+                                <p class="active"></p>
                             </a>
-                            <ol tabindex="-1" class="video_speeds"></ol>
+                            <ol class="video_speeds"></ol>
                         </div>
                         <div class="volume">
-                            <a href="#" tabindex="0" title="Volume"></a>
-                            <div tabindex="-1" class="volume-slider-container">
-                                <div tabindex="-1" class="volume-slider"></div>
+                            <a href="#" title="Volume"></a>
+                            <div class="volume-slider-container">
+                                <div class="volume-slider"></div>
                             </div>
                         </div>
-                        <a href="#" class="add-fullscreen" tabindex="0" title="${_('Fill browser')}">${_('Fill browser')}</a>
-                        <a href="#" class="quality_control" tabindex="0" title="${_('HD')}">${_('HD')}</a>
+                        <a href="#" class="add-fullscreen" title="${_('Fill browser')}">${_('Fill browser')}</a>
+                        <a href="#" class="quality_control" title="${_('HD')}">${_('HD')}</a>
 
                         <a href="#" class="hide-subtitles" title="${_('Turn off captions')}">${_('Captions')}</a>
                     </div>