Merge pull request #911 from edx/valera/bugfix_access_to_hidden_controls

Valera/bugfix access to hidden controls

common/lib/xmodule/xmodule/css/video/display.scss

@@ -15,6 +15,13 @@ div.video {
     @include clearfix;
   }
 
+  div.focus_grabber {
+    position: relative;
+    display: inline;
+    width: 0px;
+    height: 0px;
+  }
+
   article.video-wrapper {
     float: left;
     margin-right: flex-gutter(9);

common/lib/xmodule/xmodule/js/fixtures/video.html

@@ -13,6 +13,8 @@
         data-yt-test-timeout="1500"
         data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
       >
+        <div class="focus_grabber first"></div>
+
         <div class="tc-wrapper">
           <article class="video-wrapper">
             <div class="video-player-pre"></div>
@@ -51,6 +53,8 @@
 
           <ol class="subtitles"><li></li></ol>
         </div>
+
+        <div class="focus_grabber last"></div>
       </div>
     </div>
   </div>

common/lib/xmodule/xmodule/js/fixtures/video_all.html

@@ -16,6 +16,8 @@
         data-yt-test-timeout="1500"
         data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
       >
+        <div class="focus_grabber first"></div>
+
         <div class="tc-wrapper">
           <article class="video-wrapper">
             <div class="video-player-pre"></div>
@@ -54,6 +56,8 @@
 
           <ol class="subtitles"><li></li></ol>
         </div>
+
+        <div class="focus_grabber last"></div>
       </div>
     </div>
   </div>

common/lib/xmodule/xmodule/js/fixtures/video_html5.html

@@ -16,6 +16,8 @@
         data-yt-test-timeout="1500"
         data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
       >
+        <div class="focus_grabber first"></div>
+
         <div class="tc-wrapper">
           <article class="video-wrapper">
             <section class="video-player">
@@ -26,6 +28,8 @@
 
           <ol class="subtitles"><li></li></ol>
         </div>
+
+        <div class="focus_grabber last"></div>
       </div>
     </div>
   </div>

common/lib/xmodule/xmodule/js/fixtures/video_no_captions.html

@@ -13,6 +13,8 @@
         data-yt-test-timeout="1500"
         data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
       >
+        <div class="focus_grabber first"></div>
+
         <div class="tc-wrapper">
           <article class="video-wrapper">
             <section class="video-player">
@@ -21,7 +23,9 @@
             <section class="video-controls"></section>
           </article>
         </div>
+
+        <div class="focus_grabber last"></div>
       </div>
     </div>
   </div>
-</div>
\ No newline at end of file
+</div>

common/lib/xmodule/xmodule/js/fixtures/video_yt_multiple.html

@@ -13,6 +13,8 @@
         data-yt-test-timeout="1500"
         data-yt-test-url="https://gdata.youtube.com/feeds/api/videos/"
       >
+        <div class="focus_grabber first"></div>
+
         <div class="tc-wrapper">
           <article class="video-wrapper">
             <div class="video-player-pre"></div>
@@ -51,6 +53,8 @@
 
           <ol class="subtitles"><li></li></ol>
         </div>
+
+        <div class="focus_grabber last"></div>
       </div>
     </div>
   </div>

common/lib/xmodule/xmodule/js/spec/combinedopenended/display_spec.coffee

@@ -48,17 +48,32 @@ describe 'CombinedOpenEnded', ->
       expect(@combined.task_count).toEqual 2
       expect(@combined.task_number).toEqual 1
 
-    it 'subelements are made collapsible', -> 
+    it 'subelements are made collapsible', ->
       expect(Collapsible.setCollapsibles).toHaveBeenCalled()
 
 
   describe 'poll', ->
+    # We will store default window.setTimeout() function here.
+    oldSetTimeout = null
+
     beforeEach =>
       # setup the spies
       @combined = new CombinedOpenEnded @element
       spyOn(@combined, 'reload').andCallFake -> return 0
+
+      # Store original window.setTimeout() function. If we do not do this, then
+      # all other tests that rely on code which uses window.setTimeout()
+      # function might (and probably will) fail.
+      oldSetTimeout = window.setTimeout
+      # Redefine window.setTimeout() function as a spy.
       window.setTimeout = jasmine.createSpy().andCallFake (callback, timeout) -> return 5
 
+    afterEach =>
+      # Reset the default window.setTimeout() function. If we do not do this,
+      # then all other tests that rely on code which uses window.setTimeout()
+      # function might (and probably will) fail.
+      window.setTimeout = oldSetTimeout
+
     it 'polls at the correct intervals', =>
       fakeResponseContinue = state: 'not done'
       spyOn($, 'postWithPrefix').andCallFake (url, callback) -> callback(fakeResponseContinue)
@@ -67,19 +82,34 @@ describe 'CombinedOpenEnded', ->
       expect(window.queuePollerID).toBe(5)
 
     it 'polling stops properly', =>
-      fakeResponseDone = state: "done" 
+      fakeResponseDone = state: "done"
       spyOn($, 'postWithPrefix').andCallFake (url, callback) -> callback(fakeResponseDone)
       @combined.poll()
       expect(window.queuePollerID).toBeUndefined()
       expect(window.setTimeout).not.toHaveBeenCalled()
 
   describe 'rebind', ->
+    # We will store default window.setTimeout() function here.
+    oldSetTimeout = null
+
     beforeEach ->
       @combined = new CombinedOpenEnded @element
       spyOn(@combined, 'queueing').andCallFake -> return 0
       spyOn(@combined, 'skip_post_assessment').andCallFake -> return 0
+
+      # Store original window.setTimeout() function. If we do not do this, then
+      # all other tests that rely on code which uses window.setTimeout()
+      # function might (and probably will) fail.
+      oldSetTimeout = window.setTimeout
+      # Redefine window.setTimeout() function as a spy.
       window.setTimeout = jasmine.createSpy().andCallFake (callback, timeout) -> return 5
 
+    afterEach =>
+      # Reset the default window.setTimeout() function. If we do not do this,
+      # then all other tests that rely on code which uses window.setTimeout()
+      # function might (and probably will) fail.
+      window.setTimeout = oldSetTimeout
+
     it 'when our child is in an assessing state', ->
       @combined.child_state = 'assessing'
       @combined.rebind()
@@ -87,19 +117,19 @@ describe 'CombinedOpenEnded', ->
       expect(@combined.submit_button.val()).toBe("Submit assessment")
       expect(@combined.queueing).toHaveBeenCalled()
 
-    it 'when our child state is initial', -> 
+    it 'when our child state is initial', ->
       @combined.child_state = 'initial'
       @combined.rebind()
       expect(@combined.answer_area.attr("disabled")).toBeUndefined()
       expect(@combined.submit_button.val()).toBe("Submit")
 
-    it 'when our child state is post_assessment', -> 
+    it 'when our child state is post_assessment', ->
       @combined.child_state = 'post_assessment'
       @combined.rebind()
       expect(@combined.answer_area.attr("disabled")).toBe("disabled")
       expect(@combined.submit_button.val()).toBe("Submit post-assessment")
 
-    it 'when our child state is done', -> 
+    it 'when our child state is done', ->
       spyOn(@combined, 'next_problem').andCallFake ->
       @combined.child_state = 'done'
       @combined.rebind()
@@ -112,7 +142,7 @@ describe 'CombinedOpenEnded', ->
       @combined.child_state = 'done'
 
     it 'handling a successful call', ->
-      fakeResponse = 
+      fakeResponse =
         success: true
         html: "dummy html"
         allow_reset: false

common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js

@@ -146,12 +146,27 @@
     });
 
     describe('mouse movement', function() {
+      // We will store default window.setTimeout() function here.
+      var oldSetTimeout = null;
+
       beforeEach(function() {
+        // Store original window.setTimeout() function. If we do not do this, then
+        // all other tests that rely on code which uses window.setTimeout()
+        // function might (and probably will) fail.
+        oldSetTimeout = window.setTimeout;
+        // Redefine window.setTimeout() function as a spy.
         window.setTimeout = jasmine.createSpy().andCallFake(function(callback, timeout) { return 5; })
         window.setTimeout.andReturn(100);
         spyOn(window, 'clearTimeout');
       });
 
+      afterEach(function () {
+        // Reset the default window.setTimeout() function. If we do not do this,
+        // then all other tests that rely on code which uses window.setTimeout()
+        // function might (and probably will) fail.
+        window.setTimeout = oldSetTimeout;
+      });
+
       describe('when cursor is outside of the caption box', function() {
         beforeEach(function() {
           $(window).trigger(jQuery.Event('mousemove'));

common/lib/xmodule/xmodule/js/spec/video/video_focus_grabber_spec.js

+(function () {
+    describe('Video FocusGrabber', function () {
+        var state;
+
+        beforeEach(function () {
+            // https://github.com/pivotal/jasmine/issues/184
+            //
+            // This is a known issue. jQuery animations depend on setTimeout
+            // and the jasmine mock clock stubs that function. You need to turn
+            // off jQuery animations ($.fx.off()) in a global beforeEach.
+            //
+            // I think this is a good pattern - you don't want animations
+            // messing with your tests. If you need to test with animations on
+            // I suggest you add incremental browser-based testing to your
+            // stack.
+            jQuery.fx.off = true;
+
+            loadFixtures('video_html5.html');
+            state = new Video('#example');
+
+            spyOnEvent(state.el, 'mousemove');
+            spyOn(state.focusGrabber, 'disableFocusGrabber').andCallThrough();
+            spyOn(state.focusGrabber, 'enableFocusGrabber').andCallThrough();
+        });
+
+        afterEach(function () {
+            // Turn jQuery animations back on.
+            jQuery.fx.off = true;
+        });
+
+        it(
+            'check existence of focus grabber elements and their position',
+            function () {
+
+            var firstFGEl = state.el.find('.focus_grabber.first'),
+                lastFGEl = state.el.find('.focus_grabber.last'),
+                tcWrapperEl = state.el.find('.tc-wrapper');
+
+            // Existence check.
+            expect(firstFGEl.length).toBe(1);
+            expect(lastFGEl.length).toBe(1);
+
+            // Position check.
+            expect(firstFGEl.index() + 1).toBe(tcWrapperEl.index());
+            expect(lastFGEl.index() - 1).toBe(tcWrapperEl.index());
+        });
+
+        it('from the start, focus grabbers are disabled', function () {
+            expect(state.focusGrabber.elFirst.attr('tabindex')).toBe(-1);
+            expect(state.focusGrabber.elLast.attr('tabindex')).toBe(-1);
+        });
+
+        it(
+            'when first focus grabber is focused "mousemove" event is ' +
+            'triggered, grabbers are disabled',
+            function () {
+
+            state.focusGrabber.elFirst.triggerHandler('focus');
+
+            expect('mousemove').toHaveBeenTriggeredOn(state.el);
+            expect(state.focusGrabber.disableFocusGrabber).toHaveBeenCalled();
+        });
+
+        it(
+            'when last focus grabber is focused "mousemove" event is ' +
+            'triggered, grabbers are disabled',
+            function () {
+
+            state.focusGrabber.elLast.triggerHandler('focus');
+
+            expect('mousemove').toHaveBeenTriggeredOn(state.el);
+            expect(state.focusGrabber.disableFocusGrabber).toHaveBeenCalled();
+        });
+
+        it('after controls hide focus grabbers are enabled', function () {
+            runs(function () {
+                // Captions should not be "sticky" for the autohide mechanism
+                // to work.
+                state.videoCaption.hideCaptions(true);
+
+                // Make sure that the controls are visible. After this event
+                // is triggered a count down is started to autohide captions.
+                state.el.triggerHandler('mousemove');
+            });
+
+            // Wait for the autohide to happen. We make it +100ms to make sure
+            // that there is clearly no race conditions for our expect below.
+            waits(state.videoControl.fadeOutTimeout + 100);
+
+            runs(function () {
+                expect(
+                    state.focusGrabber.enableFocusGrabber
+                ).toHaveBeenCalled();
+            });
+        });
+    });
+}).call(this);

common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js

@@ -145,7 +145,18 @@
     });
 
     describe('onStop', function() {
+      // We will store default window.setTimeout() function here.
+      var oldSetTimeout = null;
+
       beforeEach(function() {
+        // Store original window.setTimeout() function. If we do not do this, then
+        // all other tests that rely on code which uses window.setTimeout()
+        // function might (and probably will) fail.
+        oldSetTimeout = window.setTimeout;
+        // Redefine window.setTimeout() function as a spy.
+        window.setTimeout = jasmine.createSpy().andCallFake(function(callback, timeout) { return 5; })
+        window.setTimeout.andReturn(100);
+
         initialize();
         spyOn(videoPlayer, 'onSlideSeek').andCallThrough();
         videoProgressSlider.onStop({}, {
@@ -153,6 +164,13 @@
         });
       });
 
+      afterEach(function () {
+        // Reset the default window.setTimeout() function. If we do not do this,
+        // then all other tests that rely on code which uses window.setTimeout()
+        // function might (and probably will) fail.
+        window.setTimeout = oldSetTimeout;
+      });
+
       it('freeze the slider', function() {
         expect(videoProgressSlider.frozen).toBeTruthy();
       });

common/lib/xmodule/xmodule/js/src/video/025_focus_grabber.js

+/*
+ * 025_focus_grabber.js
+ *
+ * Purpose: Provide a way to focus on autohidden Video controls.
+ *
+ *
+ * Because in HTML player mode we have a feature of autohiding controls on
+ * mouse inactivity, sometimes focus is lost from the currently selected
+ * control. What's more, when all controls are autohidden, we can't get to any
+ * of them because by default browser does not place hidden elements on the
+ * focus chain.
+ *
+ * To get around this minor annoyance, this module will manage 2 placeholder
+ * elements that will be invisible to the user's eye, but visible to the
+ * browser. This will allow for a sneaky stealing of focus and placing it where
+ * we need (on hidden controls).
+ *
+ * This code has been moved to a separate module because it provides a concrete
+ * block of functionality that can be turned on (off).
+ */
+
+/*
+ * "If you want to climb a mountain, begin at the top."
+ *
+ * ~ Zen saying
+ */
+
+(function (requirejs, require, define) {
+
+// FocusGrabber module.
+define(
+'video/025_focus_grabber.js',
+[],
+function () {
+    return function (state) {
+        state.focusGrabber = {};
+
+        _makeFunctionsPublic(state);
+        _renderElements(state);
+        _bindHandlers(state);
+    };
+
+
+    // Private functions.
+
+    function _makeFunctionsPublic(state) {
+        state.focusGrabber.enableFocusGrabber  = _.bind(enableFocusGrabber, state);
+        state.focusGrabber.disableFocusGrabber = _.bind(disableFocusGrabber, state);
+
+        state.focusGrabber.onFocus = _.bind(onFocus, state);
+    }
+
+    function _renderElements(state) {
+        state.focusGrabber.elFirst = state.el.find('.focus_grabber.first');
+        state.focusGrabber.elLast = state.el.find('.focus_grabber.last');
+
+        // From the start, the Focus Grabber must be disabled so that
+        // tabbing (switching focus) does not land the user on one of the
+        // placeholder elements (elFirst, elLast).
+        state.focusGrabber.disableFocusGrabber();
+    }
+
+    function _bindHandlers(state) {
+        state.focusGrabber.elFirst.on('focus', state.focusGrabber.onFocus);
+        state.focusGrabber.elLast.on('focus', state.focusGrabber.onFocus);
+
+        // When the video container element receives programmatic focus, then
+        // on un-focus ('blur' event) we should trigger a 'mousemove' event so
+        // as to reveal autohidden controls.
+        state.el.on('blur', function () {
+            state.el.trigger('mousemove');
+        });
+    }
+
+
+    // Public functions.
+
+    function enableFocusGrabber() {
+        var tabIndex;
+
+        // When the Focus Grabber is being enabled, there are two different
+        // scenarios:
+        //
+        //     1.) Currently focused element was inside the video player.
+        //     2.) Currently focused element was somewhere else on the page.
+        //
+        // In the first case we must make sure that the video player doesn't
+        // loose focus, even though the controls are autohidden.
+        if ($(document.activeElement).parents().hasClass('video')) {
+            tabIndex = -1;
+        } else {
+            tabIndex = 0;
+        }
+
+        this.focusGrabber.elFirst.attr('tabindex', tabIndex);
+        this.focusGrabber.elLast.attr('tabindex', tabIndex);
+
+        // Don't loose focus. We are inside video player on some control, but
+        // because we can't remain focused on a hidden element, we will shift
+        // focus to the main video element.
+        //
+        // Once the main element will receive the un-focus ('blur') event, a
+        // 'mousemove' event will be triggered, and the video controls will
+        // receive focus once again.
+        if (tabIndex === -1) {
+            this.el.focus();
+
+            this.focusGrabber.elFirst.attr('tabindex', 0);
+            this.focusGrabber.elLast.attr('tabindex', 0);
+        }
+    }
+
+    function disableFocusGrabber() {
+        // Only programmatic focusing on these elements will be available.
+        // We don't want the user to focus on them (for example with the 'Tab'
+        // key).
+        this.focusGrabber.elFirst.attr('tabindex', -1);
+        this.focusGrabber.elLast.attr('tabindex', -1);
+    }
+
+    function onFocus(event, params) {
+        // Once the Focus Grabber placeholder elements will gain focus, we will
+        // trigger 'mousemove' event so that the autohidden controls will
+        // become visible.
+        this.el.trigger('mousemove');
+
+        this.focusGrabber.disableFocusGrabber();
+    }
+});
+}(RequireJS.requirejs, RequireJS.require, RequireJS.define));

common/lib/xmodule/xmodule/js/src/video/04_video_control.js

@@ -77,7 +77,7 @@ function () {
             state.el.on('mousemove', state.videoControl.showControls);
             state.el.on('keydown', state.videoControl.showControls);
         }
-        // The state.previousFocus is used in video_speed_control to track 
+        // The state.previousFocus is used in video_speed_control to track
         // the element that had the focus before it.
         state.videoControl.playPauseEl.on('blur', function () {
             state.previousFocus = 'playPause';
@@ -128,6 +128,15 @@ function () {
 
         this.videoControl.el.fadeOut(this.videoControl.fadeOutTimeout, function () {
             _this.controlState = 'invisible';
+
+            // If the focus was on the video control or the volume control,
+            // then we must make sure to close these dialogs. Otherwise, after
+            // next autofocus, these dialogs will be open, but the focus will
+            // not be on them.
+            _this.videoVolumeControl.el.removeClass('open');
+            _this.videoSpeedControl.el.removeClass('open');
+
+            _this.focusGrabber.enableFocusGrabber();
         });
     }
 

common/lib/xmodule/xmodule/js/src/video/10_main.js

@@ -4,6 +4,7 @@
 require(
 [
     'video/01_initialize.js',
+    'video/025_focus_grabber.js',
     'video/04_video_control.js',
     'video/05_video_quality_control.js',
     'video/06_video_progress_slider.js',
@@ -13,6 +14,7 @@ require(
 ],
 function (
     Initialize,
+    FocusGrabber,
     VideoControl,
     VideoQualityControl,
     VideoProgressSlider,
@@ -60,6 +62,7 @@ function (
             youtubeXhr = state.youtubeXhr;
         }
 
+        FocusGrabber(state);
         VideoControl(state);
         VideoQualityControl(state);
         VideoProgressSlider(state);

common/lib/xmodule/xmodule/video_module.py

@@ -136,6 +136,7 @@ class VideoModule(VideoFields, XModule):
     js = {
         'js': [
             resource_string(__name__, 'js/src/video/01_initialize.js'),
+            resource_string(__name__, 'js/src/video/025_focus_grabber.js'),
             resource_string(__name__, 'js/src/video/02_html5_video.js'),
             resource_string(__name__, 'js/src/video/03_video_player.js'),
             resource_string(__name__, 'js/src/video/04_video_control.js'),

lms/templates/video.html

@@ -25,7 +25,11 @@
     data-autoplay="${autoplay}"
     data-yt-test-timeout="${yt_test_timeout}"
     data-yt-test-url="${yt_test_url}"
+
+    tabindex="-1"
 >
+    <div class="focus_grabber first"></div>
+
     <div class="tc-wrapper">
         <article class="video-wrapper">
             <div class="video-player-pre"></div>
@@ -70,6 +74,8 @@
 
         <ol class="subtitles" tabindex="0" title="Captions"><li></li></ol>
     </div>
+
+    <div class="focus_grabber last"></div>
 </div>
 
 % if sources.get('main'):