Refactor anchoring interface and sorting

Make setupAnnotation into a tiny compatibility wrapper around a new
method #anchor() method that returns a promise of the anchors. This
is cleaner than trying to store the anchors on the annotation itself,
which creates circular references, and it means there's no need to
strip these properties when serializes annotations across the frame
boundary.

Sort annotations by TextPositionSelector in the widget display. This
provides greater visual stability -- cards don't change positions
after anchoring -- at the cost of potentially out of order cards when
anchors have moved.

h/static/scripts/annotator/guest.coffee

@@ -161,7 +161,7 @@ module.exports = class Guest extends Annotator
 
     this.removeEvents()
 
-  setupAnnotation: (annotation) ->
+  anchor: (annotation) ->
     self = this
     root = @element[0]
 
@@ -200,22 +200,13 @@ module.exports = class Guest extends Annotator
       return animationPromise ->
         range = Annotator.Range.sniff(anchor.range)
         normedRange = range.normalize(root)
-
         highlights = highlighter.highlightRange(normedRange)
-        rect = highlighter.getBoundingClientRect(highlights)
-
         $(highlights).data('annotation', anchor.annotation)
-
         anchor.highlights = highlights
-        anchor.pos =
-          left: rect.left + window.scrollX
-          top: rect.top + window.scrollY
-
         return anchor
 
     sync = (anchors) ->
       # Store the results of anchoring.
-      annotation.$anchors = ({pos} for {pos} in anchors)
       annotation.$orphan = anchors.length > 0
       for anchor in anchors
         if anchor.range?
@@ -228,6 +219,8 @@ module.exports = class Guest extends Annotator
       self.plugins.BucketBar?.update()
       self.plugins.CrossFrame?.sync([annotation])
 
+      return anchors
+
     # Remove all the anchors for this annotation from the instance storage.
     for anchor in self.anchors.splice(0, self.anchors.length)
       if anchor.annotation is annotation
@@ -253,9 +246,12 @@ module.exports = class Guest extends Annotator
       anchor = locate(target).then(highlight)
       anchors.push(anchor)
 
-    # Wait for all the anchoring tasks to complete then call sync.
-    Promise.all(anchors).then(sync)
+    return Promise.all(anchors).then(sync)
 
+  # Provided for backward compatibility.
+  # Currently only used by #loadAnnotations().
+  setupAnnotation: (annotation) ->
+    this.anchor(annotation)
     return annotation
 
   createAnnotation: (annotation = {}) ->
@@ -286,8 +282,8 @@ module.exports = class Guest extends Annotator
     metadata = info.then(setDocumentInfo)
     targets = Promise.all([info, selectors]).then(setTargets)
 
-    targets.then(-> self.setupAnnotation(annotation))
     targets.then(-> self.publish('beforeAnnotationCreated', [annotation]))
+    targets.then(-> self.anchor(annotation))
 
     annotation
 

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

@@ -104,7 +104,7 @@ class PDF extends Annotator.Plugin
           break
 
     for annotation in refreshAnnotations
-      annotator.setupAnnotation(annotation)
+      annotator.anchor(annotation)
 
 Annotator.Plugin.PDF = PDF
 

h/static/scripts/annotator/test/guest-test.coffee

@@ -17,13 +17,6 @@ Guest = proxyquire('../guest', {
 
 $ = require('jquery')
 
-waitForSync = (annotation) ->
-  if annotation.$anchors?
-    return Promise.resolve()
-  else
-    return new Promise(setTimeout).then(-> waitForSync(annotation))
-
-
 describe 'Guest', ->
   sandbox = null
   fakeCrossFrame = null
@@ -294,7 +287,7 @@ describe 'Guest', ->
       annotation = guest.createAnnotation(annotation)
       assert.equal(annotation.foo, 'bar')
 
-  describe 'setupAnnotation()', ->
+  describe 'anchor()', ->
     el = null
     range = null
 
@@ -312,8 +305,7 @@ describe 'Guest', ->
     it "doesn't declare annotation without targets as orphans", (done) ->
       guest = createGuest()
       annotation = target: []
-      guest.setupAnnotation(annotation)
-      waitForSync(annotation).then ->
+      guest.anchor(annotation).then ->
         assert.isFalse(annotation.$orphan)
         done()
 
@@ -321,8 +313,7 @@ describe 'Guest', ->
       guest = createGuest()
       annotation = target: [{selector: "test"}]
       sandbox.stub(anchoring, 'anchor').returns(Promise.resolve(range))
-      guest.setupAnnotation(annotation)
-      waitForSync(annotation).then ->
+      guest.anchor(annotation).then ->
         assert.isFalse(annotation.$orphan)
         done()
 
@@ -330,8 +321,7 @@ describe 'Guest', ->
       guest = createGuest()
       annotation = target: [{selector: 'broken selector'}]
       sandbox.stub(anchoring, 'anchor').returns(Promise.reject())
-      guest.setupAnnotation(annotation)
-      waitForSync(annotation).then ->
+      guest.anchor(annotation).then ->
         assert.isTrue(annotation.$orphan)
         done()
 
@@ -342,25 +332,21 @@ describe 'Guest', ->
       guest.plugins.BucketBar =
         update: sinon.stub()
       annotation = {}
-      guest.setupAnnotation(annotation)
-      waitForSync(annotation).then ->
+      guest.anchor(annotation).then ->
         assert.called(guest.plugins.BucketBar.update)
         assert.called(guest.plugins.CrossFrame.sync)
         done()
 
-    it 'saves the anchor positions on the annotation', (done) ->
+    it 'returns a promise of the anchors for the annotation', (done) ->
       guest = createGuest()
+      highlights = [document.createElement('span')]
       sandbox.stub(anchoring, 'anchor').returns(Promise.resolve(range))
-      clientRect = {top: 100, left: 200}
-      window.scrollX = 50
-      window.scrollY = 25
-      sandbox.stub(highlighter, 'getBoundingClientRect').returns(clientRect)
-      annotation = guest.setupAnnotation({target: [{selector: []}]})
-      waitForSync(annotation).then ->
-        assert.equal(annotation.$anchors.length, 1)
-        pos = annotation.$anchors[0].pos
-        assert.equal(pos.top, 125)
-        assert.equal(pos.left, 250)
+      sandbox.stub(highlighter, 'highlightRange').returns(highlights)
+      target = [{selector: []}]
+      promise = guest.anchor({target: [target]})
+      assert.instanceOf(promise, Promise)
+      promise.then (anchors) ->
+        assert.equal(anchors.length, 1)
         done()
 
     it 'adds the anchor to the "anchors" instance property"', (done) ->
@@ -369,8 +355,8 @@ describe 'Guest', ->
       sandbox.stub(anchoring, 'anchor').returns(Promise.resolve(range))
       sandbox.stub(highlighter, 'highlightRange').returns(highlights)
       target = [{selector: []}]
-      annotation = guest.setupAnnotation({target: [target]})
-      waitForSync(annotation).then ->
+      annotation = {target: [target]}
+      guest.anchor(annotation).then ->
         assert.equal(guest.anchors.length, 1)
         assert.strictEqual(guest.anchors[0].annotation, annotation)
         assert.strictEqual(guest.anchors[0].target, target)
@@ -385,8 +371,7 @@ describe 'Guest', ->
       guest = createGuest()
       guest.anchors = [{annotation, target, highlights}]
       removeHighlights = sandbox.stub(highlighter, 'removeHighlights')
-      guest.setupAnnotation(annotation)
-      waitForSync(annotation).then ->
+      guest.anchor(annotation).then ->
         assert.equal(guest.anchors.length, 0)
         assert.calledWith(removeHighlights, highlights)
         done()
@@ -395,11 +380,8 @@ describe 'Guest', ->
       guest = createGuest()
       annotation = target: [{selector: "test"}]
       stub = sandbox.stub(anchoring, 'anchor').returns(Promise.resolve(range))
-      guest.setupAnnotation(annotation)
-      waitForSync(annotation).then ->
-        delete annotation.$anchors
-        guest.setupAnnotation(annotation)
-        waitForSync(annotation).then ->
+      guest.anchor(annotation).then ->
+        guest.anchor(annotation).then ->
           assert.equal(guest.anchors.length, 1)
           assert.calledOnce(stub)
           done()

h/static/scripts/app-controller.coffee

@@ -82,11 +82,13 @@ module.exports = class AppController
       predicate = switch name
         when 'Newest' then ['-!!message', '-message.updated']
         when 'Oldest' then ['-!!message',  'message.updated']
-        when 'Location' then [
-          '-!!message'
-          'message.$anchors[0].pos.top'
-          'message.$anchors[0].pos.left'
-        ]
+        when 'Location' then (thread) ->
+          if thread.message?
+            for target in thread.message.target ? []
+              for selector in target.selector ? []
+                if selector.type is 'TextPositionSelector'
+                  return selector.start
+          return Number.POSITIVE_INFINITY
       $scope.sort = {name, predicate}
 
     $scope.$watch 'auth.user', (newVal, oldVal) ->

h/static/scripts/cross-frame.coffee

@@ -18,10 +18,7 @@ module.exports = class CrossFrame
       new Discovery($window, options)
 
     createAnnotationSync = ->
-      whitelist = [
-        '$anchors', '$highlight', '$orphan',
-        'target', 'document', 'uri'
-      ]
+      whitelist = ['$highlight', '$orphan','target', 'document', 'uri']
       options =
         formatter: (annotation) ->
           formatted = {}