From cf69943d3019499cc9c55718498d48b44d728c13 Mon Sep 17 00:00:00 2001 From: Sumedha Pramod Date: Fri, 16 Mar 2018 11:35:07 -0700 Subject: [PATCH] Fix: Binds handlers appropriately for desktop/touch/mobile devices (#139) - Fixes situation where touch events are improperly bound in the shared mobile annotation dialog and therefore prevents the keyboard from appearing on mobile dialogs --- src/AnnotationDialog.js | 5 +- src/CreateAnnotationDialog.js | 20 ++++--- src/__tests__/AnnotationDialog-test.js | 8 +-- src/__tests__/CreateAnnotationDialog-test.js | 1 + src/controllers/DrawingModeController.js | 12 +++-- .../__tests__/DrawingModeController-test.js | 36 ++++++++++--- src/doc/DocDrawingDialog.js | 8 ++- src/doc/__tests__/DocDrawingDialog-test.js | 54 ++++++++++--------- 8 files changed, 91 insertions(+), 53 deletions(-) diff --git a/src/AnnotationDialog.js b/src/AnnotationDialog.js index 7bb2dbeed..0656b30e0 100644 --- a/src/AnnotationDialog.js +++ b/src/AnnotationDialog.js @@ -366,7 +366,6 @@ class AnnotationDialog extends EventEmitter { bindDOMListeners() { this.element.addEventListener('keydown', this.keydownHandler); this.element.addEventListener('wheel', this.stopPropagation); - this.element.addEventListener('click', this.clickHandler); this.element.addEventListener('mouseup', this.stopPropagation); if (this.hasTouch) { @@ -385,6 +384,7 @@ class AnnotationDialog extends EventEmitter { } if (!this.isMobile) { + this.element.addEventListener('click', this.clickHandler); this.element.addEventListener('mouseenter', this.mouseenterHandler); this.element.addEventListener('mouseleave', this.mouseleaveHandler); return; @@ -422,7 +422,6 @@ class AnnotationDialog extends EventEmitter { */ unbindDOMListeners() { this.element.removeEventListener('keydown', this.keydownHandler); - this.element.removeEventListener('click', this.clickHandler); this.element.removeEventListener('mouseup', this.stopPropagation); this.element.removeEventListener('wheel', this.stopPropagation); @@ -442,6 +441,7 @@ class AnnotationDialog extends EventEmitter { } if (!this.isMobile) { + this.element.removeEventListener('click', this.clickHandler); this.element.removeEventListener('mouseenter', this.mouseenterHandler); this.element.removeEventListener('mouseleave', this.mouseleaveHandler); return; @@ -580,7 +580,6 @@ class AnnotationDialog extends EventEmitter { */ clickHandler(event) { event.stopPropagation(); - event.preventDefault(); const eventTarget = event.target; const dataType = util.findClosestDataType(eventTarget); diff --git a/src/CreateAnnotationDialog.js b/src/CreateAnnotationDialog.js index fb3846fc2..dd740c898 100644 --- a/src/CreateAnnotationDialog.js +++ b/src/CreateAnnotationDialog.js @@ -143,10 +143,12 @@ class CreateAnnotationDialog extends EventEmitter { this.hide(); - // Stop interacting with this element from triggering outside actions - this.containerEl.removeEventListener('click', this.stopPropagation); - this.containerEl.removeEventListener('mouseup', this.stopPropagation); - this.containerEl.removeEventListener('dblclick', this.stopPropagation); + if (!this.isMobile) { + // Stop interacting with this element from triggering outside actions + this.containerEl.removeEventListener('click', this.stopPropagation); + this.containerEl.removeEventListener('mouseup', this.stopPropagation); + this.containerEl.removeEventListener('dblclick', this.stopPropagation); + } // Event listeners if (this.hasTouch) { @@ -276,10 +278,12 @@ class CreateAnnotationDialog extends EventEmitter { this.containerEl.classList.add(CLASS_MOBILE_CREATE_ANNOTATION_DIALOG); this.containerEl.classList.add(CLASS_ANNOTATION_DIALOG); - // Stop interacting with this element from triggering outside actions - this.containerEl.addEventListener('click', this.stopPropagation); - this.containerEl.addEventListener('mouseup', this.stopPropagation); - this.containerEl.addEventListener('dblclick', this.stopPropagation); + if (!this.isMobile) { + // Stop interacting with this element from triggering outside actions + this.containerEl.addEventListener('click', this.stopPropagation); + this.containerEl.addEventListener('mouseup', this.stopPropagation); + this.containerEl.addEventListener('dblclick', this.stopPropagation); + } // touch events if (this.hasTouch) { diff --git a/src/__tests__/AnnotationDialog-test.js b/src/__tests__/AnnotationDialog-test.js index eca643190..e1358472f 100644 --- a/src/__tests__/AnnotationDialog-test.js +++ b/src/__tests__/AnnotationDialog-test.js @@ -423,7 +423,7 @@ describe('AnnotationDialog', () => { sandbox.stub(stubs.annotationTextEl, 'addEventListener'); }); - it('should bind DOM listeners', () => { + it('should bind ALL DOM listeners for touch enabled laptops', () => { dialog.hasTouch = true; dialog.bindDOMListeners(); @@ -457,8 +457,8 @@ describe('AnnotationDialog', () => { dialog.bindDOMListeners(); expect(stubs.add).to.be.calledWith('keydown', sinon.match.func); - expect(stubs.add).to.be.calledWith('click', sinon.match.func); expect(stubs.add).to.be.calledWith('mouseup', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('click', 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); expect(stubs.add).to.be.calledWith('wheel', sinon.match.func); @@ -499,7 +499,7 @@ describe('AnnotationDialog', () => { sandbox.stub(stubs.annotationTextEl, 'removeEventListener'); }); - it('should unbind DOM listeners', () => { + it('should unbind ALL DOM listeners for touch enabled laptops', () => { dialog.hasTouch = true; dialog.unbindDOMListeners(); @@ -533,8 +533,8 @@ describe('AnnotationDialog', () => { dialog.unbindDOMListeners(); expect(stubs.remove).to.be.calledWith('keydown', sinon.match.func); - expect(stubs.remove).to.be.calledWith('click', sinon.match.func); expect(stubs.remove).to.be.calledWith('mouseup', sinon.match.func); + expect(stubs.remove).to.not.be.calledWith('click', 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); expect(stubs.remove).to.be.calledWith('wheel', sinon.match.func); diff --git a/src/__tests__/CreateAnnotationDialog-test.js b/src/__tests__/CreateAnnotationDialog-test.js index 29faf4ca9..9a98a0992 100644 --- a/src/__tests__/CreateAnnotationDialog-test.js +++ b/src/__tests__/CreateAnnotationDialog-test.js @@ -176,6 +176,7 @@ describe('CreateAnnotationDialog', () => { }); it('should remove events that are bound to stopPropagation()', () => { + dialog.isMobile = false; const remove = sandbox.stub(dialog.containerEl, 'removeEventListener'); dialog.destroy(); expect(remove).to.be.calledWith('click'); diff --git a/src/controllers/DrawingModeController.js b/src/controllers/DrawingModeController.js index f69de2c78..0675e9646 100644 --- a/src/controllers/DrawingModeController.js +++ b/src/controllers/DrawingModeController.js @@ -69,18 +69,22 @@ class DrawingModeController extends AnnotationModeController { /** @inheritdoc */ bindDOMListeners() { - if (this.isMobile && this.hasTouch) { + if (this.hasTouch) { this.annotatedElement.addEventListener('touchstart', this.handleSelection); - } else { + } + + if (!this.isMobile) { this.annotatedElement.addEventListener('click', this.handleSelection); } } /** @inheritdoc */ unbindDOMListeners() { - if (this.isMobile && this.hasTouch) { + if (this.hasTouch) { this.annotatedElement.removeEventListener('touchstart', this.handleSelection); - } else { + } + + if (!this.isMobile) { this.annotatedElement.removeEventListener('click', this.handleSelection); } } diff --git a/src/controllers/__tests__/DrawingModeController-test.js b/src/controllers/__tests__/DrawingModeController-test.js index 04db3e39b..9d828df32 100644 --- a/src/controllers/__tests__/DrawingModeController-test.js +++ b/src/controllers/__tests__/DrawingModeController-test.js @@ -90,17 +90,27 @@ describe('controllers/DrawingModeController', () => { stubs.add = controller.annotatedElement.addEventListener; }); - it('should unbind the mobileDOM listeners', () => { + it('should bind DOM listeners for desktop devices', () => { + controller.isMobile = false; + controller.hasTouch = false; + controller.bindDOMListeners(); + expect(stubs.add).to.be.calledWith('click', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('touchstart', sinon.match.func); + }); + + it('should bind DOM listeners for touch enabled mobile devices', () => { controller.isMobile = true; controller.hasTouch = true; controller.bindDOMListeners(); expect(stubs.add).to.be.calledWith('touchstart', sinon.match.func); + expect(stubs.add).to.not.be.calledWith('click', sinon.match.func); }); - it('should unbind the DOM listeners', () => { - controller.isMobile = true; - controller.hasTouch = false; + it('should bind ALL DOM listeners for touch enabled desktop devices', () => { + controller.isMobile = false; + controller.hasTouch = true; controller.bindDOMListeners(); + expect(stubs.add).to.be.calledWith('touchstart', sinon.match.func); expect(stubs.add).to.be.calledWith('click', sinon.match.func); }); }); @@ -113,17 +123,27 @@ describe('controllers/DrawingModeController', () => { stubs.remove = controller.annotatedElement.removeEventListener; }); - it('should unbind the mobileDOM listeners', () => { + it('should unbind DOM listeners for desktop devices', () => { + controller.isMobile = false; + controller.hasTouch = false; + controller.unbindDOMListeners(); + expect(stubs.remove).to.be.calledWith('click', sinon.match.func); + expect(stubs.remove).to.not.be.calledWith('touchstart', sinon.match.func); + }); + + it('should unbind DOM listeners for touch enabled mobile devices', () => { controller.isMobile = true; controller.hasTouch = true; controller.unbindDOMListeners(); expect(stubs.remove).to.be.calledWith('touchstart', sinon.match.func); + expect(stubs.remove).to.not.be.calledWith('click', sinon.match.func); }); - it('should unbind the DOM listeners', () => { - controller.isMobile = true; - controller.hasTouch = false; + it('should unbind ALL DOM listeners for touch enabled desktop devices', () => { + controller.isMobile = false; + controller.hasTouch = true; controller.unbindDOMListeners(); + expect(stubs.remove).to.be.calledWith('touchstart', sinon.match.func); expect(stubs.remove).to.be.calledWith('click', sinon.match.func); }); }); diff --git a/src/doc/DocDrawingDialog.js b/src/doc/DocDrawingDialog.js index 24c4ead6d..be975a287 100644 --- a/src/doc/DocDrawingDialog.js +++ b/src/doc/DocDrawingDialog.js @@ -82,7 +82,9 @@ class DocDrawingDialog extends AnnotationDialog { */ bindDOMListeners() { if (this.commitButtonEl) { - this.commitButtonEl.addEventListener('click', this.postDrawing); + if (!this.isMobile) { + this.commitButtonEl.addEventListener('click', this.postDrawing); + } if (this.hasTouch) { this.commitButtonEl.addEventListener('touchend', this.postDrawing); @@ -90,7 +92,9 @@ class DocDrawingDialog extends AnnotationDialog { } if (this.deleteButtonEl) { - this.deleteButtonEl.addEventListener('click', this.deleteAnnotation); + if (!this.isMobile) { + this.deleteButtonEl.addEventListener('click', this.deleteAnnotation); + } if (this.hasTouch) { this.deleteButtonEl.addEventListener('touchend', this.deleteAnnotation); diff --git a/src/doc/__tests__/DocDrawingDialog-test.js b/src/doc/__tests__/DocDrawingDialog-test.js index 5a852616b..6ce568f78 100644 --- a/src/doc/__tests__/DocDrawingDialog-test.js +++ b/src/doc/__tests__/DocDrawingDialog-test.js @@ -60,39 +60,45 @@ describe('doc/DocDrawingDialog', () => { }); describe('bindDOMListeners()', () => { - it('should bind listeners to a commit button element', () => { - dialog.hasTouch = true; + beforeEach(() => { dialog.commitButtonEl = { addEventListener: sandbox.stub() }; + stubs.commitAdd = dialog.commitButtonEl.addEventListener; - dialog.bindDOMListeners(); - expect(dialog.commitButtonEl.addEventListener).to.be.calledWith( - 'click', - dialog.postDrawing - ); - expect(dialog.commitButtonEl.addEventListener).to.be.calledWith( - 'touchend', - dialog.postDrawing - ); - }); - - it('should bind listeners to a delete button element', () => { - dialog.hasTouch = true; dialog.deleteButtonEl = { addEventListener: sandbox.stub() }; + stubs.deleteAdd = dialog.deleteButtonEl.addEventListener; + }); + it('should bind touch listeners to buttons', () => { + dialog.isMobile = true; + dialog.hasTouch = true; dialog.bindDOMListeners(); - expect(dialog.deleteButtonEl.addEventListener).to.be.calledWith( - 'click', - dialog.deleteAnnotation - ); - expect(dialog.deleteButtonEl.addEventListener).to.be.calledWith( - 'touchend', - dialog.deleteAnnotation - ); - }) + expect(stubs.commitAdd).to.not.be.calledWith('click', dialog.postDrawing); + expect(stubs.commitAdd).to.be.calledWith('touchend', dialog.postDrawing); + expect(stubs.deleteAdd).to.not.be.calledWith('click', dialog.deleteAnnotation); + expect(stubs.deleteAdd).to.be.calledWith('touchend', dialog.deleteAnnotation); + }); + it('should bind touch-enabled desktop listeners to buttons', () => { + dialog.isMobile = false; + dialog.hasTouch = true; + dialog.bindDOMListeners(); + expect(stubs.commitAdd).to.be.calledWith('click', dialog.postDrawing); + expect(stubs.commitAdd).to.be.calledWith('touchend', dialog.postDrawing); + expect(stubs.deleteAdd).to.be.calledWith('click', dialog.deleteAnnotation); + expect(stubs.deleteAdd).to.be.calledWith('touchend', dialog.deleteAnnotation); + }); + it('should bind desktop listeners to buttons', () => { + dialog.isMobile = false; + dialog.hasTouch = false; + dialog.bindDOMListeners(); + expect(stubs.commitAdd).to.be.calledWith('click', dialog.postDrawing); + expect(stubs.commitAdd).to.not.be.calledWith('touchend', dialog.postDrawing); + expect(stubs.deleteAdd).to.be.calledWith('click', dialog.deleteAnnotation); + expect(stubs.deleteAdd).to.not.be.calledWith('touchend', dialog.deleteAnnotation); + }); }); describe('unbindDOMListeners()', () => {