Merge pull request #2549 from hypothesis/2484-fix-page-notes

Fix comments (AKA "page notes") with search_normalized

h/static/scripts/annotator/guest.coffee

@@ -197,11 +197,19 @@ module.exports = class Guest extends Annotator
 
     sync = (anchors) ->
       # Store the results of anchoring.
-      annotation.$orphan = anchors.length > 0
+
+      # An annotation is considered to be an orphan if it has at least one
+      # target with selectors, and all targets with selectors failed to anchor
+      # (i.e. we didn't find it in the page and thus it has no range).
+      hasAnchorableTargets = false
+      hasAnchoredTargets = false
       for anchor in anchors
+        if anchor.target.selector?
+          hasAnchorableTargets = true
           if anchor.range?
-          annotation.$orphan = false
+            hasAnchoredTargets = true
             break
+      annotation.$orphan = hasAnchorableTargets and not hasAnchoredTargets
 
       # Add the anchors for this annotation to instance storage.
       self.anchors = self.anchors.concat(anchors)
@@ -269,6 +277,7 @@ module.exports = class Guest extends Annotator
         cache: self.anchoringCache
         ignoreSelector: '[class^="annotator-"]'
       }
+      # Returns an array of selectors for the passed range.
       return self.anchoring.describe(root, range, options)
 
     setDocumentInfo = (info) ->
@@ -276,6 +285,8 @@ module.exports = class Guest extends Annotator
       annotation.uri = info.uri
 
     setTargets = ([info, selectors]) ->
+      # `selectors` is an array of arrays: each item is an array of selectors
+      # identifying a distinct target.
       source = info.uri
       annotation.target = ({source, selector} for selector in selectors)
 
@@ -293,6 +304,22 @@ module.exports = class Guest extends Annotator
   createHighlight: ->
     return this.createAnnotation({$highlight: true})
 
+  # Create a blank comment (AKA "page note")
+  createComment: () ->
+    annotation = {}
+    self = this
+
+    prepare = (info) ->
+      annotation.document = info.metadata
+      annotation.uri = info.uri
+      annotation.target = [{source: info.uri}]
+
+    this.getDocumentInfo()
+      .then(prepare)
+      .then(-> self.publish('beforeAnnotationCreated', [annotation]))
+
+    annotation
+
   showAnnotations: (annotations) ->
     tags = (a.$$tag for a in annotations)
     @crossframe?.call('showAnnotations', tags)

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

@@ -57,7 +57,7 @@ module.exports = class Toolbar extends Annotator.Plugin
         "click": (event) =>
           event.preventDefault()
           event.stopPropagation()
-          @annotator.createAnnotation()
+          @annotator.createComment()
           @annotator.show()
     ]
     @buttons = $(makeButton(item) for item in items)

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

@@ -19,6 +19,10 @@ Guest = proxyquire('../guest', {
   'scroll-into-view': scrollIntoView,
 })
 
+# A little helper which returns a promise that resolves after a timeout
+timeoutPromise = (millis = 0) ->
+  new Promise((resolve) -> setTimeout(resolve, millis))
+
 describe 'Guest', ->
   sandbox = sinon.sandbox.create()
   CrossFrame = null
@@ -215,18 +219,20 @@ describe 'Guest', ->
       assert.isTrue(event.isPropagationStopped())
 
   describe 'createAnnotation()', ->
-    it 'adds metadata to the annotation object', (done) ->
+    it 'adds metadata to the annotation object', ->
       guest = createGuest()
       sinon.stub(guest, 'getDocumentInfo').returns(Promise.resolve({
         metadata: {title: 'hello'}
         uri: 'http://example.com/'
       }))
       annotation = {}
+
       guest.createAnnotation(annotation)
-      setTimeout ->
+
+      timeoutPromise()
+      .then ->
         assert.equal(annotation.uri, 'http://example.com/')
         assert.deepEqual(annotation.document, {title: 'hello'})
-        done()
 
     it 'treats an argument as the annotation object', ->
       guest = createGuest()
@@ -234,6 +240,46 @@ describe 'Guest', ->
       annotation = guest.createAnnotation(annotation)
       assert.equal(annotation.foo, 'bar')
 
+    it 'triggers a beforeAnnotationCreated event', (done) ->
+      guest = createGuest()
+      guest.subscribe('beforeAnnotationCreated', -> done())
+
+      guest.createAnnotation()
+
+  describe 'createComment()', ->
+    it 'adds metadata to the annotation object', ->
+      guest = createGuest()
+      sinon.stub(guest, 'getDocumentInfo').returns(Promise.resolve({
+        metadata: {title: 'hello'}
+        uri: 'http://example.com/'
+      }))
+
+      annotation = guest.createComment()
+
+      timeoutPromise()
+      .then ->
+        assert.equal(annotation.uri, 'http://example.com/')
+        assert.deepEqual(annotation.document, {title: 'hello'})
+
+    it 'adds a single target with a source property', ->
+      guest = createGuest()
+      sinon.stub(guest, 'getDocumentInfo').returns(Promise.resolve({
+        metadata: {title: 'hello'}
+        uri: 'http://example.com/'
+      }))
+
+      annotation = guest.createComment()
+
+      timeoutPromise()
+      .then ->
+        assert.deepEqual(annotation.target, [{source: 'http://example.com/'}])
+
+    it 'triggers a beforeAnnotationCreated event', (done) ->
+      guest = createGuest()
+      guest.subscribe('beforeAnnotationCreated', -> done())
+
+      guest.createComment()
+
   describe 'anchor()', ->
     el = null
     range = null
@@ -249,28 +295,60 @@ describe 'Guest', ->
     afterEach ->
       document.body.removeChild(el)
 
-    it "doesn't declare annotations without targets as orphans", (done) ->
+    it "doesn't mark an annotation lacking targets as an orphan", ->
       guest = createGuest()
       annotation = target: []
+
       guest.anchor(annotation).then ->
         assert.isFalse(annotation.$orphan)
-      .then(done, done)
 
-    it "doesn't declare annotations with a working target orphans", (done) ->
+    it "doesn't mark an annotation with a selectorless target as an orphan", ->
       guest = createGuest()
-      annotation = target: [{selector: "test"}]
+      annotation = {target: [{source: 'wibble'}]}
+
+      guest.anchor(annotation).then ->
+        assert.isFalse(annotation.$orphan)
+
+    it "doesn't mark an annotation with only selectorless targets as an orphan", ->
+      guest = createGuest()
+      annotation = {target: [{source: 'foo'}, {source: 'bar'}]}
+
+      guest.anchor(annotation).then ->
+        assert.isFalse(annotation.$orphan)
+
+    it "doesn't mark an annotation in which the target anchors as an orphan", ->
+      guest = createGuest()
+      annotation = {target: [{selector: []}]}
       sandbox.stub(anchoring, 'anchor').returns(Promise.resolve(range))
+
       guest.anchor(annotation).then ->
         assert.isFalse(annotation.$orphan)
-      .then(done, done)
 
-    it "declares annotations with broken targets as orphans", (done) ->
+    it "doesn't mark an annotation in which at least one target anchors as an orphan", ->
+      guest = createGuest()
+      annotation = {target: [{selector: []}, {selector: []}]}
+      sandbox.stub(anchoring, 'anchor')
+        .onFirstCall().returns(Promise.reject())
+        .onSecondCall().returns(Promise.resolve(range))
+
+      guest.anchor(annotation).then ->
+        assert.isFalse(annotation.$orphan)
+
+    it "marks an annotation in which the target fails to anchor as an orphan", ->
+      guest = createGuest()
+      annotation = target: [{selector: []}]
+      sandbox.stub(anchoring, 'anchor').returns(Promise.reject())
+
+      guest.anchor(annotation).then ->
+        assert.isTrue(annotation.$orphan)
+
+    it "marks an annotation in which all (suitable) targets fail to anchor as an orphan", ->
       guest = createGuest()
-      annotation = target: [{selector: 'broken selector'}]
+      annotation = target: [{selector: []}, {selector: []}]
       sandbox.stub(anchoring, 'anchor').returns(Promise.reject())
+
       guest.anchor(annotation).then ->
         assert.isTrue(annotation.$orphan)
-      .then(done, done)
 
     it 'updates the cross frame and bucket bar plugins', (done) ->
       guest = createGuest()

h/static/scripts/directive/annotation.coffee

@@ -83,14 +83,6 @@ AnnotationController = [
     this.tagsAutoComplete = (query) ->
       $q.when(tags.filter(query))
 
-    ###*
-    # @ngdoc method
-    # @name annotation.AnnotationController#isComment.
-    # @returns {boolean} True if the annotation is a comment.
-    ###
-    this.isComment = ->
-      not (model.target?.length or model.references?.length)
-
     ###*
     # @ngdoc method
     # @name annotation.AnnotationController#isHighlight.