Merge pull request #1908 from hypothesis/1890-fix-scrolling-nopadding

Refactor and fix scrolling - except padding

h/static/scripts/annotator/plugin/bucket-bar.coffee

@@ -53,8 +53,7 @@ class Annotator.Plugin.BucketBar extends Annotator.Plugin
     $(window).on 'resize scroll', this._scheduleUpdate
     $(document.body).on 'resize scroll', '*', this._scheduleUpdate
 
-    # Event handler to finish scrolling when we have to
-    # wait for anchors to be realized
+    # Event handler to to update when new highlights have been created
     @annotator.subscribe "highlightsCreated", (highlights) =>
       # All the highlights are guaranteed to belong to one anchor,
       # so we can do this:
@@ -68,26 +67,7 @@ class Annotator.Plugin.BucketBar extends Annotator.Plugin
       if anchor.annotation.id? # Is this a finished annotation ?
         this._scheduleUpdate()
 
-      if @pendingScroll? and anchor in @pendingScroll.anchors
-        # One of the wanted anchors has been realized
-        unless --@pendingScroll.count
-          # All anchors have been realized
-          page = @pendingScroll.page
-          dir = if @pendingScroll.direction is "up" then +1 else -1
-          {next} = @pendingScroll.anchors.reduce (acc, anchor) ->
-            {start, next} = acc
-            hl = anchor.highlight[page]
-            if not next? or hl.getTop()*dir > start*dir
-              start: hl.getTop()
-              next: hl
-            else
-              acc
-          , {}
-
-          # Scroll down to the annotation from the top of the annotation's page
-          next.paddedScrollTo('down')
-          delete @pendingScroll
-
+    # Event handler to to update when highlights have been removed
     @annotator.subscribe "highlightRemoved", (highlight) =>
       if highlight.annotation.id? # Is this a finished annotation ?
         this._scheduleUpdate()
@@ -162,21 +142,7 @@ class Annotator.Plugin.BucketBar extends Annotator.Plugin
 
     # Get an anchor from the page we want to go to
     anchor = next[0]
-    startPage = anchor.startPage
-
-    # Is this rendered?
-    if @annotator.anchoring.document.isPageMapped startPage
-      # If it was rendered, then we only have one result. Go there.
-      hl = anchor.highlight[startPage]
-      hl.paddedScrollTo direction
-    else
-      # Not rendered yet. Go to the page, and see what happens.
-      @pendingScroll =
-        anchors: next
-        count: next.length
-        page: startPage
-        direction: direction
-      @annotator.anchoring.document.setPageIndex startPage
+    anchor.scrollIntoView()
 
   _update: =>
     wrapper = @annotator.wrapper

h/static/scripts/annotator/plugin/enhancedanchoring.coffee

@@ -57,6 +57,14 @@ class Anchor
     # Announce the creation of the highlights
     @anchoring.annotator.publish 'highlightsCreated', created
 
+    # If we are supposed to scroll to the highlight on a page,
+    # and it's available now, go scroll there.
+    if @pendingScrollTargetPage? and (hl = @highlight[@pendingScrollTargetPage])
+      hl.scrollIntoView().then =>
+        @pendingScrollResolve()
+        delete @pendingScrollTargetPage
+        delete @pendingScrollResolve
+
   # Remove the highlights for the given set of pages
   virtualize: (pageIndex) =>
     highlight = @highlight[pageIndex]
@@ -85,6 +93,39 @@ class Anchor
       # Kill the list if it's empty
       delete @anchoring.anchors[index] unless anchors.length
 
+  # Scroll to this anchor
+  scrollIntoView: ->
+    currentPage = @anchoring.document.getPageIndex()
+
+    if @startPage is @endPage and currentPage is @startPage
+      # It's all in one page. Simply scrolling
+      @highlight[@startPage].scrollIntoView()
+    else
+      if currentPage < @startPage
+        # We need to go forward
+        wantedPage = @startPage
+        scrollPage = wantedPage - 1
+      else if currentPage > @endPage
+        # We need to go backwards
+        wantedPage = @endPage
+        scrollPage = wantedPage + 1
+      else
+        # We have no idea where we need to go.
+        # Let's just go to the start.
+        wantedPage = @startPage
+        scrollPage = wantedPage
+
+      # Is this rendered?
+      if @anchoring.document.isPageMapped wantedPage
+        # The wanted page is already rendered, we can simply go there
+        @highlight[wantedPage].scrollIntoView()
+      else
+        # Not rendered yet. Go to the page, we will continue from there
+        @pendingScrollTargetPage = wantedPage
+        new Promise (resolve, reject) =>
+          @pendingScrollResolve = resolve
+          @anchoring.document.setPageIndex scrollPage
+
 Annotator.Anchor = Anchor
 
 # This plugin contains the enhanced anchoring framework.

h/static/scripts/annotator/plugin/texthighlights.coffee

@@ -85,27 +85,11 @@ class TextHighlight
   # Get the height of the highlight.
   getHeight: -> $(@_highlights).outerHeight true
 
-  # Scroll the highlight into view.
-  scrollTo: -> $(@_highlights).scrollintoview()
-
-  # Scroll the highlight into view, with a comfortable margin.
-  # up should be true if we need to scroll up; false otherwise
-  paddedScrollTo: (direction) ->
-    unless direction? then throw "Direction is required"
-    dir = if direction is "up" then -1 else +1
-    wrapper = @annotator.wrapper
-    defaultView = wrapper[0].ownerDocument.defaultView
-    pad = defaultView.innerHeight * .2
-    $(@_highlights).scrollintoview
-      complete: ->
-        scrollable = if this.parentNode is this.ownerDocument
-          $(this.ownerDocument.body)
-        else
-          $(this)
-        top = scrollable.scrollTop()
-        correction = pad * dir
-        scrollable.stop().animate {scrollTop: top + correction}, 300
-
+  # Scroll the highlight into view
+  scrollIntoView: ->
+    new Promise (resolve, reject) ->
+      $(@_highlights).scrollintoview complete: ->
+        resolve()
 
 # Public: Wraps the DOM Nodes within the provided range with a highlight
 # element of the specified class and returns the highlight Elements.

h/static/scripts/guest.coffee

@@ -164,9 +164,9 @@ class Annotator.Guest extends Annotator
         else
           hl.setFocused false
     crossframe.on 'scrollToAnnotation', (ctx, tag) =>
-      for hl in @anchoring.getHighlights()
-        if hl.annotation.$$tag is tag
-          hl.scrollTo()
+      for a in @anchoring.getAnchors()
+        if a.annotation.$$tag is tag
+          a.scrollIntoView()
           return
     crossframe.on 'getDocumentInfo', (trans) =>
       (@plugins.PDF?.getMetaData() ? Promise.reject())

tests/js/guest-test.coffee

@@ -194,14 +194,14 @@ describe 'Annotator.Guest', ->
         assert.calledWith(highlights[1].setFocused, false)
 
     describe 'on "scrollToAnnotation" event', ->
-      it 'scrolls to the highLight with the matching tag', ->
+      it 'scrolls to the anchor with the matching tag', ->
         guest = createGuest()
-        highlights = [
-          {annotation: {$$tag: 'tag1'}, scrollTo: sandbox.stub()}
+        anchors = [
+          {annotation: {$$tag: 'tag1'}, scrollIntoView: sandbox.stub()}
         ]
-        sandbox.stub(guest.anchoring, 'getHighlights').returns(highlights)
+        sandbox.stub(guest.anchoring, 'getAnchors').returns(anchors)
         emitGuestEvent('scrollToAnnotation', 'ctx', 'tag1')
-        assert.called(highlights[0].scrollTo)
+        assert.called(anchors[0].scrollIntoView)
 
     describe 'on "getDocumentInfo" event', ->
       guest = null

tests/js/plugin/enhancedanchoring-test.coffee

@@ -37,6 +37,8 @@ describe 'Annotator.Plugin.EnhancedAnchoring', ->
 
     am.chooseAccessPolicy()
 
+    am.document.setPageIndex = sinon.spy()
+
     am.strategies.push
       name: "dummy anchoring strategy"
       code: (annotation, target) ->
@@ -57,6 +59,8 @@ describe 'Annotator.Plugin.EnhancedAnchoring', ->
         anchor: anchor
         page: page
         removeFromDocument: sinon.spy()
+        scrollIntoView: sinon.spy ->
+          new Promise (resolve, reject) -> setTimeout -> resolve()
 
   afterEach ->
     sandbox.restore()
@@ -219,6 +223,15 @@ describe 'Annotator.Plugin.EnhancedAnchoring', ->
       assert.include hls, anchor2.highlight[anchor2.startPage]
       assert.notInclude hls, anchor3.highlight[anchor3.startPage]
 
+  describe 'Anchor.scrollIntoView()', ->
+    it 'calls scrollIntoView() on the highlight', ->
+      am = createAnchoringManager()
+      ann = createTestAnnotation "a1"
+      anchor = am.createAnchor(ann, ann.target[0]).result
+      anchor.scrollIntoView().then ->
+
+        assert.called anchor.highlight[anchor.startPage].scrollIntoView
+
   describe 'two-phased anchoring', ->
 
     class DummyDocumentAccess
@@ -226,6 +239,7 @@ describe 'Annotator.Plugin.EnhancedAnchoring', ->
       @applicable: -> true
 
       isPageMapped: (index) -> index in @_rendered
+      getPageIndex: -> @currentIndex
 
       constructor: ->
         @_rendered = []
@@ -634,3 +648,45 @@ describe 'Annotator.Plugin.EnhancedAnchoring', ->
             anchor.realize()
 
             assert anchor.fullyRealized
+
+    describe 'when scrolling to a virtual anchor', ->
+
+      it 'scrolls right next to the wanted page (to get it rendered)', ->
+        am = createAnchoringManagerAndLazyDocument()
+        ann = createTestAnnotationForPages "a1", [10]
+        anchor = am.createAnchor(ann, ann.target[0]).result
+        am.document.currentIndex = 5  # We start from page 5
+        am.document.setPageIndex = sinon.spy()
+
+        # Now we trigger the actual action
+        anchor.scrollIntoView()
+        assert.calledWith am.document.setPageIndex, 9
+
+      it 'gets the wanted page rendered', ->
+        am = createAnchoringManagerAndLazyDocument()
+        ann = createTestAnnotationForPages "a1", [10]
+        anchor = am.createAnchor(ann, ann.target[0]).result
+        am.document.currentIndex = 5  # We start from page 5
+        am.document.setPageIndex = sinon.spy (index) ->
+          am.document.currentIndex = index
+          if index is 9
+            renderPage am.document, 9
+            renderPage am.document, 10
+
+        # Now we trigger the actual action
+        anchor.scrollIntoView().then ->
+          assert am.document.isPageMapped 10
+
+      it 'calls scrollIntoView() on the highlight', ->
+        am = createAnchoringManagerAndLazyDocument()
+        ann = createTestAnnotationForPages "a1", [10]
+        anchor = am.createAnchor(ann, ann.target[0]).result
+        am.document.currentIndex = 5  # We start from page 5
+        am.document.setPageIndex = sinon.spy (index) ->
+          am.document.currentIndex = index
+          if index is 9
+            renderPage am.document, 9
+            renderPage am.document, 10
+
+        anchor.scrollIntoView().then ->
+          assert.called anchor.highlight[10].scrollIntoView