From 68eddcee38f7001aaea545508e4d26a76d90ec91 Mon Sep 17 00:00:00 2001 From: Matjaz Gregoric Date: Tue, 2 Jun 2020 09:37:40 +0200 Subject: [PATCH] Fix issues with video transcript panel This fixes a bug where the video transcripts panel would be open on page load even when no transcripts were present. It also cleans up the code a bit to remove unused cookies and tries to reduce flickering of the transcript panel. --- .../js/spec/video/video_caption_spec.js | 56 +++++++-- .../xmodule/js/src/video/01_initialize.js | 29 ----- .../xmodule/js/src/video/09_video_caption.js | 116 +++++++++--------- 3 files changed, 102 insertions(+), 99 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 17cc31b7c8..f4ca156fd0 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 @@ -94,6 +94,38 @@ }); }); + it('adds "closed" class to the main element if transcript setting is off', function() { + // No cookie, showCaptions setting is off: hide transcripts panel. + $.cookie.and.returnValue(null); + state = jasmine.initializePlayer('video_all.html', {showCaptions: false}); + expect(state.el).toHaveClass('closed'); + + // No cookie, showCaptions setting is on: show transcripts panel. + $.cookie.and.returnValue(null); + state = jasmine.initializePlayer('video_all.html', {showCaptions: true}); + expect(state.el).not.toHaveClass('closed'); + + // Cookie preference is on, showCaptions setting is off: hide transcripts panel. + $.cookie.and.returnValue('true'); + state = jasmine.initializePlayer('video_all.html', {showCaptions: false}); + expect(state.el).toHaveClass('closed'); + + // Cookie preference is on, showCaptions setting is on: show transcripts panel. + $.cookie.and.returnValue('true'); + state = jasmine.initializePlayer('video_all.html', {showCaptions: true}); + expect(state.el).not.toHaveClass('closed'); + + // Cookie preference is off, showCaptions setting is off: hide transcripts panel. + $.cookie.and.returnValue('false'); + state = jasmine.initializePlayer('video_all.html', {showCaptions: false}); + expect(state.el).toHaveClass('closed'); + + // Cookie preference is off, showCaptions setting is on: hide transcripts panel. + $.cookie.and.returnValue('false'); + state = jasmine.initializePlayer('video_all.html', {showCaptions: true}); + expect(state.el).toHaveClass('closed'); + }); + it('fetch the transcript in HTML5 mode', function(done) { var transcriptURL = '/transcript/translation/en', transcriptCall; @@ -623,19 +655,19 @@ 'loaded yet'; it(msg, function() { Caption.loaded = false; - state.hideCaptions = false; + Caption.hideCaptionsOnLoad = false; Caption.fetchCaption(); expect($.ajaxWithPrefix).toHaveBeenCalled(); - expect(Caption.hideCaptions).toHaveBeenCalledWith(false, false); + expect(Caption.hideCaptions).toHaveBeenCalledWith(false); Caption.loaded = false; Caption.hideCaptions.calls.reset(); - state.hideCaptions = true; + Caption.hideCaptionsOnLoad = true; Caption.fetchCaption(); expect($.ajaxWithPrefix).toHaveBeenCalled(); - expect(Caption.hideCaptions).toHaveBeenCalledWith(true, false); + expect(Caption.hideCaptions).toHaveBeenCalledWith(true); }); it('on success: on touch devices', function() { @@ -714,8 +746,7 @@ expect($.ajaxWithPrefix).toHaveBeenCalled(); expect(Caption.fetchAvailableTranslations).not.toHaveBeenCalled(); - expect(Caption.hideCaptions.calls.mostRecent().args) - .toEqual([true, false]); + expect(Caption.hideCaptions.calls.mostRecent().args[0]).toEqual(true); }); msg = 'on error: for Html5 player an attempt to fetch transcript ' + @@ -735,8 +766,7 @@ expect(Caption.fetchAvailableTranslations).not.toHaveBeenCalled(); expect($.ajaxWithPrefix.calls.mostRecent().args[0].data) .toEqual({videoId: 'Z5KLxerq05Y'}); - expect(Caption.hideCaptions.calls.mostRecent().args) - .toEqual([true, false]); + expect(Caption.hideCaptions.calls.mostRecent().args[0]).toEqual(true); expect(Caption.fetchCaption.calls.mostRecent().args[0]).toEqual(true); expect(Caption.fetchCaption.calls.count()).toEqual(2); }); @@ -846,8 +876,8 @@ Caption.fetchAvailableTranslations(); expect($.ajaxWithPrefix).toHaveBeenCalled(); - expect(Caption.hideCaptions).toHaveBeenCalledWith(true, false); - expect(Caption.subtitlesEl).toBeHidden(); + expect(Caption.hideCaptions).toHaveBeenCalledWith(true); + expect(Caption.languageChooserEl).toBeHidden(); }); }); @@ -1151,7 +1181,7 @@ }); }); - describe('toggle', function() { + describe('toggleTranscript', function() { beforeEach(function() { state = jasmine.initializePlayer(); $('.subtitles li span[data-index=1]').addClass('current'); @@ -1160,7 +1190,7 @@ describe('when the transcript is visible', function() { beforeEach(function() { state.el.removeClass('closed'); - state.videoCaption.toggle(jQuery.Event('click')); + state.videoCaption.toggleTranscript(jQuery.Event('click')); }); it('hide the transcript', function() { @@ -1171,7 +1201,7 @@ describe('when the transcript is hidden', function() { beforeEach(function() { state.el.addClass('closed'); - state.videoCaption.toggle(jQuery.Event('click')); + state.videoCaption.toggleTranscript(jQuery.Event('click')); jasmine.clock().install(); }); 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 6051cc4286..72a8092e2c 100644 --- a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js +++ b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js @@ -232,34 +232,6 @@ function(VideoPlayer, i18n, moment, _) { firstScriptTag.parentNode.insertBefore(scriptTag, firstScriptTag); } - // function _configureCaptions(state) - // Configure displaying of captions. - // - // Option - // this.config.showCaptions = true | false - // - // Defines whether or not captions are shown on first viewing. - // - // Option - // this.hideCaptions = true | false - // - // represents the user's choice of having the subtitles shown or - // hidden. This choice is stored in cookies. - function _configureCaptions(state) { - if (state.config.showCaptions) { - state.hideCaptions = ($.cookie('hide_captions') === 'true'); - } else { - state.hideCaptions = true; - - $.cookie('hide_captions', state.hideCaptions, { - expires: 3650, - path: '/' - }); - - state.el.addClass('closed'); - } - } - // function _parseYouTubeIDs(state) // The function parse YouTube stream ID's. // @return @@ -333,7 +305,6 @@ function(VideoPlayer, i18n, moment, _) { } function _setConfigurations(state) { - _configureCaptions(state); state.setPlayerMode(state.config.mode); // Possible value are: 'visible', 'hiding', and 'invisible'. state.controlState = 'visible'; 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 af67bef637..70b68a6e05 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 @@ -29,22 +29,22 @@ return new VideoCaption(state); } - _.bindAll(this, 'toggle', 'onMouseEnter', 'onMouseLeave', 'onMovement', + _.bindAll(this, 'toggleTranscript', 'onMouseEnter', 'onMouseLeave', 'onMovement', 'onContainerMouseEnter', 'onContainerMouseLeave', 'fetchCaption', 'onResize', 'pause', 'play', 'onCaptionUpdate', 'onCaptionHandler', 'destroy', 'handleKeypress', 'handleKeypressLink', 'openLanguageMenu', 'closeLanguageMenu', 'previousLanguageMenuItem', 'nextLanguageMenuItem', 'handleCaptionToggle', 'showClosedCaptions', 'hideClosedCaptions', 'toggleClosedCaptions', 'updateCaptioningCookie', 'handleCaptioningCookie', 'handleTranscriptToggle', - 'listenForDragDrop', 'handleTranscriptCookie', 'updateTranscriptCookie' + 'listenForDragDrop', 'setTranscriptVisibility', 'updateTranscriptCookie' ); this.state = state; this.state.videoCaption = this; this.renderElements(); this.handleCaptioningCookie(); + this.setTranscriptVisibility(); this.listenForDragDrop(); - this.handleTranscriptCookie(); return $.Deferred().resolve().promise(); }; @@ -154,7 +154,7 @@ keydown: this.handleCaptionToggle }); this.transcriptControlEl.on({ - click: this.toggle, + click: this.toggleTranscript, keydown: this.handleTranscriptToggle }); this.subtitlesMenuEl.on({ @@ -222,7 +222,7 @@ case KEY.SPACE: case KEY.ENTER: event.preventDefault(); - this.toggle(event); + this.toggleTranscript(event); // no default } }, @@ -563,7 +563,7 @@ } else { self.renderCaption(start, captions); } - self.hideCaptions(state.hideCaptions, false); + self.hideCaptions(self.hideCaptionsOnLoad); HtmlUtils.append( self.state.el.find('.video-wrapper').parent(), HtmlUtils.HTML(self.subtitlesEl) @@ -578,6 +578,7 @@ self.loaded = true; }, error: function(jqXHR, textStatus, errorThrown) { + var canFetchWithYoutubeId; console.log('[Video info]: ERROR while fetching captions.'); console.log( '[Video info]: STATUS:', textStatus + @@ -591,13 +592,16 @@ if (_.keys(state.config.transcriptLanguages).length > 1) { self.fetchAvailableTranslations(); } else if (!fetchWithYoutubeId && state.videoType === 'html5') { - console.log('[Video info]: Html5 mode fetching caption with youtubeId.'); - self.fetchCaption(true); + canFetchWithYoutubeId = self.fetchCaption(true); + if (canFetchWithYoutubeId) { + console.log('[Video info]: Html5 mode fetching caption with youtubeId.'); // eslint-disable-line max-len, no-console + } else { + self.hideCaptions(true); + self.languageChooserEl.hide(); + } } else { - self.hideCaptions(true, false); - self.state.el.find('.lang').hide(); - self.state.el.find('.transcript-control').hide(); - self.subtitlesEl.hide(); + self.hideCaptions(true); + self.languageChooserEl.hide(); } } }); @@ -632,10 +636,8 @@ } }, error: function() { - self.hideCaptions(true, false); - self.state.el.find('.lang').hide(); - self.state.el.find('.transcript-control').hide(); - self.subtitlesEl.hide(); + self.hideCaptions(true); + self.languageChooserEl.hide(); } }); @@ -1136,23 +1138,6 @@ ); }, - /** - * @desc Shows/Hides transcript on click `transcript` button - * - * @param {jquery Event} event - * - */ - toggle: function(event) { - event.preventDefault(); - if (this.state.el.hasClass('closed')) { - this.hideCaptions(false, true, true); - this.updateTranscriptCookie(true); - } else { - this.hideCaptions(true, true, true); - this.updateTranscriptCookie(false); - } - }, - handleCaptioningCookie: function() { if ($.cookie('show_closed_captions') === 'true') { this.state.showClosedCaptions = true; @@ -1235,19 +1220,48 @@ }); } }, - handleTranscriptCookie: function() { - if ($.cookie('show_transcript') === null) { - return; + + /** + * This runs when the video block is first rendered and sets the initial visibility + * of the transcript panel based on the value of the 'show_transcript' cookie and/or + * the block's showCaptions setting. + */ + setTranscriptVisibility: function() { + var hideCaptionsOnRender = !this.state.config.showCaptions; + + if ($.cookie('show_transcript') === 'true') { + this.hideCaptionsOnLoad = false; + // Keep it going until turned off. + this.updateTranscriptCookie(true); + } else if ($.cookie('show_transcript') === 'false') { + hideCaptionsOnRender = true; + this.hideCaptionsOnLoad = true; + } else { + this.hideCaptionsOnLoad = !this.state.config.showCaptions; } - if ($.cookie('show_transcript') !== 'false') { - this.state.hideCaptions = false; - // keep it going until turned off or in case of null initially change to true + + if (hideCaptionsOnRender) { + this.state.el.addClass('closed'); + } + }, + + /** + * @desc Shows/Hides transcript on click `transcript` button + * + * @param {jquery Event} event + * + */ + toggleTranscript: function(event) { + event.preventDefault(); + if (this.state.el.hasClass('closed')) { + this.hideCaptions(false, true); this.updateTranscriptCookie(true); } else { - this.state.hideCaptions = true; + this.hideCaptions(true, true); + this.updateTranscriptCookie(false); } - this.hideCaptions(this.state.hideCaptions, true, true); }, + updateTranscriptCookie: function(showTranscript) { if (showTranscript) { $.cookie('show_transcript', 'true', { @@ -1272,22 +1286,16 @@ }, /** - * @desc Shows/Hides captions and updates the cookie. + * @desc Shows/Hides the transcript panel. * - * @param {boolean} hideCaptions if `true` hides the caption, + * @param {boolean} hideCaptions if `true` hides the transcript panel, * otherwise - show. - * @param {boolean} updateCookie Flag to update or not the cookie. - * */ - hideCaptions: function(hideCaptions, updateCookie, triggerEvent) { + hideCaptions: function(hideCaptions, triggerEvent) { var transcriptControlEl = this.transcriptControlEl, state = this.state, text; - if (typeof updateCookie === 'undefined') { - updateCookie = true; - } - if (hideCaptions) { state.captionsHidden = true; state.el.addClass('closed'); @@ -1324,12 +1332,6 @@ } this.setSubtitlesHeight(); - if (updateCookie) { - $.cookie('hideCaptions', hideCaptions, { - expires: 3650, - path: '/' - }); - } }, /** @@ -1355,7 +1357,7 @@ var height = 0, state = this.state; // on page load captionHidden = undefined - if ((state.captionsHidden === undefined && state.hideCaptions) || + if ((state.captionsHidden === undefined && this.hideCaptionsOnLoad) || state.captionsHidden === true ) { // In case of html5 autoshowing subtitles, we adjust height of