Merge pull request #3526 from hypothesis/gh3523-non-text-selection-fix

Fix adder breaking after making a selection containing no text

h/static/scripts/annotator/guest.coffee

@@ -359,6 +359,14 @@ module.exports = class Guest extends Annotator
     @crossframe?.call('focusAnnotations', tags)
 
   _onSelection: (range) ->
+    selection = Annotator.Util.getGlobal().getSelection()
+    isBackwards = rangeUtil.isSelectionBackwards(selection)
+    focusRect = rangeUtil.selectionFocusRect(selection)
+    if !focusRect
+      # The selected range does not contain any text
+      this._onClearSelection()
+      return
+
     @selectedRanges = [range]
 
     Annotator.$('.annotator-toolbar .h-icon-note')
@@ -366,9 +374,6 @@ module.exports = class Guest extends Annotator
       .removeClass('h-icon-note')
       .addClass('h-icon-annotate');
 
-    selection = Annotator.Util.getGlobal().getSelection()
-    isBackwards = rangeUtil.isSelectionBackwards(selection)
-    focusRect = rangeUtil.selectionFocusRect(selection)
     {left, top, arrowDirection} = this.adderCtrl.target(focusRect, isBackwards)
     this.adderCtrl.showAt(left, top, arrowDirection)
 

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

 Annotator = require('annotator')
+proxyquire = require('proxyquire')
+
+adder = require('../adder')
+Observable = require('../../util/observable').Observable
+
 $ = Annotator.$
 Annotator['@noCallThru'] = true;
 
-highlighter = {}
+Guest = null
 anchoring = {}
+highlighter = {}
+rangeUtil = null
+selections = null
 
 raf = sinon.stub().yields()
 raf['@noCallThru'] = true
@@ -11,14 +19,15 @@ raf['@noCallThru'] = true
 scrollIntoView = sinon.stub()
 scrollIntoView['@noCallThru'] = true
 
-proxyquire = require('proxyquire')
-Guest = proxyquire('../guest', {
-  './highlighter': highlighter,
-  './anchoring/html': anchoring,
-  'annotator': Annotator,
-  'raf': raf,
-  'scroll-into-view': scrollIntoView,
-})
+class FakeAdder
+  instance: null
+
+  constructor: ->
+    FakeAdder::instance = this
+
+    this.hide = sinon.stub()
+    this.showAt = sinon.stub()
+    this.target = sinon.stub()
 
 # A little helper which returns a promise that resolves after a timeout
 timeoutPromise = (millis = 0) ->
@@ -34,6 +43,28 @@ describe 'Guest', ->
     return new Guest(element, options || {})
 
   beforeEach ->
+    FakeAdder::instance = null
+    rangeUtil = {
+      isSelectionBackwards: sinon.stub()
+      selectionFocusRect: sinon.stub()
+    }
+    selections = null
+
+    Guest = proxyquire('../guest', {
+      './adder': {Adder: FakeAdder},
+      './anchoring/html': anchoring,
+      './highlighter': highlighter,
+      './range-util': rangeUtil,
+      './selections': (document) ->
+        new Observable((obs) ->
+          selections = obs
+          return () ->
+        )
+      'annotator': Annotator,
+      'raf': raf,
+      'scroll-into-view': scrollIntoView,
+    })
+
     fakeCrossFrame = {
       onConnect: sinon.stub()
       on: sinon.stub()
@@ -206,7 +237,29 @@ describe 'Guest', ->
 
         emitGuestEvent('getDocumentInfo', assertComplete)
 
-  describe 'getDocumentInfo()', ->
+  describe 'when the selection changes', ->
+    it 'shows the adder if the selection contains text', ->
+      guest = createGuest()
+      rangeUtil.selectionFocusRect.returns({left: 0, top: 0, width: 5, height: 5})
+      FakeAdder::instance.target.returns({
+        left: 0, top: 0, arrowDirection: adder.ARROW_POINTING_UP
+      })
+      selections.next({})
+      assert.called FakeAdder::instance.showAt
+
+    it 'hides the adder if the selection does not contain text', ->
+      guest = createGuest()
+      rangeUtil.selectionFocusRect.returns(null)
+      selections.next({})
+      assert.called FakeAdder::instance.hide
+      assert.notCalled FakeAdder::instance.showAt
+
+    it 'hides the adder if the selection is empty', ->
+      guest = createGuest()
+      selections.next(null)
+      assert.called FakeAdder::instance.hide
+
+  describe '#getDocumentInfo()', ->
     guest = null
 
     beforeEach ->
@@ -228,7 +281,7 @@ describe 'Guest', ->
       return guest.getDocumentInfo().then ({uri}) ->
         assert.equal uri, 'urn:x-pdf:aabbcc'
 
-  describe 'onAdderMouseUp', ->
+  describe '#onAdderMouseUp()', ->
     it 'it prevents the default browser action when triggered', () ->
       event = jQuery.Event('mouseup')
       guest = createGuest()
@@ -241,7 +294,7 @@ describe 'Guest', ->
       guest.onAdderMouseup(event)
       assert.isTrue(event.isPropagationStopped())
 
-  describe 'createAnnotation()', ->
+  describe '#createAnnotation()', ->
     it 'adds metadata to the annotation object', ->
       guest = createGuest()
       sinon.stub(guest, 'getDocumentInfo').returns(Promise.resolve({
@@ -269,7 +322,7 @@ describe 'Guest', ->
 
       guest.createAnnotation()
 
-  describe 'createComment()', ->
+  describe '#createComment()', ->
     it 'adds metadata to the annotation object', ->
       guest = createGuest()
       sinon.stub(guest, 'getDocumentInfo').returns(Promise.resolve({
@@ -303,7 +356,7 @@ describe 'Guest', ->
 
       guest.createComment()
 
-  describe 'anchor()', ->
+  describe '#anchor()', ->
     el = null
     range = null
 
@@ -478,7 +531,7 @@ describe 'Guest', ->
           assert.calledOnce(stub)
       .then(done, done)
 
-  describe 'detach()', ->
+  describe '#detach()', ->
     it 'removes the anchors from the "anchors" instance variable', ->
       guest = createGuest()
       annotation = {}