From 05539b1bbef92d73df2ab966f391dfd9ec678da2 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 13 Aug 2013 12:29:15 +0300 Subject: [PATCH 1/7] Removed unnecessary tabindex -1 from Video template. --- lms/templates/video.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lms/templates/video.html b/lms/templates/video.html index ab3fd08d0c..90b3e5b25f 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -45,15 +45,15 @@
-

${_('Speed')}

-

+

${_('Speed')}

+

-
    +
      -
      -
      +
      +
      ${_('Fill browser')} From 0a5d261fc422dc237f7e59f25aa742948eb960d5 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 13 Aug 2013 12:40:01 +0300 Subject: [PATCH 2/7] For YouTube videos tabbing from Speeds to Volume closes Speeds dialog. An old TODO item was done. It turns out a simple case of calling the method to bind handlers after the Spees dialog was re-rendered. --- .../xmodule/js/src/video/08_video_speed_control.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) 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 740a1aa63d..e8cb1e15ed 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 @@ -151,7 +151,7 @@ function () { $.each(this.videoSpeedControl.speeds, function(index, speed) { var link, listItem; - link = '' + speed + 'x'; + link = '' + speed + 'x'; listItem = $('
    1. ' + link + '
      2. '); @@ -162,11 +162,7 @@ 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. + _bindHandlers(this); } }); From da3e21ceee67bde92452c99515785ff85729ec47 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 13 Aug 2013 14:51:47 +0300 Subject: [PATCH 3/7] Fixed tabbing backwards. Now you can tab through all of the controls in Video forwards, and then tab backwards. --- .../js/src/video/07_video_volume_control.js | 15 ++++++++---- .../js/src/video/08_video_speed_control.js | 24 ++++++++++++++++++- 2 files changed, 33 insertions(+), 6 deletions(-) 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 6b12783e9e..cb05887144 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 @@ -77,20 +77,25 @@ function () { $(this).addClass('open'); }); - state.videoVolumeControl.buttonEl.on('focus', function() { - $(this).parent().addClass('open'); - }); - state.videoVolumeControl.el.on('mouseleave', function() { $(this).removeClass('open'); }); state.videoVolumeControl.buttonEl.on('blur', function() { - state.videoVolumeControl.volumeSliderEl.find('a').focus(); + if (state.volumeBlur !== true) { + state.videoVolumeControl.el.addClass('open'); + state.videoVolumeControl.volumeSliderEl.find('a').focus(); + } else { + state.volumeBlur = false; + } }); state.videoVolumeControl.volumeSliderEl.find('a').on('blur', function () { state.videoVolumeControl.el.removeClass('open'); + + state.videoVolumeControl.buttonEl.focus(); + + state.volumeBlur = true; }); } 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 e8cb1e15ed..29424ae4b2 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 @@ -58,6 +58,8 @@ function () { ); }); + state.videoSpeedControl.videoSpeedsEl.find('a:first').addClass('first_speed_el'); + state.videoSpeedControl.setSpeed(state.speed); } @@ -89,7 +91,13 @@ function () { state.videoSpeedControl.el.children('a') .on('focus', function () { - $(this).parent().addClass('open'); + if (state.firstSpeedBlur === true) { + $(this).parent().removeClass('open'); + + state.firstSpeedBlur = false; + } else { + $(this).parent().addClass('open'); + } }) .on('blur', function () { state.videoSpeedControl.videoSpeedsEl @@ -101,6 +109,16 @@ function () { .on('blur', function () { state.videoSpeedControl.el.removeClass('open'); }); + + state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:first') + .on('blur', function () { + state.firstSpeedBlur = true; + }); + + state.videoSpeedControl.videoSpeedsEl.find('a.speed_link') + .on('focus', function () { + state.firstSpeedBlur = false; + }); } } @@ -162,6 +180,10 @@ function () { _this.videoSpeedControl.videoSpeedsEl.prepend(listItem); }); + this.videoSpeedControl.videoSpeedsEl + .find('a:first') + .addClass('first_speed_el'); + _bindHandlers(this); } From efac70e6b392812bf2c61b4389909202faad21d5 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 13 Aug 2013 16:06:05 +0300 Subject: [PATCH 4/7] Adding documentation to the event handlers for volume and speed control. --- .../js/src/video/07_video_volume_control.js | 60 ++++++++++--- .../js/src/video/08_video_speed_control.js | 86 +++++++++++++++---- 2 files changed, 113 insertions(+), 33 deletions(-) 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 cb05887144..a21a19f23c 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 @@ -67,36 +67,68 @@ function () { 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.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 (state.volumeBlur !== true) { + // 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(); - } else { - state.volumeBlur = false; } }); - state.videoVolumeControl.volumeSliderEl.find('a').on('blur', function () { - state.videoVolumeControl.el.removeClass('open'); + // 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 + // contrinue 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 sisible element that + // we can tab to after the volume button. + state.videoVolumeControl.el.removeClass('open'); - state.videoVolumeControl.buttonEl.focus(); + // Set focus to the volume button. + state.videoVolumeControl.buttonEl.focus(); - state.volumeBlur = true; - }); + // We store the fact that previous element that lost focus was + // the volume clontrol. + state.volumeBlur = true; + }); } // *************************************************************** 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 29424ae4b2..988ac6b2f0 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 @@ -58,65 +58,115 @@ function () { ); }); - state.videoSpeedControl.videoSpeedsEl.find('a:first').addClass('first_speed_el'); - 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) { 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 () { + // 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) { - $(this).parent().removeClass('open'); + state.videoSpeedControl.el.removeClass('open'); state.firstSpeedBlur = false; - } else { - $(this).parent().addClass('open'); + } + + // 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(); }); + + // ****************************** + // Attach 'focus', and 'blur' events to elements which represent + // individual speed entries. state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:last') .on('blur', function () { + // If we have reached the last speed enrty, and the focus + // changes to the next element, we need to hide the speeds + // control drop-down. state.videoSpeedControl.el.removeClass('open'); }); - state.videoSpeedControl.videoSpeedsEl.find('a.speed_link: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; }); - state.videoSpeedControl.videoSpeedsEl.find('a.speed_link') .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; }); } @@ -180,10 +230,8 @@ function () { _this.videoSpeedControl.videoSpeedsEl.prepend(listItem); }); - this.videoSpeedControl.videoSpeedsEl - .find('a:first') - .addClass('first_speed_el'); - + // Re-attach all events with their appropriate callbacks to the + // newly generated elements. _bindHandlers(this); } From 3b326c307204226be35c552e315273476c53b3e0 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 14 Aug 2013 11:48:59 +0300 Subject: [PATCH 5/7] Fixing spelling typos in comments. --- .../xmodule/xmodule/js/src/video/07_video_volume_control.js | 4 ++-- .../xmodule/xmodule/js/src/video/08_video_speed_control.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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 a21a19f23c..785401aa65 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 @@ -116,9 +116,9 @@ function () { state.videoVolumeControl.volumeSliderEl.find('a') .on('blur', function () { // Hide the volume slider. This is done so that we can - // contrinue to the next (or previous) element by tabbing. + // 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 sisible element that + // slider because it is the next element visible element that // we can tab to after the volume button. state.videoVolumeControl.el.removeClass('open'); 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 988ac6b2f0..87d0b31b10 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 @@ -148,7 +148,7 @@ function () { // individual speed entries. state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:last') .on('blur', function () { - // If we have reached the last speed enrty, and the focus + // 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'); From 99ae63777e317183fa7fb5ac9fa0f35603dab337 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 14 Aug 2013 11:51:15 +0300 Subject: [PATCH 6/7] Removed unnecessary tabindex = 0 from - they get it by default. --- .../xmodule/js/src/video/07_video_volume_control.js | 3 --- lms/templates/video.html | 10 +++++----- 2 files changed, 5 insertions(+), 8 deletions(-) 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 785401aa65..90154d2079 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 @@ -61,9 +61,6 @@ 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); } diff --git a/lms/templates/video.html b/lms/templates/video.html index 90b3e5b25f..43f36915a0 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -39,25 +39,25 @@
        -
        • +
      • 0:00 / 0:00
      - +

      ${_('Speed')}
        - +
        - ${_('Fill browser')} - ${_('HD')} + ${_('Fill browser')} + ${_('HD')} ${_('Captions')}
        From b583a4e7932d19f6a39f34bc3b2295abbd69ae8f Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 14 Aug 2013 11:56:13 +0300 Subject: [PATCH 7/7] Optimizing code. Caching an element selector. --- .../js/src/video/08_video_speed_control.js | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) 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 87d0b31b10..27e4888d0d 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 @@ -77,6 +77,8 @@ function () { * @returns {undefined} */ function _bindHandlers(state) { + var speedLinks; + state.videoSpeedControl.videoSpeedsEl.find('a') .on('click', state.videoSpeedControl.changeVideoSpeed); @@ -146,29 +148,29 @@ function () { // ****************************** // Attach 'focus', and 'blur' events to elements which represent // individual speed entries. - state.videoSpeedControl.videoSpeedsEl.find('a.speed_link: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'); - }); - state.videoSpeedControl.videoSpeedsEl.find('a.speed_link: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; - }); - state.videoSpeedControl.videoSpeedsEl.find('a.speed_link') - .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; - }); + 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; + }); } }