From 4936b5044bf15f08943deba46ca1bc558f0ced3b Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 21 Aug 2013 14:33:21 +0300 Subject: [PATCH 1/6] Fix for bug BLD-277 "There is a white panel over a non-youtube video." When the captions file is not specified, we simply do not render the captions panel. --- .../xmodule/js/src/video/09_video_caption.js | 35 ++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) 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 a34f33ba4c..d313a7f485 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 @@ -11,7 +11,14 @@ function () { state.videoCaption = {}; _makeFunctionsPublic(state); - state.videoCaption.renderElements(); + + // Depending on whether captions file could be loaded, the following + // function invocation can succeed or fail. If it fails, we do not + // go on with binding handlers to events. + if (!state.videoCaption.renderElements()) { + return; + } + state.videoCaption.bindHandlers(); }; @@ -71,7 +78,16 @@ function () { this.el.find('.video-wrapper').after(this.videoCaption.subtitlesEl); this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); - this.videoCaption.fetchCaption(); + // Fetch the captions file. If no file was specified, then we hide + // the "CC" button, and exit from this module. No further caption + // initialization will happen. + if (!this.videoCaption.fetchCaption()) { + this.videoCaption.hideSubtitlesEl.hide(); + + // Abandon all further operations with captions panel. + return false; + } + this.videoCaption.setSubtitlesHeight(); if (this.videoType === 'html5') { @@ -80,6 +96,8 @@ function () { this.videoCaption.subtitlesEl.addClass('html5'); this.captionHideTimeout = setTimeout(this.videoCaption.autoHideCaptions, this.videoCaption.fadeOutTimeout); } + + return true; } // function bindHandlers() @@ -123,8 +141,12 @@ function () { this.videoCaption.hideCaptions(this.hide_captions); + // Check whether the captions file was specified. This is the point + // where we either stop with the caption panel (so that a white empty + // panel to the right of the video will not be shown), or carry on + // further. if (!this.youtubeId('1.0')) { - return; + return false; } $.ajaxWithPrefix({ @@ -137,13 +159,18 @@ function () { if (onTouchBasedDevice()) { _this.videoCaption.subtitlesEl.find('li').html( - gettext('Caption will be displayed when you start playing the video.') + gettext( + 'Caption will be displayed when ' + + 'you start playing the video.' + ) ); } else { _this.videoCaption.renderCaption(); } } }); + + return true; } function captionURL() { From e788d6ce37f9b7999fbc34ead6dae2ed5c322bbf Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 22 Aug 2013 12:10:03 +0300 Subject: [PATCH 2/6] Added Jasmine test for bug fix. Updated comments. --- .../js/spec/video/video_caption_spec.js | 16 +++++ .../xmodule/js/src/video/01_initialize.js | 10 ++- .../xmodule/js/src/video/09_video_caption.js | 62 ++++++++++++++++--- 3 files changed, 76 insertions(+), 12 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 9458b483da..eaf21b10fd 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 @@ -126,6 +126,22 @@ expect(videoCaption.rendered).toBeFalsy(); }); }); + + describe('when no captions file was specified', function () { + beforeEach(function () { + loadFixtures('video_all.html'); + + // Unspecify the captions file. + $('#example').find('#video_id').data('sub', ''); + + state = new Video('#example'); + videoCaption = state.videoCaption; + }); + + it('captions panel is not shown', function () { + expect(videoCaption.hideSubtitlesEl.css('display')).toBe('none'); + }); + }); }); describe('mouse movement', function() { diff --git a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js index 79bc16dbda..1efb1a1871 100644 --- a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js +++ b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js @@ -25,7 +25,9 @@ function (VideoPlayer) { * * Initialize module exports this function. * - * @param {Object} state A place for all properties, and methods of Video. + * @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. * @param {DOM element} element Container of the entire Video DOM element. */ return function (state, element) { @@ -40,10 +42,12 @@ function (VideoPlayer) { /** * @function _makeFunctionsPublic * - * Functions which will be accessible via 'state' object. When called, these functions will get the 'state' + * Functions which will be accessible via 'state' object. When called, + * these functions will get the 'state' * object as a context. * - * @param {Object} state A place for all properties, and methods of Video. + * @param {object} state The object containg the state (properties, + * methods, modules) of the Video player. */ function _makeFunctionsPublic(state) { state.setSpeed = _.bind(setSpeed, state); 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 d313a7f485..69ec14d4ff 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 @@ -6,7 +6,20 @@ define( [], function () { - // VideoCaption() function - what this module "exports". + /** + * @desc VideoCaption module exports a function. + * + * @type {function} + * @access public + * + * @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} + */ return function (state) { state.videoCaption = {}; @@ -64,11 +77,25 @@ function () { // The magic private function that makes them available and sets up their context is makeFunctionsPublic(). // *************************************************************** - // function renderElements() - // - // Create any necessary DOM elements, attach them, and set their initial configuration. Also - // make the created DOM elements available via the 'state' object. Much easier to work this - // way - you don't have to do repeated jQuery element selects. + /** + * @desc Create any necessary DOM elements, attach them, and set their + * initial configuration. Also make the created DOM elements available + * via the 'state' object. Much easier to work this way - you don't + * have to do repeated jQuery element selects. + * + * @type {function} + * @access public + * + * @this {object} - The object containg the state of the video + * player. All other modules, their parameters, public variables, etc. + * are available via this object. + * + * @returns {boolean} + * true: The function fethched captions successfully, and compltely + * rendered everything related to captions. + * false: The captions were not fetched. Nothing will be rendered, + * and the CC button will be hidden. + */ function renderElements() { this.videoCaption.loaded = false; @@ -79,12 +106,10 @@ function () { this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); // Fetch the captions file. If no file was specified, then we hide - // the "CC" button, and exit from this module. No further caption - // initialization will happen. + // the "CC" button, and return. if (!this.videoCaption.fetchCaption()) { this.videoCaption.hideSubtitlesEl.hide(); - // Abandon all further operations with captions panel. return false; } @@ -136,6 +161,25 @@ function () { } } + /** + * @desc Fetch the caption file specified by the user. Upn successful + * receival of the file, the captions will be rendered. + * + * @type {function} + * @access public + * + * @this {object} - The object containg the state of the video + * player. All other modules, their parameters, public variables, etc. + * are available via this object. + * + * @returns {boolean} + * true: The user specified a caption file. NOTE: if an error happens + * while the specified file is being retrieved (for example the + * file is missing on the server), this function will still return + * true. + * false: No caption file was specified, or an empty string was + * specified. + */ function fetchCaption() { var _this = this; From ba954ff1647b8f3e536ade87d6d9bd0e00a16621 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 27 Aug 2013 10:19:55 +0300 Subject: [PATCH 3/6] Fixing code as per suggestions on PR 771. --- .../xmodule/xmodule/js/src/video/09_video_caption.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) 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 69ec14d4ff..e597f2736c 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 @@ -28,11 +28,9 @@ function () { // Depending on whether captions file could be loaded, the following // function invocation can succeed or fail. If it fails, we do not // go on with binding handlers to events. - if (!state.videoCaption.renderElements()) { - return; + if (state.videoCaption.renderElements()) { + state.videoCaption.bindHandlers(); } - - state.videoCaption.bindHandlers(); }; // *************************************************************** @@ -102,9 +100,6 @@ function () { this.videoCaption.subtitlesEl = this.el.find('ol.subtitles'); this.videoCaption.hideSubtitlesEl = this.el.find('a.hide-subtitles'); - this.el.find('.video-wrapper').after(this.videoCaption.subtitlesEl); - this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); - // Fetch the captions file. If no file was specified, then we hide // the "CC" button, and return. if (!this.videoCaption.fetchCaption()) { @@ -113,6 +108,9 @@ function () { return false; } + this.el.find('.video-wrapper').after(this.videoCaption.subtitlesEl); + this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); + this.videoCaption.setSubtitlesHeight(); if (this.videoType === 'html5') { From 2a4f7f61c57dfac21f2951936eb5cbf586409203 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 28 Aug 2013 13:28:40 +0300 Subject: [PATCH 4/6] Remove caption panel when anything bad happens. Now the captions panel will be shown with captions only after successful retrieval of captions. --- .../xmodule/js/src/video/09_video_caption.js | 61 ++++++++++--------- 1 file changed, 31 insertions(+), 30 deletions(-) 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 e597f2736c..621a2ebfe4 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 @@ -25,12 +25,7 @@ function () { _makeFunctionsPublic(state); - // Depending on whether captions file could be loaded, the following - // function invocation can succeed or fail. If it fails, we do not - // go on with binding handlers to events. - if (state.videoCaption.renderElements()) { - state.videoCaption.bindHandlers(); - } + state.videoCaption.renderElements(); }; // *************************************************************** @@ -100,27 +95,7 @@ function () { this.videoCaption.subtitlesEl = this.el.find('ol.subtitles'); this.videoCaption.hideSubtitlesEl = this.el.find('a.hide-subtitles'); - // Fetch the captions file. If no file was specified, then we hide - // the "CC" button, and return. - if (!this.videoCaption.fetchCaption()) { - this.videoCaption.hideSubtitlesEl.hide(); - - return false; - } - - this.el.find('.video-wrapper').after(this.videoCaption.subtitlesEl); - this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); - - this.videoCaption.setSubtitlesHeight(); - - if (this.videoType === 'html5') { - this.videoCaption.fadeOutTimeout = this.config.fadeOutTimeout; - - this.videoCaption.subtitlesEl.addClass('html5'); - this.captionHideTimeout = setTimeout(this.videoCaption.autoHideCaptions, this.videoCaption.fadeOutTimeout); - } - - return true; + this.videoCaption.fetchCaption(); } // function bindHandlers() @@ -181,8 +156,6 @@ function () { function fetchCaption() { var _this = this; - this.videoCaption.hideCaptions(this.hide_captions); - // Check whether the captions file was specified. This is the point // where we either stop with the caption panel (so that a white empty // panel to the right of the video will not be shown), or carry on @@ -191,10 +164,12 @@ function () { return false; } + // Fetch the captions file. If no file was specified, or if an error + // occurred, then we hide the captions panel, and the "CC" button $.ajaxWithPrefix({ url: _this.videoCaption.captionURL(), notifyOnError: false, - success: function(captions) { + success: function (captions) { _this.videoCaption.captions = captions.text; _this.videoCaption.start = captions.start; _this.videoCaption.loaded = true; @@ -209,6 +184,16 @@ function () { } else { _this.videoCaption.renderCaption(); } + }, + error: function (jqXHR, textStatus, errorThrown) { + console.log('ERROR while fetching captions.'); + console.log( + 'STATUS:', textStatus + ', MESSAGE:', '' + errorThrown + ); + console.log('arguments:', arguments); + + _this.videoCaption.hideCaptions(true); + _this.videoCaption.hideSubtitlesEl.hide(); } }); @@ -300,6 +285,22 @@ function () { _this = this; container = $('
    '); + this.el.find('.video-wrapper').after(this.videoCaption.subtitlesEl); + this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); + + this.videoCaption.setSubtitlesHeight(); + + if (this.videoType === 'html5') { + this.videoCaption.fadeOutTimeout = this.config.fadeOutTimeout; + + this.videoCaption.subtitlesEl.addClass('html5'); + this.captionHideTimeout = setTimeout(this.videoCaption.autoHideCaptions, this.videoCaption.fadeOutTimeout); + } + + this.videoCaption.hideCaptions(this.hide_captions); + + this.videoCaption.bindHandlers(); + $.each(this.videoCaption.captions, function(index, text) { var liEl = $('
  1. '); From 33fa1e2c7f7ca6a9f0d64b9d64355610e549626b Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 28 Aug 2013 17:15:52 +0300 Subject: [PATCH 5/6] Applying code changes as suggested by Anton. Fixed 2 failing Jasmine tests. Removed unnecessary console.log() call. Initializing of variables in a single var statement. --- .../xmodule/xmodule/js/spec/video/video_caption_spec.js | 5 +++-- .../lib/xmodule/xmodule/js/src/video/09_video_caption.js | 9 +++++---- 2 files changed, 8 insertions(+), 6 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 eaf21b10fd..b4d31c66db 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 @@ -53,7 +53,8 @@ expect($.ajaxWithPrefix).toHaveBeenCalledWith({ url: videoCaption.captionURL(), notifyOnError: false, - success: jasmine.any(Function) + success: jasmine.any(Function), + error: jasmine.any(Function), }); }); }); @@ -462,7 +463,7 @@ }); // Temporarily disabled due to intermittent failures - // Fails with error: "InvalidStateError: An attempt was made to + // Fails with error: "InvalidStateError: An attempt was made to // use an object that is not, or is no longer, usable // Expected 0 to equal 14.91." // on Firefox 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 621a2ebfe4..2ebb73c692 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 @@ -95,7 +95,10 @@ function () { this.videoCaption.subtitlesEl = this.el.find('ol.subtitles'); this.videoCaption.hideSubtitlesEl = this.el.find('a.hide-subtitles'); - this.videoCaption.fetchCaption(); + if (!this.videoCaption.fetchCaption()) { + this.videoCaption.hideCaptions(true); + this.videoCaption.hideSubtitlesEl.hide(); + } } // function bindHandlers() @@ -190,7 +193,6 @@ function () { console.log( 'STATUS:', textStatus + ', MESSAGE:', '' + errorThrown ); - console.log('arguments:', arguments); _this.videoCaption.hideCaptions(true); _this.videoCaption.hideSubtitlesEl.hide(); @@ -281,9 +283,8 @@ function () { } function renderCaption() { - var container, + var container = $('
      '), _this = this; - container = $('
        '); this.el.find('.video-wrapper').after(this.videoCaption.subtitlesEl); this.el.find('.video-controls .secondary-controls').append(this.videoCaption.hideSubtitlesEl); From 0a110b672dad85878de13d9381253b558a015212 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 28 Aug 2013 17:23:42 +0300 Subject: [PATCH 6/6] Using native Jasmine-jQuery method to check for hidden el. --- common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b4d31c66db..0b26573568 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 @@ -140,7 +140,7 @@ }); it('captions panel is not shown', function () { - expect(videoCaption.hideSubtitlesEl.css('display')).toBe('none'); + expect(videoCaption.hideSubtitlesEl).toBeHidden(); }); }); });