Fix: Fixes issues with adding annotations on tablets (#186)

- Some listeners were never unbound from the docHighlightDialog which caused issues with the shared mobile dialog
- Cleaning up some other small things I noticed relating to highlights

src/lib/annotations/doc/DocHighlightDialog.js

@@ -159,7 +159,9 @@ const PAGE_PADDING_TOP = 15;
         }
 
         // Reposition dialog
-        this.position();
+        if (!this.isMobile) {
+            this.position();
+        }
     }
 
     //--------------------------------------------------------------------------
@@ -255,10 +257,12 @@ const PAGE_PADDING_TOP = 15;
      */
     bindDOMListeners() {
         this.element.addEventListener('mousedown', this.mousedownHandler);
-        this.element.addEventListener('mouseup', this.mouseupHandler);
         this.element.addEventListener('keydown', this.keydownHandler);
-        this.element.addEventListener('mouseenter', this.mouseenterHandler);
-        this.element.addEventListener('mouseleave', this.mouseleaveHandler);
+
+        if (!this.isMobile) {
+            this.element.addEventListener('mouseenter', this.mouseenterHandler);
+            this.element.addEventListener('mouseleave', this.mouseleaveHandler);
+        }
     }
 
     /**
@@ -270,10 +274,12 @@ const PAGE_PADDING_TOP = 15;
      */
     unbindDOMListeners() {
         this.element.removeEventListener('mousedown', this.mousedownHandler);
-        this.element.removeEventListener('mouseup', this.mouseupHandler);
         this.element.removeEventListener('keydown', this.keydownHandler);
-        this.element.removeEventListener('mouseenter', this.mouseenterHandler);
-        this.element.removeEventListener('mouseleave', this.mouseleaveHandler);
+
+        if (!this.isMobile) {
+            this.element.removeEventListener('mouseenter', this.mouseenterHandler);
+            this.element.removeEventListener('mouseleave', this.mouseleaveHandler);
+        }
     }
 
     /**

src/lib/annotations/doc/DocHighlightThread.js

@@ -360,11 +360,11 @@ const HOVER_TIMEOUT_MS = 75;
      * @return {void}
      */
     unbindCustomListenersOnDialog() {
-        this.removeAllListeners('annotationdraw');
-        this.removeAllListeners('annotationcommentpending');
-        this.removeAllListeners('annotationcreate');
-        this.removeAllListeners('annotationcancel');
-        this.removeAllListeners('annotationdelete');
+        this.dialog.removeAllListeners('annotationdraw');
+        this.dialog.removeAllListeners('annotationcommentpending');
+        this.dialog.removeAllListeners('annotationcreate');
+        this.dialog.removeAllListeners('annotationcancel');
+        this.dialog.removeAllListeners('annotationdelete');
     }
 
     //--------------------------------------------------------------------------

src/lib/annotations/doc/__tests__/DocHighlightDialog-test.js

@@ -5,6 +5,7 @@ import AnnotationDialog from '../../AnnotationDialog';
 import * as annotatorUtil from '../../annotatorUtil';
 import * as docAnnotatorUtil from '../docAnnotatorUtil';
 import { CLASS_HIDDEN } from '../../../constants';
+import * as util from '../../../util';
 import * as constants from '../../annotationConstants';
 
 let dialog;
@@ -190,6 +191,19 @@ describe('lib/annotations/doc/DocHighlightDialog', () => {
             expect(commentTextEl.classList.contains(CLASS_HIDDEN)).to.be.true;
             expect(replyTextEl.classList.contains(CLASS_HIDDEN)).to.be.false;
         });
+
+        it('should reposition the dialog if using a desktop browser', () => {
+            sandbox.stub(dialog, 'position');
+            dialog.toggleHighlightCommentsReply();
+            expect(dialog.position).to.be.called;
+        });
+
+        it('should not reposition the dialog on a mobile device', () => {
+            sandbox.stub(dialog, 'position');
+            dialog.isMobile = true;
+            dialog.toggleHighlightCommentsReply();
+            expect(dialog.position).to.not.be.called;
+        });
     });
 
     describe('setup()', () => {
@@ -297,6 +311,110 @@ describe('lib/annotations/doc/DocHighlightDialog', () => {
         });
     });
 
+    describe('bindDOMListeners()', () => {
+        it('should bind DOM listeners', () => {
+            stubs.add = sandbox.stub(dialog.element, 'addEventListener');
+
+            dialog.bindDOMListeners();
+            expect(stubs.add).to.be.calledWith('mousedown', sinon.match.func);
+            expect(stubs.add).to.be.calledWith('keydown', sinon.match.func);
+            expect(stubs.add).to.be.calledWith('mouseleave', sinon.match.func);
+            expect(stubs.add).to.be.calledWith('mouseenter', sinon.match.func);
+        });
+
+        it('should not bind mouseenter/leave events for mobile browsers', () => {
+            stubs.add = sandbox.stub(dialog.element, 'addEventListener');
+            dialog.isMobile = true;
+
+            dialog.bindDOMListeners();
+            expect(stubs.add).to.be.calledWith('mousedown', sinon.match.func);
+            expect(stubs.add).to.be.calledWith('keydown', sinon.match.func);
+            expect(stubs.add).to.not.be.calledWith('mouseenter', sinon.match.func);
+            expect(stubs.add).to.not.be.calledWith('mouseleave', sinon.match.func);
+        });
+    });
+
+    describe('unbindDOMListeners()', () => {
+        it('should unbind DOM listeners', () => {
+            stubs.remove = sandbox.stub(dialog.element, 'removeEventListener');
+
+            dialog.unbindDOMListeners();
+            expect(stubs.remove).to.be.calledWith('mousedown', sinon.match.func);
+            expect(stubs.remove).to.be.calledWith('keydown', sinon.match.func);
+            expect(stubs.remove).to.be.calledWith('mouseleave', sinon.match.func);
+            expect(stubs.remove).to.be.calledWith('mouseenter', sinon.match.func);
+        });
+
+        it('should not unbind mouseenter/leave events for mobile browsers', () => {
+            stubs.remove = sandbox.stub(dialog.element, 'removeEventListener');
+            dialog.isMobile = true;
+
+            dialog.unbindDOMListeners();
+            expect(stubs.remove).to.be.calledWith('mousedown', sinon.match.func);
+            expect(stubs.remove).to.be.calledWith('keydown', sinon.match.func);
+            expect(stubs.remove).to.not.be.calledWith('mouseenter', sinon.match.func);
+            expect(stubs.remove).to.not.be.calledWith('mouseleave', sinon.match.func);
+        });
+    });
+
+    describe('keydownHandler()', () => {
+        const event = {
+            stopPropagation: sandbox.stub()
+        };
+
+        it('should call mousedownHandler if \'Enter\' is pressed', () => {
+            sandbox.stub(util, 'decodeKeydown').returns('Enter');
+            sandbox.stub(dialog, 'mousedownHandler');
+            dialog.keydownHandler(event);
+            expect(dialog.mousedownHandler).to.be.called;
+        });
+
+        it('should not call mousedownHandler if a key other than \'Enter\' is pressed', () => {
+            sandbox.stub(util, 'decodeKeydown').returns('Delete');
+            sandbox.stub(dialog, 'mousedownHandler');
+            dialog.keydownHandler(event);
+            expect(dialog.mousedownHandler).to.not.be.called;
+        });
+    });
+
+    describe('mousedownHandler()', () => {
+        const event = {
+            stopPropagation: sandbox.stub(),
+            preventDefault: sandbox.stub()
+        };
+
+        beforeEach(() => {
+            stubs.dataType = sandbox.stub(annotatorUtil, 'findClosestDataType');
+            sandbox.stub(dialog, 'toggleHighlight');
+            sandbox.stub(dialog, 'toggleHighlightCommentsReply');
+            sandbox.stub(dialog, 'toggleHighlightDialogs');
+            sandbox.stub(dialog, 'focusAnnotationsTextArea');
+        });
+
+        it('should create/remove a highlight when the \'highlight-btn\' is pressed', () => {
+            stubs.dataType.returns('highlight-btn');
+            dialog.mousedownHandler(event);
+            expect(stubs.emit).to.be.calledWith('annotationdraw');
+            expect(dialog.toggleHighlight).to.be.called;
+        });
+
+        it('should create highlight when the \'add-highlight-comment-btn\' is pressed', () => {
+            stubs.dataType.returns('add-highlight-comment-btn');
+            dialog.mousedownHandler(event);
+            expect(stubs.emit).to.be.calledWith('annotationdraw');
+            expect(dialog.toggleHighlightCommentsReply).to.be.called;
+            expect(dialog.toggleHighlightDialogs).to.be.called;
+            expect(event.preventDefault).to.be.called;
+            expect(dialog.focusAnnotationsTextArea).to.be.called;
+        });
+
+        it('should not create a highlight when any other button is pressed', () => {
+            stubs.dataType.returns('anything-else');
+            dialog.mousedownHandler(event);
+            expect(stubs.emit).to.not.be.calledWith('annotationdraw');
+        });
+    });
+
     describe('toggleHighlightIcon()', () => {
         it('should display active highlight icon when highlight is active', () => {
             const addHighlightBtn = dialog.element.querySelector('.bp-add-highlight-btn');
@@ -399,4 +517,12 @@ describe('lib/annotations/doc/DocHighlightDialog', () => {
             expect(highlight.dataset.annotationId).to.equal('1');
         });
     });
+
+    describe('generateHighlightDialogEl()', () => {
+        it('should return a highlight annotation dialog DOM element', () => {
+            const highlightEl = dialog.generateHighlightDialogEl();
+            const highlightBtnEl = highlightEl.querySelector('.bp-annotations-highlight-btns');
+            expect(highlightBtnEl).to.not.be.null;
+        });
+    });
 });

src/lib/annotations/doc/__tests__/DocHighlightThread-test.js

@@ -436,6 +436,17 @@ describe('lib/annotations/doc/DocHighlightThread', () => {
             expect(highlightThread.showDialog).to.be.called;
             expect(highlightThread.draw).to.be.calledWith(constants.HIGHLIGHT_ACTIVE_FILL_STYLE);
         });
+
+        it('should do nothing if state is invalid', () => {
+            sandbox.stub(highlightThread, 'showDialog');
+            sandbox.stub(highlightThread, 'draw');
+
+            highlightThread.state = 'invalid';
+            highlightThread.show();
+
+            expect(highlightThread.showDialog).to.not.be.called;
+            expect(highlightThread.draw).to.not.be.called;
+        });
     });
 
     describe('createDialog()', () => {
@@ -463,6 +474,24 @@ describe('lib/annotations/doc/DocHighlightThread', () => {
         });
     });
 
+    describe('unbindCustomListenersOnDialog()', () => {
+        it('should unbind custom listeners on dialog', () => {
+            highlightThread.dialog = {
+                removeAllListeners: () => {}
+            };
+
+            const removeAllListenersStub = sandbox.stub(highlightThread.dialog, 'removeAllListeners');
+
+            highlightThread.unbindCustomListenersOnDialog();
+
+            expect(removeAllListenersStub).to.be.calledWith('annotationdraw');
+            expect(removeAllListenersStub).to.be.calledWith('annotationcommentpending');
+            expect(removeAllListenersStub).to.be.calledWith('annotationcreate');
+            expect(removeAllListenersStub).to.be.calledWith('annotationcancel');
+            expect(removeAllListenersStub).to.be.calledWith('annotationdelete');
+        });
+    });
+
     describe('draw()', () => {
         it('should not draw if no context exists', () => {
             sandbox.stub(highlightThread, 'getPageEl');