Merge pull request #82 from MITx/ps-fix-player

Fix Youtube player glitches on iOS

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) ->

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
-
-    it 'set the youtube id', ->
-      expect(@caption.youtubeId).toEqual 'def456'
-
-    it 'create the caption element', ->
-      expect($('.video')).toContain 'ol.subtitles'
-
-    it 'add caption control to video player', ->
-      expect($('.video')).toContain 'a.hide-subtitles'
-
-    it 'fetch the caption', ->
-      expect($.getWithPrefix).toHaveBeenCalledWith @caption.captionURL(), jasmine.any(Function)
-
-    it 'render the caption', ->
-      expect($('.subtitles').html()).toMatch new RegExp('''
-        <li data-index="0" data-start="0">Caption at 0</li>
-        <li data-index="1" data-start="10000">Caption at 10000</li>
-        <li data-index="2" data-start="20000">Caption at 20000</li>
-        <li data-index="3" data-start="30000">Caption at 30000</li>
-        <li data-index="4" data-start="40000">Caption at 40000</li>
-        <li data-index="5" data-start="50000">Caption at 50000</li>
-        <li data-index="6" data-start="60000">Caption at 60000</li>
-        <li data-index="7" data-start="70000">Caption at 70000</li>
-        <li data-index="8" data-start="80000">Caption at 80000</li>
-        <li data-index="9" data-start="90000">Caption at 90000</li>
-        <li data-index="10" data-start="100000">Caption at 100000</li>
-        <li data-index="11" data-start="110000">Caption at 110000</li>
-        <li data-index="12" data-start="120000">Caption at 120000</li>
-      '''.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 'bind window resize event', ->
-      expect($(window)).toHandleWith 'resize', @caption.onWindowResize
-
-    it 'bind player resize event', ->
-      expect($(@player)).toHandleWith 'resize', @caption.onWindowResize
-
-    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 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 'always', ->
+      beforeEach ->
+        @caption = new VideoCaption @player, 'def456'
+
+      it 'set the player', ->
+        expect(@caption.player).toEqual @player
+
+      it 'set the youtube id', ->
+        expect(@caption.youtubeId).toEqual 'def456'
+
+      it 'create the caption element', ->
+        expect($('.video')).toContain 'ol.subtitles'
+
+      it 'add caption control to video player', ->
+        expect($('.video')).toContain 'a.hide-subtitles'
+
+      it 'fetch the caption', ->
+        expect($.getWithPrefix).toHaveBeenCalledWith @caption.captionURL(), jasmine.any(Function)
+
+      it 'bind window resize event', ->
+        expect($(window)).toHandleWith 'resize', @caption.onWindowResize
+
+      it 'bind player resize event', ->
+        expect($(@player)).toHandleWith 'resize', @caption.onWindowResize
+
+      it 'bind player updatePlayTime event', ->
+        expect($(@player)).toHandleWith 'updatePlayTime', @caption.onUpdatePlayTime
+
+      it 'bind player play event', ->
+        expect($(@player)).toHandleWith 'play', @caption.onPlay
+
+      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
+
+    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('''
+          <li data-index="0" data-start="0">Caption at 0</li>
+          <li data-index="1" data-start="10000">Caption at 10000</li>
+          <li data-index="2" data-start="20000">Caption at 20000</li>
+          <li data-index="3" data-start="30000">Caption at 30000</li>
+        '''.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('''
+          <li data-index="0" data-start="0">Caption at 0</li>
+          <li data-index="1" data-start="10000">Caption at 10000</li>
+          <li data-index="2" data-start="20000">Caption at 20000</li>
+          <li data-index="3" data-start="30000">Caption at 30000</li>
+        '''.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 ->

static/coffee/spec/modules/video/video_control_spec.coffee

 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 '''
         <div class="slider"></div>
         <div>
           <ul class="vcr">
-            <li><a class="video_control play">Play</a></li>
+            <li><a class="video_control play" href="#">Play</a></li>
             <li>
               <div class="vidtime">0:00 / 0:00</div>
             </li>
@@ -23,12 +22,33 @@ describe 'VideoControl', ->
       '''
 
     it 'bind player events', ->
-      expect($(@player)).toHandleWith 'play', @control.onPlay
-      expect($(@player)).toHandleWith 'pause', @control.onPause
-      expect($(@player)).toHandleWith 'ended', @control.onPause
+      control = new VideoControl @player
+      expect($(@player)).toHandleWith 'play', control.onPlay
+      expect($(@player)).toHandleWith 'pause', control.onPause
+      expect($(@player)).toHandleWith 'ended', control.onPause
 
     it 'bind the playback button', ->
-      expect($('.video_control')).toHandleWith 'click', @control.togglePlayback
+      control = new VideoControl @player
+      expect($('.video_control')).toHandleWith 'click', control.togglePlayback
+
+    describe 'when on a touch based device', ->
+      beforeEach ->
+        spyOn(window, 'onTouchBasedDevice').andReturn true
+
+      it 'does not add the play class to video control', ->
+        new VideoControl @player
+        expect($('.video_control')).not.toHaveClass 'play'
+        expect($('.video_control')).not.toHaveHtml 'Play'
+
+
+    describe 'when on a non-touch based device', ->
+      beforeEach ->
+        spyOn(window, 'onTouchBasedDevice').andReturn false
+
+      it 'add the play class to video control', ->
+        new VideoControl @player
+        expect($('.video_control')).toHaveClass 'play'
+        expect($('.video_control')).toHaveHtml 'Play'
 
   describe 'onPlay', ->
     beforeEach ->
@@ -54,20 +74,47 @@ describe 'VideoControl', ->
     beforeEach ->
       @control = new VideoControl @player
 
-    describe 'when the video is playing', ->
+    describe 'when the control does not have play or pause class', ->
       beforeEach ->
-        spyOn(@player, 'isPlaying').andReturn true
-        spyOnEvent @player, 'pause'
-        @control.togglePlayback jQuery.Event('click')
+        $('.video_control').removeClass('play').removeClass('pause')
 
-      it 'trigger the pause event', ->
-        expect('pause').toHaveBeenTriggeredOn @player
+      describe 'when the video is playing', ->
+        beforeEach ->
+          spyOn(@player, 'isPlaying').andReturn true
+          spyOnEvent @player, 'pause'
+          @control.togglePlayback jQuery.Event('click')
 
-    describe 'when the video is paused', ->
-      beforeEach ->
-        spyOn(@player, 'isPlaying').andReturn false
-        spyOnEvent @player, 'play'
-        @control.togglePlayback jQuery.Event('click')
+        it 'does not trigger the pause event', ->
+          expect('pause').not.toHaveBeenTriggeredOn @player
+
+      describe 'when the video is paused', ->
+        beforeEach ->
+          spyOn(@player, 'isPlaying').andReturn false
+          spyOnEvent @player, 'play'
+          @control.togglePlayback jQuery.Event('click')
+
+        it 'does not trigger the play event', ->
+          expect('play').not.toHaveBeenTriggeredOn @player
+
+    for className in ['play', 'pause']
+      describe "when the control has #{className} class", ->
+        beforeEach ->
+          $('.video_control').addClass className
+
+        describe 'when the video is playing', ->
+          beforeEach ->
+            spyOn(@player, 'isPlaying').andReturn true
+            spyOnEvent @player, 'pause'
+            @control.togglePlayback jQuery.Event('click')
+
+          it 'trigger the pause event', ->
+            expect('pause').toHaveBeenTriggeredOn @player
+
+        describe 'when the video is paused', ->
+          beforeEach ->
+            spyOn(@player, 'isPlaying').andReturn false
+            spyOnEvent @player, 'play'
+            @control.togglePlayback jQuery.Event('click')
 
-      it 'trigger the play event', ->
-        expect('play').toHaveBeenTriggeredOn @player
+          it 'trigger the play event', ->
+            expect('play').toHaveBeenTriggeredOn @player

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()
 

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 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
+        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 '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 'bind player events', ->
-      expect($(@player)).toHandleWith 'updatePlayTime', @slider.onUpdatePlayTime
+      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

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 = $('<ol>')
@@ -49,6 +54,8 @@ class @VideoCaption
     @$('.subtitles').prepend($('<li class="spacing">').height(@topSpacingHeight()))
       .append($('<li class="spacing">').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)

static/coffee/src/modules/video/video_control.coffee

@@ -17,7 +17,7 @@ class @VideoControl
       <div class="slider"></div>
       <div>
         <ul class="vcr">
-          <li><a class="video_control play">Play</a></li>
+          <li><a class="video_control" href="#"></a></li>
           <li>
             <div class="vidtime">0:00 / 0:00</div>
           </li>
@@ -26,7 +26,10 @@ class @VideoControl
           <a href="#" class="add-fullscreen" title="Fill browser">Fill Browser</a>
         </div>
       </div>
-     """
+      """
+
+    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')

static/coffee/src/modules/video/video_progress_slider.coffee

 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)
 

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,11 +147,17 @@ 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: url('../images/play-icon.png') center center no-repeat;
+                background-position: 17px -114px;
 
                 &:hover {
                   background-color: #444;
@@ -158,7 +165,7 @@ section.course-content {
               }
 
               &.pause {
-                background: url('../images/pause-icon.png') center center no-repeat;
+                background-position: 16px -50px;
 
                 &:hover {
                   background-color: #444;
@@ -361,7 +368,6 @@ section.course-content {
         cursor: pointer;
         margin-bottom: 8px;
         padding: 0;
-        @include transition(all, .5s, ease-in);
 
         &.current {
           color: #333;
@@ -386,7 +392,6 @@ section.course-content {
       }
 
       ol.subtitles {
-        height: 0;
         width: 0px;
       }
     }
@@ -408,7 +413,6 @@ section.course-content {
 
       &.closed {
         ol.subtitles {
-          height: auto;
           right: -(flex-grid(4));
           width: auto;
         }