From fe849dedd91f62c41dc8231ee1e4082ff8f1f35e Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Thu, 31 May 2012 16:28:19 -0400 Subject: [PATCH 1/7] Do not display play/pause button on iOS Device iOS devices has a restriction that user has to click on the video element to initiate playback. By visually disable the button, user will be forced to click on the video itself, and everything will be worked as expected. Fixes https://www.pivotaltracker.com/story/show/30334381 --- .../modules/video/video_control_spec.coffee | 89 ++++++++++++++----- .../src/modules/video/video_control.coffee | 16 ++-- 2 files changed, 78 insertions(+), 27 deletions(-) diff --git a/static/coffee/spec/modules/video/video_control_spec.coffee b/static/coffee/spec/modules/video/video_control_spec.coffee index a912abb2d0..0c8615b8f7 100644 --- a/static/coffee/spec/modules/video/video_control_spec.coffee +++ b/static/coffee/spec/modules/video/video_control_spec.coffee @@ -1,17 +1,16 @@ describe 'VideoControl', -> beforeEach -> @player = jasmine.stubVideoPlayer @ + $('.video-controls').html '' describe 'constructor', -> - beforeEach -> - @control = new VideoControl @player - it 'render the video controls', -> + new VideoControl @player expect($('.video-controls').html()).toContain '''
- """ + """ + + unless onTouchBasedDevice() + @$('.video_control').addClass('play').html('Play') onPlay: => @$('.video_control').removeClass('play').addClass('pause').html('Pause') @@ -36,7 +39,8 @@ class @VideoControl togglePlayback: (event) => event.preventDefault() - if @player.isPlaying() - $(@player).trigger('pause') - else - $(@player).trigger('play') + if $('.video_control').hasClass('play') || $('.video_control').hasClass('pause') + if @player.isPlaying() + $(@player).trigger('pause') + else + $(@player).trigger('play') From a6852eefb8d518b3bd7e5571c0e27f31e36a7802 Mon Sep 17 00:00:00 2001 From: Kyle Fiedler Date: Fri, 1 Jun 2012 14:35:18 -0400 Subject: [PATCH 2/7] Sprited vcr controlls and added default bg image --- static/images/pause-icon.png | Bin 102 -> 0 bytes static/images/play-icon.png | Bin 179 -> 0 bytes static/images/vcr.png | Bin 0 -> 584 bytes static/sass/courseware/_video.scss | 5 +++-- 4 files changed, 3 insertions(+), 2 deletions(-) delete mode 100644 static/images/pause-icon.png delete mode 100644 static/images/play-icon.png create mode 100644 static/images/vcr.png diff --git a/static/images/pause-icon.png b/static/images/pause-icon.png deleted file mode 100644 index e3eac519472b7d8715ef5567595c8cae2db0c534..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 102 zcmeAS@N?(olHy`uVBq!ia0vp^JRr=$1|-8uW1a&kT~8OskcwMxPaEdut=Bkj$2o{%0GP6zW_1)a?@xsBmDsSjor#bBp+UphgByS3j3^P6}{y&o1JUHn&XQ6^cGjoOLtM`f`nqQtDKK=5b|D&WH+XAsm76-YGXf0>C zP-yv_QF&r_j>C>Q#f{aaIo}}SnuWt@zM8Gm_PmRao>-jg%a?F1 eO08N=Q+|_vC9M+yl4S z;hvd0Gg}NZK#Xb?sMmpB4;T!hxV??ORjZiI+#Jwo0AUD_(<2Olqa$E`eoFGi#q7lI z(QZdMV-60ICy%1h_)K5Q z0BE(!0ulTlfglKiAP9mW2*N)m+r9gQ;QE?8JS1a} z64D4rvfCw>mvab6^5uox-p(Ny;dzNmnl2#8n;UXvC5K>C(#}o}!HCn-StTVr`aV2F z7uTsr@bpA(Y-B8<-6ofoGE8C+ko)`Nw*~1035GmdkZ{OxLAOip?Ij*^T##V5{lEu0 WNP6m=6v~$X0000 Date: Fri, 1 Jun 2012 15:06:24 -0400 Subject: [PATCH 3/7] Added styles for non play button --- static/sass/courseware/_video.scss | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/static/sass/courseware/_video.scss b/static/sass/courseware/_video.scss index 6420379f2b..54c3fd8e95 100644 --- a/static/sass/courseware/_video.scss +++ b/static/sass/courseware/_video.scss @@ -137,6 +137,7 @@ section.course-content { float: left; margin-bottom: 0; + a { border-bottom: none; border-right: 1px solid #000; @@ -146,10 +147,15 @@ section.course-content { line-height: 46px; padding: 0 lh(.75); text-indent: -9999px; - @include transition(); + @include transition(background-color, opacity); width: 14px; background: url('../images/vcr.png') 15px 15px no-repeat; + &:empty { + height: 46px; + background: url('../images/vcr.png') 15px 15px no-repeat; + } + &.play { background-position: 17px -114px; From 3f7dc68828d89a5933f8ec131d16502be97ddb75 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Fri, 1 Jun 2012 17:55:59 -0400 Subject: [PATCH 4/7] Lazily build video slider when on iOS device This will fix the problem when user trying to click the seek button on the iOS, as the video need to be started first before it can be seekable. --- .../video/video_progress_slider_spec.coffee | 106 +++++++++++++----- .../video/video_progress_slider.coffee | 12 +- 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/static/coffee/spec/modules/video/video_progress_slider_spec.coffee b/static/coffee/spec/modules/video/video_progress_slider_spec.coffee index b2ade26ba8..ba7d439a94 100644 --- a/static/coffee/spec/modules/video/video_progress_slider_spec.coffee +++ b/static/coffee/spec/modules/video/video_progress_slider_spec.coffee @@ -3,34 +3,51 @@ describe 'VideoProgressSlider', -> @player = jasmine.stubVideoPlayer @ describe 'constructor', -> - beforeEach -> - spyOn($.fn, 'slider').andCallThrough() - @slider = new VideoProgressSlider @player + describe 'on a non-touch based device', -> + beforeEach -> + spyOn($.fn, 'slider').andCallThrough() + spyOn(window, 'onTouchBasedDevice').andReturn false + @slider = new VideoProgressSlider @player - it 'build the slider', -> - expect(@slider.slider).toBe '.slider' - expect($.fn.slider).toHaveBeenCalledWith - range: 'min' - change: @slider.onChange - slide: @slider.onSlide - stop: @slider.onStop + it 'build the slider', -> + expect(@slider.slider).toBe '.slider' + expect($.fn.slider).toHaveBeenCalledWith + range: 'min' + change: @slider.onChange + slide: @slider.onSlide + stop: @slider.onStop - it 'build the seek handle', -> - expect(@slider.handle).toBe '.ui-slider-handle' - expect($.fn.qtip).toHaveBeenCalledWith - content: "0:00" - position: - my: 'bottom center' - at: 'top center' - container: @slider.handle - hide: - delay: 700 - style: - classes: 'ui-tooltip-slider' - widget: true + it 'build the seek handle', -> + expect(@slider.handle).toBe '.ui-slider-handle' + expect($.fn.qtip).toHaveBeenCalledWith + content: "0:00" + position: + my: 'bottom center' + at: 'top center' + container: @slider.handle + hide: + delay: 700 + style: + classes: 'ui-tooltip-slider' + widget: true - it 'bind player events', -> - expect($(@player)).toHandleWith 'updatePlayTime', @slider.onUpdatePlayTime + it 'bind player events', -> + expect($(@player)).toHandleWith 'updatePlayTime', @slider.onUpdatePlayTime + expect($(@player)).toHandleWith 'ready', @slider.onReady + expect($(@player)).toHandleWith 'play', @slider.onPlay + + describe 'on a touch-based device', -> + beforeEach -> + spyOn($.fn, 'slider').andCallThrough() + spyOn(window, 'onTouchBasedDevice').andReturn true + @slider = new VideoProgressSlider @player + + it 'does not build the slider', -> + expect(@slider.slider).toBeUndefined + expect($.fn.slider).not.toHaveBeenCalled() + + it 'bind player events', -> + expect($(@player)).toHandleWith 'updatePlayTime', @slider.onUpdatePlayTime describe 'onReady', -> beforeEach -> @@ -41,6 +58,45 @@ describe 'VideoProgressSlider', -> it 'set the max value to the length of video', -> expect(@slider.slider.slider('option', 'max')).toEqual 120 + describe 'onPlay', -> + beforeEach -> + @slider = new VideoProgressSlider @player + spyOn($.fn, 'slider').andCallThrough() + + describe 'when the slider was already built', -> + beforeEach -> + @slider.onPlay() + + it 'does not build the slider', -> + expect($.fn.slider).not.toHaveBeenCalled + + describe 'when the slider was not already built', -> + beforeEach -> + @slider.slider = null + @slider.onPlay() + + it 'build the slider', -> + expect(@slider.slider).toBe '.slider' + expect($.fn.slider).toHaveBeenCalledWith + range: 'min' + change: @slider.onChange + slide: @slider.onSlide + stop: @slider.onStop + + it 'build the seek handle', -> + expect(@slider.handle).toBe '.ui-slider-handle' + expect($.fn.qtip).toHaveBeenCalledWith + content: "0:00" + position: + my: 'bottom center' + at: 'top center' + container: @slider.handle + hide: + delay: 700 + style: + classes: 'ui-tooltip-slider' + widget: true + describe 'onUpdatePlayTime', -> beforeEach -> @slider = new VideoProgressSlider @player diff --git a/static/coffee/src/modules/video/video_progress_slider.coffee b/static/coffee/src/modules/video/video_progress_slider.coffee index 4dfd558e4c..923f06b0c3 100644 --- a/static/coffee/src/modules/video/video_progress_slider.coffee +++ b/static/coffee/src/modules/video/video_progress_slider.coffee @@ -1,9 +1,9 @@ class @VideoProgressSlider constructor: (@player) -> - @buildSlider() - @buildHandle() + @buildSlider() unless onTouchBasedDevice() $(@player).bind('updatePlayTime', @onUpdatePlayTime) $(@player).bind('ready', @onReady) + $(@player).bind('play', @onPlay) $: (selector) -> @player.$(selector) @@ -14,6 +14,7 @@ class @VideoProgressSlider change: @onChange slide: @onSlide stop: @onStop + @buildHandle() buildHandle: -> @handle = @$('.ui-slider-handle') @@ -30,10 +31,13 @@ class @VideoProgressSlider widget: true onReady: => - @slider.slider('option', 'max', @player.duration()) + @slider.slider('option', 'max', @player.duration()) if @slider + + onPlay: => + @buildSlider() unless @slider onUpdatePlayTime: (event, currentTime) => - if !@frozen + if @slider && !@frozen @slider.slider('option', 'max', @player.duration()) @slider.slider('value', currentTime) From 3b0a1b7ee2464be52657fedfe6983c1e9663fbec Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Mon, 4 Jun 2012 11:51:18 -0400 Subject: [PATCH 5/7] Fix test variable leakage --- static/coffee/spec/modules/video/video_player_spec.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/static/coffee/spec/modules/video/video_player_spec.coffee b/static/coffee/spec/modules/video/video_player_spec.coffee index 26f5e3b5ed..cdfedcb4f9 100644 --- a/static/coffee/spec/modules/video/video_player_spec.coffee +++ b/static/coffee/spec/modules/video/video_player_spec.coffee @@ -90,7 +90,7 @@ describe 'VideoPlayer', -> describe 'when not on a touch based device', -> beforeEach -> - window.onTouchBasedDevice = -> false + spyOn(window, 'onTouchBasedDevice').andReturn false spyOn @player, 'play' @player.onReady() @@ -99,7 +99,7 @@ describe 'VideoPlayer', -> describe 'when on a touch based device', -> beforeEach -> - window.onTouchBasedDevice = -> true + spyOn(window, 'onTouchBasedDevice').andReturn true spyOn @player, 'play' @player.onReady() From 6f9870f75fa56b82af8e1d622606fa4427765c11 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Mon, 4 Jun 2012 14:15:53 -0400 Subject: [PATCH 6/7] Hide the caption first when on the iOS browser This prevent user from prematurely seeking the video. --- static/coffee/spec/helper.coffee | 8 +- .../modules/video/video_caption_spec.coffee | 140 ++++++++++++------ .../src/modules/video/video_caption.coffee | 12 +- 3 files changed, 104 insertions(+), 56 deletions(-) diff --git a/static/coffee/spec/helper.coffee b/static/coffee/spec/helper.coffee index 34e03e9d32..3d9537f872 100644 --- a/static/coffee/spec/helper.coffee +++ b/static/coffee/spec/helper.coffee @@ -11,12 +11,8 @@ jasmine.stubbedMetadata = duration: 300 jasmine.stubbedCaption = - start: [0, 10000, 20000, 30000, 40000, 50000, 60000, 70000, 80000, 90000, - 100000, 110000, 120000] - text: ['Caption at 0', 'Caption at 10000', 'Caption at 20000', - 'Caption at 30000', 'Caption at 40000', 'Caption at 50000', 'Caption at 60000', - 'Caption at 70000', 'Caption at 80000', 'Caption at 90000', 'Caption at 100000', - 'Caption at 110000', 'Caption at 120000'] + start: [0, 10000, 20000, 30000] + text: ['Caption at 0', 'Caption at 10000', 'Caption at 20000', 'Caption at 30000'] jasmine.stubRequests = -> spyOn($, 'ajax').andCallFake (settings) -> diff --git a/static/coffee/spec/modules/video/video_caption_spec.coffee b/static/coffee/spec/modules/video/video_caption_spec.coffee index acf900fdcf..e816da593a 100644 --- a/static/coffee/spec/modules/video/video_caption_spec.coffee +++ b/static/coffee/spec/modules/video/video_caption_spec.coffee @@ -9,66 +9,82 @@ describe 'VideoCaption', -> describe 'constructor', -> beforeEach -> spyOn($, 'getWithPrefix').andCallThrough() - @caption = new VideoCaption @player, 'def456' - it 'set the player', -> - expect(@caption.player).toEqual @player + describe 'always', -> + beforeEach -> + @caption = new VideoCaption @player, 'def456' - it 'set the youtube id', -> - expect(@caption.youtubeId).toEqual 'def456' + it 'set the player', -> + expect(@caption.player).toEqual @player - it 'create the caption element', -> - expect($('.video')).toContain 'ol.subtitles' + it 'set the youtube id', -> + expect(@caption.youtubeId).toEqual 'def456' - it 'add caption control to video player', -> - expect($('.video')).toContain 'a.hide-subtitles' + it 'create the caption element', -> + expect($('.video')).toContain 'ol.subtitles' - it 'fetch the caption', -> - expect($.getWithPrefix).toHaveBeenCalledWith @caption.captionURL(), jasmine.any(Function) + it 'add caption control to video player', -> + expect($('.video')).toContain 'a.hide-subtitles' - it 'render the caption', -> - expect($('.subtitles').html()).toMatch new RegExp(''' -
  • Caption at 0
    • -
  • Caption at 10000
    • -
  • Caption at 20000
    • -
  • Caption at 30000
    • -
  • Caption at 40000
    • -
  • Caption at 50000
    • -
  • Caption at 60000
    • -
  • Caption at 70000
    • -
  • Caption at 80000
    • -
  • Caption at 90000
    • -
  • Caption at 100000
    • -
  • Caption at 110000
    • -
  • Caption at 120000
    • - '''.replace(/\n/g, '')) + it 'fetch the caption', -> + expect($.getWithPrefix).toHaveBeenCalledWith @caption.captionURL(), jasmine.any(Function) - it 'add a padding element to caption', -> - expect($('.subtitles li:first')).toBe '.spacing' - expect($('.subtitles li:last')).toBe '.spacing' + it 'bind window resize event', -> + expect($(window)).toHandleWith 'resize', @caption.onWindowResize - it 'bind all the caption link', -> - $('.subtitles li[data-index]').each (index, link) => - expect($(link)).toHandleWith 'click', @caption.seekPlayer + it 'bind player resize event', -> + expect($(@player)).toHandleWith 'resize', @caption.onWindowResize - it 'bind window resize event', -> - expect($(window)).toHandleWith 'resize', @caption.onWindowResize + it 'bind player updatePlayTime event', -> + expect($(@player)).toHandleWith 'updatePlayTime', @caption.onUpdatePlayTime - it 'bind player resize event', -> - expect($(@player)).toHandleWith 'resize', @caption.onWindowResize + it 'bind player play event', -> + expect($(@player)).toHandleWith 'play', @caption.onPlay - it 'bind player updatePlayTime event', -> - expect($(@player)).toHandleWith 'updatePlayTime', @caption.onUpdatePlayTime + it 'bind the hide caption button', -> + expect($('.hide-subtitles')).toHandleWith 'click', @caption.toggle - it 'bind the hide caption button', -> - expect($('.hide-subtitles')).toHandleWith 'click', @caption.toggle + it 'bind the mouse movement', -> + expect($('.subtitles')).toHandleWith 'mouseenter', @caption.onMouseEnter + expect($('.subtitles')).toHandleWith 'mouseleave', @caption.onMouseLeave + expect($('.subtitles')).toHandleWith 'mousemove', @caption.onMovement + expect($('.subtitles')).toHandleWith 'mousewheel', @caption.onMovement + expect($('.subtitles')).toHandleWith 'DOMMouseScroll', @caption.onMovement - it 'bind the mouse movement', -> - expect($('.subtitles')).toHandleWith 'mouseenter', @caption.onMouseEnter - expect($('.subtitles')).toHandleWith 'mouseleave', @caption.onMouseLeave - expect($('.subtitles')).toHandleWith 'mousemove', @caption.onMovement - expect($('.subtitles')).toHandleWith 'mousewheel', @caption.onMovement - expect($('.subtitles')).toHandleWith 'DOMMouseScroll', @caption.onMovement + describe 'when on a non touch-based device', -> + beforeEach -> + spyOn(window, 'onTouchBasedDevice').andReturn false + @caption = new VideoCaption @player, 'def456' + + it 'render the caption', -> + expect($('.subtitles').html()).toMatch new RegExp(''' +
  • Caption at 0
    • +
  • Caption at 10000
    • +
  • Caption at 20000
    • +
  • Caption at 30000
    • + '''.replace(/\n/g, '')) + + it 'add a padding element to caption', -> + expect($('.subtitles li:first')).toBe '.spacing' + expect($('.subtitles li:last')).toBe '.spacing' + + it 'bind all the caption link', -> + $('.subtitles li[data-index]').each (index, link) => + expect($(link)).toHandleWith 'click', @caption.seekPlayer + + it 'set rendered to true', -> + expect(@caption.rendered).toBeTruthy() + + describe 'when on a touch-based device', -> + beforeEach -> + spyOn(window, 'onTouchBasedDevice').andReturn true + @caption = new VideoCaption @player, 'def456' + + it 'show explaination message', -> + expect($('.subtitles li')).toHaveHtml "Caption will be displayed when you start playing the video." + + it 'does not set rendered to true', -> + expect(@caption.rendered).toBeFalsy() describe 'mouse movement', -> beforeEach -> @@ -145,8 +161,34 @@ describe 'VideoCaption', -> expect(@caption.search(9999)).toEqual 0 expect(@caption.search(10000)).toEqual 1 expect(@caption.search(15000)).toEqual 1 - expect(@caption.search(120000)).toEqual 12 - expect(@caption.search(120001)).toEqual 12 + expect(@caption.search(30000)).toEqual 3 + expect(@caption.search(30001)).toEqual 3 + + describe 'onPlay', -> + describe 'when the caption was not rendered', -> + beforeEach -> + spyOn(window, 'onTouchBasedDevice').andReturn true + @caption = new VideoCaption @player, 'def456' + @caption.onPlay() + + it 'render the caption', -> + expect($('.subtitles').html()).toMatch new RegExp(''' +
  • Caption at 0
    • +
  • Caption at 10000
    • +
  • Caption at 20000
    • +
  • Caption at 30000
    • + '''.replace(/\n/g, '')) + + it 'add a padding element to caption', -> + expect($('.subtitles li:first')).toBe '.spacing' + expect($('.subtitles li:last')).toBe '.spacing' + + it 'bind all the caption link', -> + $('.subtitles li[data-index]').each (index, link) => + expect($(link)).toHandleWith 'click', @caption.seekPlayer + + it 'set rendered to true', -> + expect(@caption.rendered).toBeTruthy() describe 'onUpdatePlayTime', -> beforeEach -> diff --git a/static/coffee/src/modules/video/video_caption.coffee b/static/coffee/src/modules/video/video_caption.coffee index 31a3d09e09..7d796245bb 100644 --- a/static/coffee/src/modules/video/video_caption.coffee +++ b/static/coffee/src/modules/video/video_caption.coffee @@ -10,6 +10,7 @@ class @VideoCaption $(window).bind('resize', @onWindowResize) $(@player).bind('resize', @onWindowResize) $(@player).bind('updatePlayTime', @onUpdatePlayTime) + $(@player).bind('play', @onPlay) @$('.hide-subtitles').click @toggle @$('.subtitles').mouseenter(@onMouseEnter).mouseleave(@onMouseLeave) .mousemove(@onMovement).bind('mousewheel', @onMovement) @@ -32,7 +33,11 @@ class @VideoCaption $.getWithPrefix @captionURL(), (captions) => @captions = captions.text @start = captions.start - @renderCaption() + + if onTouchBasedDevice() + $('.subtitles li').html "Caption will be displayed when you start playing the video." + else + @renderCaption() renderCaption: -> container = $('
      ') @@ -49,6 +54,8 @@ class @VideoCaption @$('.subtitles').prepend($('
    1. ').height(@topSpacingHeight())) .append($('
    2. ').height(@bottomSpacingHeight())) + @rendered = true + search: (time) -> min = 0 max = @start.length - 1 @@ -62,6 +69,9 @@ class @VideoCaption return min + onPlay: => + @renderCaption() unless @rendered + onUpdatePlayTime: (event, time) => # This 250ms offset is required to match the video speed time = Math.round(Time.convert(time, @player.currentSpeed(), '1.0') * 1000 + 250) From c380bd82aa1d6637e438367daa2e82d4e8ede394 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Mon, 4 Jun 2012 15:25:49 -0400 Subject: [PATCH 7/7] Fix subtitle scroll and hide subtitle transition --- static/sass/courseware/_video.scss | 3 --- 1 file changed, 3 deletions(-) diff --git a/static/sass/courseware/_video.scss b/static/sass/courseware/_video.scss index 54c3fd8e95..7130cc8b7b 100644 --- a/static/sass/courseware/_video.scss +++ b/static/sass/courseware/_video.scss @@ -368,7 +368,6 @@ section.course-content { cursor: pointer; margin-bottom: 8px; padding: 0; - @include transition(all, .5s, ease-in); &.current { color: #333; @@ -393,7 +392,6 @@ section.course-content { } ol.subtitles { - height: 0; width: 0px; } } @@ -415,7 +413,6 @@ section.course-content { &.closed { ol.subtitles { - height: auto; right: -(flex-grid(4)); width: auto; }