From 1e686f4763853a906f158bc79e80679f6e4ca872 Mon Sep 17 00:00:00 2001 From: Ari Rizzitano Date: Fri, 2 Dec 2016 16:43:15 -0500 Subject: [PATCH] [video] fix AC-343 [captions] move role attr from li element to nested span (AC-343) [video] fix failing tests for video captions (AC-343) [video] couple more failing tests (AC-343) [video] properly outline the focused caption (AC-343) --- .../xmodule/xmodule/css/video/display.scss | 34 +++++---- .../js/spec/video/video_caption_spec.js | 71 ++++++++++--------- .../xmodule/js/src/video/09_video_caption.js | 21 +++--- .../test/acceptance/pages/lms/video/video.py | 4 +- .../acceptance/pages/studio/video/video.py | 9 ++- 5 files changed, 73 insertions(+), 66 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/video/display.scss b/common/lib/xmodule/xmodule/css/video/display.scss index 79bdf9f09f..556e860f80 100644 --- a/common/lib/xmodule/xmodule/css/video/display.scss +++ b/common/lib/xmodule/xmodule/css/video/display.scss @@ -74,18 +74,18 @@ width: 0px; height: 0px; } - + .downloads-heading { margin: 1em 0 0 0; } - + .wrapper-downloads { display: flex; - + .hd { margin: 0; } - + .wrapper-download-video, .wrapper-download-transcripts, .wrapper-handouts, @@ -95,27 +95,27 @@ @include padding-right($baseline); vertical-align: top; } - + .wrapper-download-video { - + .video-sources { margin: 0; } } - + .wrapper-download-transcripts { - + .list-download-transcripts { margin: 0; padding: 0; list-style: none; - + .transcript-option { margin: 0; } } } - + .branding { @include padding-right(0); @@ -681,9 +681,9 @@ } } } - + &.video-fullscreen { - + .closed-captions { width: 60%; } @@ -705,7 +705,7 @@ .subtitles-menu { height: 100%; margin: 0; - padding: 0; + padding: 0 3px; list-style: none; li { @@ -716,6 +716,10 @@ color: #0074b5; // AA compliant line-height: lh(); + span { + display: block; + } + &.current { color: #333; font-weight: 700; @@ -734,10 +738,10 @@ &:empty { margin-bottom: 0; } - + &.spacing:last-of-type { position: relative; - + .transcript-end { position: absolute; bottom: 0; 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 1ebff4a5fb..7ddda1c092 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 @@ -333,10 +333,10 @@ it('render the transcript', function() { var captionsData = jasmine.stubbedCaption, - items = $('.subtitles li[data-index]'); + $items = $('.subtitles li span[data-index]'); _.each(captionsData.text, function(text, index) { - var item = items.eq(index); + var item = $items.eq(index); expect(parseIntAttribute(item, 'data-index')).toEqual(index); expect(parseIntAttribute(item, 'data-start')).toEqual(captionsData.start[index]); @@ -362,7 +362,7 @@ $.each(handlerList, function(index, handler) { spyOn(state.videoCaption, handler); }); - $('.subtitles li[data-index]').each( + $('.subtitles li span[data-index]').each( function(index, link) { $(link).trigger('mouseover'); expect(state.videoCaption.captionMouseOverOut).toHaveBeenCalled(); @@ -420,10 +420,10 @@ return state.videoCaption.rendered; }).then(function() { var captionsData = jasmine.stubbedCaption, - items = $('.subtitles li[data-index]'); + $items = $('.subtitles li span[data-index]'); _.each(captionsData.text, function(text, index) { - var item = items.eq(index); + var item = $items.eq(index); expect(parseIntAttribute(item, 'data-index')).toEqual(index); expect(parseIntAttribute(item, 'data-start')).toEqual(captionsData.start[index]); @@ -533,8 +533,9 @@ describe('when the player is playing', function() { beforeEach(function() { state.videoCaption.playing = true; - $('.subtitles-menu li[data-index]:first') - .addClass('current'); + $('.subtitles-menu span[data-index]:first') + .parent() + .addClass('current'); $('.subtitles-menu').trigger(jQuery.Event('mouseout')); }); @@ -832,7 +833,7 @@ captionsData = jasmine.stubbedCaption; - $('.subtitles li[data-index]').each( + $('.subtitles li span[data-index]').each( function(index, item) { expect(parseIntAttribute($(item), 'data-index')).toEqual(index); expect(parseIntAttribute($(item), 'data-start')).toEqual(captionsData.start[index]); @@ -913,17 +914,17 @@ describe('when the index is not the same', function() { beforeEach(function() { state.videoCaption.currentIndex = 1; - $('.subtitles li[data-index=5]').addClass('current'); + $('.subtitles li span[data-index=5]').addClass('current'); state.videoCaption.updatePlayTime(25.000); }); it('deactivate the previous transcript', function() { - expect($('.subtitles li[data-index=1]')) + expect($('.subtitles li span[data-index=1]')) .not.toHaveClass('current'); }); it('activate new transcript', function() { - expect($('.subtitles li[data-index=5]')) + expect($('.subtitles li span[data-index=5]')) .toHaveClass('current'); }); @@ -939,9 +940,9 @@ describe('when the index is the same', function() { it('does not change current subtitle', function() { state.videoCaption.currentIndex = 1; - $('.subtitles li[data-index=3]').addClass('current'); + $('.subtitles li span[data-index=3]').addClass('current'); state.videoCaption.updatePlayTime(15.000); - expect($('.subtitles li[data-index=3]')) + expect($('.subtitles li span[data-index=3]')) .toHaveClass('current'); }); }); @@ -955,7 +956,7 @@ return state.videoCaption.rendered; }).then(function() { videoControl = state.videoControl; - $('.subtitles li[data-index=1]').addClass('current'); + $('.subtitles li span[data-index=1]').addClass('current'); state.videoCaption.onResize(); }).always(done); }); @@ -1029,7 +1030,7 @@ it('does not scroll the transcript', function() { runs(function() { state.videoCaption.frozen = true; - $('.subtitles li[data-index=1]').addClass('current'); + $('.subtitles li span[data-index=1]').addClass('current'); state.videoCaption.scrollCaption(); expect($.fn.scrollTo).not.toHaveBeenCalled(); }); @@ -1055,7 +1056,7 @@ describe('when there is a current transcript', function() { it('scroll to current transcript', function() { runs(function() { - $('.subtitles li[data-index=1]').addClass('current'); + $('.subtitles li span[data-index=1]').addClass('current'); state.videoCaption.scrollCaption(); expect($.fn.scrollTo).toHaveBeenCalled(); }); @@ -1082,7 +1083,7 @@ it('trigger seek event with the correct time', function() { runs(function() { state.videoSpeedControl.currentSpeed = '1.0'; - $('.subtitles li[data-start="14910"]').trigger('click'); + $('.subtitles li span[data-start="14910"]').trigger('click'); expect(state.videoPlayer.currentTime).toEqual(14.91); }); }); @@ -1092,7 +1093,7 @@ it('trigger seek event with the correct time', function() { runs(function() { state.videoSpeedControl.currentSpeed = '0.75'; - $('.subtitles li[data-start="14910"]').trigger('click'); + $('.subtitles li span[data-start="14910"]').trigger('click'); expect(state.videoPlayer.currentTime).toEqual(14.91); }); }); @@ -1104,7 +1105,7 @@ runs(function() { state.videoSpeedControl.currentSpeed = '0.75'; state.currentPlayerMode = 'flash'; - $('.subtitles li[data-start="14910"]').trigger('click'); + $('.subtitles li span[data-start="14910"]').trigger('click'); expect(state.videoPlayer.currentTime).toEqual(15); }); }); @@ -1114,7 +1115,7 @@ describe('toggle', function() { beforeEach(function() { state = jasmine.initializePlayer(); - $('.subtitles li[data-index=1]').addClass('current'); + $('.subtitles li span[data-index=1]').addClass('current'); }); describe('when the transcript is visible', function() { @@ -1172,13 +1173,13 @@ describe('when getting focus through TAB key', function() { beforeEach(function() { state.videoCaption.isMouseFocus = false; - $('.subtitles li[data-index=0]').trigger( + $('.subtitles li span[data-index=0]').trigger( jQuery.Event('focus') ); }); it('shows an outline around the transcript', function() { - expect($('.subtitles li[data-index=0]')) + expect($('.subtitles span[data-index=0]').parent()) .toHaveClass('focused'); }); @@ -1189,13 +1190,13 @@ describe('when loosing focus through TAB key', function() { beforeEach(function() { - $('.subtitles li[data-index=0]').trigger( + $('.subtitles li span[data-index=0]').trigger( jQuery.Event('blur') ); }); it('does not show an outline around the transcript', function() { - expect($('.subtitles li[data-index=0]')) + expect($('.subtitles li span[data-index=0]')) .not.toHaveClass('focused'); }); @@ -1210,14 +1211,14 @@ function() { beforeEach(function() { state.videoCaption.isMouseFocus = false; - $('.subtitles li[data-index=0]') + $('.subtitles li span[data-index=0]') .trigger(jQuery.Event('focus')); - $('.subtitles li[data-index=0]') + $('.subtitles li span[data-index=0]') .trigger(jQuery.Event('mousedown')); }); it('does not show an outline around it', function() { - expect($('.subtitles li[data-index=0]')) + expect($('.subtitles li span[data-index=0]')) .not.toHaveClass('focused'); }); @@ -1230,28 +1231,28 @@ 'when a second transcript gets focus through mouse after ' + 'first had focus through TAB key', function() { - var subDataLiIdx__0, subDataLiIdx__1; + var $subDataLiIdx0, $subDataLiIdx1; beforeEach(function() { - subDataLiIdx__0 = $('.subtitles li[data-index=0]'); - subDataLiIdx__1 = $('.subtitles li[data-index=1]'); + $subDataLiIdx0 = $('.subtitles li span[data-index=0]'); + $subDataLiIdx1 = $('.subtitles li span[data-index=1]'); state.videoCaption.isMouseFocus = false; - subDataLiIdx__0.trigger(jQuery.Event('focus')); - subDataLiIdx__0.trigger(jQuery.Event('blur')); + $subDataLiIdx0.trigger(jQuery.Event('focus')); + $subDataLiIdx0.trigger(jQuery.Event('blur')); state.videoCaption.isMouseFocus = true; - subDataLiIdx__1.trigger(jQuery.Event('mousedown')); + $subDataLiIdx1.trigger(jQuery.Event('mousedown')); }); it('does not show an outline around the first', function() { - expect(subDataLiIdx__0).not.toHaveClass('focused'); + expect($subDataLiIdx0).not.toHaveClass('focused'); }); it('does not show an outline around the second', function() { - expect(subDataLiIdx__1).not.toHaveClass('focused'); + expect($subDataLiIdx1).not.toHaveClass('focused'); }); it('has automatic scrolling enabled', function() { 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 b784ae6702..a99cecbccd 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 @@ -161,7 +161,7 @@ mousewheel: this.onMovement, DOMMouseScroll: this.onMovement }) - .on(events, 'li[data-index]', this.onCaptionHandler); + .on(events, 'span[data-index]', this.onCaptionHandler); this.container.on({ mouseenter: this.onContainerMouseEnter, mouseleave: this.onContainerMouseLeave @@ -739,16 +739,16 @@ */ buildCaptions: function(container, start, captions) { var process = function(text, index) { - var liEl = $('
  • ', { + var $spanEl = $('', { 'role': 'link', 'data-index': index, 'data-start': start[index], 'tabindex': 0 }); - HtmlUtils.setHtml($(liEl), HtmlUtils.HTML(text.toString())); + HtmlUtils.setHtml($($spanEl), HtmlUtils.HTML(text.toString())); - return liEl[0]; + return $spanEl.wrap('
  • ').parent()[0]; // safe-lint: disable=javascript-jquery-insertion }; return AsyncProcess.array(captions, process).done(function(list) { @@ -912,20 +912,21 @@ */ captionFocus: function(event) { var caption = $(event.target), + container = caption.parent(), captionIndex = parseInt(caption.attr('data-index'), 10); // If the focus comes from a mouse click, hide the outline, turn on // automatic scrolling and set currentCaptionIndex to point outside of // caption list (ie -1) to disable mouseenter, mouseleave behavior. if (this.isMouseFocus) { this.autoScrolling = true; - caption.removeClass('focused'); + container.removeClass('focused'); this.currentCaptionIndex = -1; } // If the focus comes from tabbing, show the outline and turn off // automatic scrolling. else { this.currentCaptionIndex = captionIndex; - caption.addClass('focused'); + container.addClass('focused'); // The second and second to last elements turn automatic scrolling // off again as it may have been enabled in captionBlur. if ( @@ -945,9 +946,10 @@ */ captionBlur: function(event) { var caption = $(event.target), + container = caption.parent(), captionIndex = parseInt(caption.attr('data-index'), 10); - caption.removeClass('focused'); + container.removeClass('focused'); // If we are on first or last index, we have to turn automatic scroll // on again when losing focus. There is no way to know in what // direction we are tabbing. So we could be on the first element and @@ -1057,11 +1059,12 @@ } this.subtitlesEl - .find("li[data-index='" + newIndex + "']") + .find("span[data-index='" + newIndex + "']") + .parent() .addClass('current'); this.currentIndex = newIndex; - this.captionDisplayEl.text(this.subtitlesEl.find("li[data-index='" + newIndex + "']").text()); + this.captionDisplayEl.text(this.subtitlesEl.find("span[data-index='" + newIndex + "']").text()); this.scrollCaption(); } } diff --git a/common/test/acceptance/pages/lms/video/video.py b/common/test/acceptance/pages/lms/video/video.py index 153a22fdb6..9c2a4a0d72 100644 --- a/common/test/acceptance/pages/lms/video/video.py +++ b/common/test/acceptance/pages/lms/video/video.py @@ -32,8 +32,8 @@ CSS_CLASS_NAMES = { 'captions_closed': '.video.closed', 'captions_rendered': '.video.is-captions-rendered', 'captions': '.subtitles', - 'captions_text': '.subtitles li', - 'captions_text_getter': '.subtitles li[role="link"][data-index="1"]', + 'captions_text': '.subtitles li span', + 'captions_text_getter': '.subtitles li span[role="link"][data-index="1"]', 'closed_captions': '.closed-captions', 'error_message': '.video .video-player .video-error', 'video_container': '.video', diff --git a/common/test/acceptance/pages/studio/video/video.py b/common/test/acceptance/pages/studio/video/video.py index 9abd633075..974be6fe39 100644 --- a/common/test/acceptance/pages/studio/video/video.py +++ b/common/test/acceptance/pages/studio/video/video.py @@ -277,7 +277,7 @@ class VideoComponentPage(VideoPage): line_number (int): caption line number """ - caption_line_selector = ".subtitles li[data-index='{index}']".format(index=line_number - 1) + caption_line_selector = ".subtitles li span[data-index='{index}']".format(index=line_number - 1) self.q(css=caption_line_selector).results[0].send_keys(Keys.ENTER) def is_caption_line_focused(self, line_number): @@ -288,10 +288,9 @@ class VideoComponentPage(VideoPage): line_number (int): caption line number """ - caption_line_selector = ".subtitles li[data-index='{index}']".format(index=line_number - 1) - attributes = self.q(css=caption_line_selector).attrs('class') - - return 'focused' in attributes + caption_line_selector = ".subtitles li span[data-index='{index}']".format(index=line_number - 1) + caption_container = self.q(css=caption_line_selector).results[0].find_element_by_xpath('..') + return 'focused' in caption_container.get_attribute('class').split() @property def is_slider_range_visible(self):