diff --git a/src/lib/annotations/AnnotationDialog.js b/src/lib/annotations/AnnotationDialog.js index 7ee9e2c8e..39251c01e 100644 --- a/src/lib/annotations/AnnotationDialog.js +++ b/src/lib/annotations/AnnotationDialog.js @@ -150,6 +150,12 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog'; dialogCloseButtonEl.removeEventListener('click', this.hideMobileDialog); annotatorUtil.hideElement(this.element); + this.unbindDOMListeners(); + + // Cancel any unsaved annotations + if (!this.hasAnnotations) { + this.cancelAnnotation(); + } } /** @@ -218,7 +224,7 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog'; /** * Sets up the dialog element. * - * @param {Annotation[]} Annotations - to show in the dialog + * @param {Annotation[]} annotations - to show in the dialog * @return {void} * @protected */ @@ -370,7 +376,12 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog'; break; // Clicking 'Cancel' button to cancel the annotation case 'cancel-annotation-btn': - this.cancelAnnotation(); + if (this.isMobile) { + this.hideMobileDialog(); + } else { + this.cancelAnnotation(); + } + this.deactivateReply(true); break; // Clicking inside reply text area @@ -529,7 +540,7 @@ const CLASS_ANIMATE_DIALOG = 'bp-animate-show-dialog'; * Deactivate reply textarea. * * @private - * @param {Boolean} clearText - Whether or not text in text area should be cleared + * @param {boolean} clearText - Whether or not text in text area should be cleared * @return {void} */ deactivateReply(clearText) { diff --git a/src/lib/annotations/AnnotationThread.js b/src/lib/annotations/AnnotationThread.js index 271593fac..78dbdea73 100644 --- a/src/lib/annotations/AnnotationThread.js +++ b/src/lib/annotations/AnnotationThread.js @@ -58,7 +58,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons'; * @return {void} */ destroy() { - if (this.dialog) { + if (this.dialog && !this.isMobile) { this.unbindCustomListenersOnDialog(); this.dialog.destroy(); } @@ -204,6 +204,10 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons'; // If this annotation was the last one in the thread, destroy the thread } else if (this.annotations.length === 0 || annotatorUtil.isPlainHighlight(this.annotations)) { + if (this.isMobile) { + this.dialog.removeAnnotation(annotationID); + this.dialog.hideMobileDialog(); + } this.destroy(); // Otherwise, remove deleted annotation from dialog @@ -353,7 +357,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons'; } this.dialog.addListener('annotationcreate', this.createAnnotation); - this.dialog.addListener('annotationcancel', this.destroy); + this.dialog.addListener('annotationcancel', this.cancelUnsavedAnnotation); this.dialog.addListener('annotationdelete', this.deleteAnnotationWithID); } @@ -373,6 +377,23 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons'; this.dialog.removeAllListeners('annotationdelete'); } + /** + * Destroys mobile and pending/pending-active annotation threads + * + * @protected + * @return {void} + */ + cancelUnsavedAnnotation() { + if ( + !this.isMobile && + this.state !== constants.ANNOTATION_STATE_PENDING && + this.state !== constants.ANNOTATION_STATE_PENDING_ACTIVE + ) { + return; + } + this.destroy(); + } + //-------------------------------------------------------------------------- // Private //-------------------------------------------------------------------------- @@ -443,7 +464,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons'; * Creates a new point annotation * * @private - * @param data - Annotation data + * @param {Object} data - Annotation data * @return {void} */ createAnnotation(data) { @@ -454,7 +475,7 @@ import { ICON_PLACED_ANNOTATION } from '../icons/icons'; * Deletes annotation with annotationID from thread * * @private - * @param data - Annotation data + * @param {Object} data - Annotation data * @return {void} */ deleteAnnotationWithID(data) { diff --git a/src/lib/annotations/__tests__/AnnotationDialog-test.js b/src/lib/annotations/__tests__/AnnotationDialog-test.js index 6f5b1d990..328d22386 100644 --- a/src/lib/annotations/__tests__/AnnotationDialog-test.js +++ b/src/lib/annotations/__tests__/AnnotationDialog-test.js @@ -182,8 +182,14 @@ describe('lib/annotations/AnnotationDialog', () => { it('should hide and reset the mobile annotations dialog', () => { dialog.element = document.querySelector('.bp-mobile-annotation-dialog'); stubs.hide = sandbox.stub(annotatorUtil, 'hideElement'); + stubs.unbind = sandbox.stub(dialog, 'unbindDOMListeners'); + stubs.cancel = sandbox.stub(dialog, 'cancelAnnotation'); + dialog.hasAnnotations = true; + dialog.hideMobileDialog(); expect(stubs.hide).to.be.called; + expect(stubs.unbind).to.be.called; + expect(stubs.cancel).to.not.be.called; expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.false; }); @@ -192,6 +198,15 @@ describe('lib/annotations/AnnotationDialog', () => { dialog.hideMobileDialog(); expect(dialog.element.classList.contains(CLASS_ANIMATE_DIALOG)).to.be.false; }); + + it('should cancel unsaved annotations only if the dialog does not have annotations', () => { + dialog.element = document.querySelector('.bp-mobile-annotation-dialog'); + stubs.cancel = sandbox.stub(dialog, 'cancelAnnotation'); + dialog.hasAnnotations = false; + + dialog.hideMobileDialog(); + expect(stubs.cancel).to.be.called; + }); }); describe('hide()', () => { @@ -460,6 +475,7 @@ describe('lib/annotations/AnnotationDialog', () => { stubs.hideDelete = sandbox.stub(dialog, 'hideDeleteConfirmation'); stubs.delete = sandbox.stub(dialog, 'deleteAnnotation'); stubs.reply = sandbox.stub(dialog, 'postReply'); + stubs.hideMobile = sandbox.stub(dialog, 'hideMobileDialog'); }); it('should post annotation when post annotation button is clicked', () => { @@ -471,9 +487,21 @@ describe('lib/annotations/AnnotationDialog', () => { it('should cancel annotation when cancel annotation button is clicked', () => { stubs.findClosest.returns('cancel-annotation-btn'); + dialog.isMobile = false; dialog.clickHandler(stubs.event); expect(stubs.cancel).to.be.called; + expect(stubs.hideMobile).to.not.be.called; + expect(stubs.deactivate).to.be.calledWith(true); + }); + + it('should only hide the mobile dialog when the cancel annotation button is clicked on mobile', () => { + stubs.findClosest.returns('cancel-annotation-btn'); + dialog.isMobile = true; + + dialog.clickHandler(stubs.event); + expect(stubs.cancel).to.not.be.called; + expect(stubs.hideMobile).to.be.called; expect(stubs.deactivate).to.be.calledWith(true); }); diff --git a/src/lib/annotations/__tests__/AnnotationThread-test.js b/src/lib/annotations/__tests__/AnnotationThread-test.js index 968918827..a27a9ff6e 100644 --- a/src/lib/annotations/__tests__/AnnotationThread-test.js +++ b/src/lib/annotations/__tests__/AnnotationThread-test.js @@ -67,6 +67,17 @@ describe('lib/annotations/AnnotationThread', () => { expect(stubs.unbindDOM).to.be.called; expect(stubs.emit).to.be.calledWith('threaddeleted'); }); + + it('should not destroy the dialog on mobile', () => { + stubs.unbindCustom = sandbox.stub(thread, 'unbindCustomListenersOnDialog'); + stubs.destroyDialog = sandbox.stub(thread.dialog, 'destroy'); + thread.element = null; + thread.isMobile = true; + + thread.destroy(); + expect(stubs.unbindCustom).to.not.be.called; + expect(stubs.destroyDialog).to.not.be.called; + }); }); describe('hide()', () => { @@ -199,7 +210,8 @@ describe('lib/annotations/AnnotationThread', () => { removeAllListeners: () => {}, show: () => {}, hide: () => {}, - removeAnnotation: () => {} + removeAnnotation: () => {}, + hideMobileDialog: () => {} }; stubs.dialogMock = sandbox.mock(thread.dialog); @@ -210,10 +222,20 @@ describe('lib/annotations/AnnotationThread', () => { }); it('should destroy the thread if the deleted annotation was the last annotation in the thread', () => { + thread.isMobile = false; + stubs.dialogMock.expects('removeAnnotation').never(); + stubs.dialogMock.expects('hideMobileDialog').never(); thread.deleteAnnotation('someID', false); expect(stubs.destroy).to.be.called; }); + it('should destroy the thread and hide the mobile dialog if the deleted annotation was the last annotation in the thread on mobile', () => { + thread.isMobile = true; + stubs.dialogMock.expects('removeAnnotation'); + stubs.dialogMock.expects('hideMobileDialog'); + thread.deleteAnnotation('someID', false); + }); + it('should remove the relevant annotation from its dialog if the deleted annotation was not the last one', () => { // Add another annotation to thread so 'someID' isn't the only annotation thread.annotations.push(stubs.annotation2); @@ -437,6 +459,28 @@ describe('lib/annotations/AnnotationThread', () => { }); }); + describe('cancelUnsavedAnnotation()', () => { + it('should only destroy thread if on a mobile browser or in a pending/pending-active state', () => { + sandbox.stub(thread, 'destroy'); + + // mobile + thread.isMobile = true; + thread.cancelUnsavedAnnotation(); + expect(thread.destroy).to.be.called; + + // 'pending' state + thread.isMobile = false; + thread.state = constants.ANNOTATION_STATE_PENDING; + thread.cancelUnsavedAnnotation(); + expect(thread.destroy).to.be.called; + + // 'pending-active' state + thread.state = constants.ANNOTATION_STATE_PENDING_ACTIVE; + thread.cancelUnsavedAnnotation(); + expect(thread.destroy).to.be.called; + }); + }); + describe('createElement()', () => { it('should create an element with the right class and attribute', () => { const element = thread.createElement(); diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index 197be14a4..cf3ec0d36 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -58,7 +58,6 @@ const RESIZE_WAIT_TIME_IN_MILLIS = 300; /** * [constructor] * - * @param {HTMLElement} containerEl - The container * @param {Object} options - some options * @return {BaseViewer} Instance of base viewer */ @@ -583,7 +582,7 @@ const RESIZE_WAIT_TIME_IN_MILLIS = 300; const { file } = this.options; // Users can currently only view annotations on mobile - this.canAnnotate = checkPermission(file, PERMISSION_ANNOTATE) && !this.isMobile; + this.canAnnotate = checkPermission(file, PERMISSION_ANNOTATE); if (this.canAnnotate) { this.showAnnotateButton(this.getPointModeClickHandler()); } @@ -592,7 +591,7 @@ const RESIZE_WAIT_TIME_IN_MILLIS = 300; } /** - * Orient anntations to the correct scale and orientatio of the annotated document. + * Orient annotations to the correct scale and orientation of the annotated document. * @TODO(jholdstock|spramod): Remove this once we are emitting the correct messaging. * * @protected