diff --git a/src/i18n/en-US.properties b/src/i18n/en-US.properties index 951cf365a..22f978826 100644 --- a/src/i18n/en-US.properties +++ b/src/i18n/en-US.properties @@ -135,6 +135,12 @@ annotation_highlight_toggle=Highlight text annotation_highlight_comment=Add comment to highlighted text # Text for which user made the highlight annotation annotation_who_highlighted={1} highlighted +# Text for which user made the drawn annotation +annotation_who_drew={1} drew +# Accessibilty text for button that soft commits drawing +annotation_draw_save=Save drawing +# Accessibilty text for button that soft deletes drawing +annotation_draw_delete=Delete drawing # Text for when annotations fail to load annotations_load_error=We're sorry, annotations failed to load for this file. # Text for when the annotation can't be created diff --git a/src/lib/annotations/AnnotationModeController.js b/src/lib/annotations/AnnotationModeController.js index f644550c6..126a8a2ea 100644 --- a/src/lib/annotations/AnnotationModeController.js +++ b/src/lib/annotations/AnnotationModeController.js @@ -88,6 +88,13 @@ class AnnotationModeController extends EventEmitter { this.threads = this.threads.filter((item) => item !== thread); } + /** + * Clean up any selected annotations + * + * @return {void} + */ + removeSelection() {} + /** * Binds custom event listeners for a thread. * diff --git a/src/lib/annotations/Annotator.js b/src/lib/annotations/Annotator.js index 931dc93e6..74b2593d8 100644 --- a/src/lib/annotations/Annotator.js +++ b/src/lib/annotations/Annotator.js @@ -415,8 +415,6 @@ class Annotator extends EventEmitter { annotatorUtil.hideElement(postButtonEl); annotatorUtil.hideElement(undoButtonEl); annotatorUtil.hideElement(redoButtonEl); - annotatorUtil.disableElement(undoButtonEl); - annotatorUtil.disableElement(redoButtonEl); } } diff --git a/src/lib/annotations/__tests__/annotatorUtil-test.js b/src/lib/annotations/__tests__/annotatorUtil-test.js index d47b2ccc5..9bbe3ba7c 100644 --- a/src/lib/annotations/__tests__/annotatorUtil-test.js +++ b/src/lib/annotations/__tests__/annotatorUtil-test.js @@ -25,7 +25,8 @@ import { getHeaders, replacePlaceholders, createLocation, - round + round, + prevDefAndStopProp } from '../annotatorUtil'; import { STATES, @@ -36,6 +37,8 @@ import { const DIALOG_WIDTH = 81; +const sandbox = sinon.sandbox.create(); + describe('lib/annotations/annotatorUtil', () => { let childEl; let parentEl; @@ -52,6 +55,7 @@ describe('lib/annotations/annotatorUtil', () => { }); afterEach(() => { + sandbox.verifyAndRestore(); fixture.cleanup(); }); @@ -403,39 +407,54 @@ describe('lib/annotations/annotatorUtil', () => { }); describe('eventToLocationHandler()', () => { - it('should not call the callback when the location is valid', () => { - const annotator = { - isChanged: false - } - const getLocation = ((event) => 'location'); - const callback = ((location) => { - annotator.isChanged = true - }); - const locationHandler = eventToLocationHandler(getLocation, callback); + let getLocation; + let annotator; + let callback; + let locationHandler; + let event; + + beforeEach(() => { + getLocation = ((event) => 'location'); + callback = sandbox.stub(); + locationHandler = eventToLocationHandler(getLocation, callback); + event = { + preventDefault: () => {}, + stopPropagation: () => {} + }; + }); + it('should not call the callback when the location is valid', () => { locationHandler(undefined); - expect(annotator.isChanged).to.be.false; + expect(callback).to.not.be.called; }); it('should call the callback when the location is valid', () => { - const annotator = { - isChanged: false - } - const getLocation = ((event) => 'location'); - const callback = ((location) => { - annotator.isChanged = true - }); - const locationHandler = eventToLocationHandler(getLocation, callback); - const event = { - preventDefault: () => {}, - stopPropagation: () => {} - }; + locationHandler(event); + expect(callback).to.be.calledWith('location'); + }); + it('should do nothing when the target exists and it is not the textLayer', () => { + event.target = { + className: 'button' + }; locationHandler(event); - expect(annotator.isChanged).to.be.true; + expect(callback).to.not.be.called; }); }); + describe('prevDefAndStopProp()', () => { + it('should prevent default and stop propogation on an event', () => { + const event = { + preventDefault: sandbox.stub(), + stopPropagation: sandbox.stub() + }; + + prevDefAndStopProp(event); + expect(event.preventDefault).to.be.called; + expect(event.stopPropagation).to.be.called; + }) + }); + describe('createLocation()', () => { it('should create a location object without dimensions', () => { const location = createLocation(1,2, undefined); diff --git a/src/lib/annotations/annotationConstants.js b/src/lib/annotations/annotationConstants.js index 3e5c19425..4b140c90b 100644 --- a/src/lib/annotations/annotationConstants.js +++ b/src/lib/annotations/annotationConstants.js @@ -1,3 +1,5 @@ +export const USER_ANONYMOUS = 'Anonymous'; + export const CLASS_ACTIVE = 'bp-is-active'; export const CLASS_HIDDEN = 'bp-is-hidden'; export const CLASS_INVISIBLE = 'bp-is-invisible'; @@ -32,6 +34,9 @@ export const CLASS_ANNOTATION_BUTTON_DRAW_REDO = 'bp-btn-annotate-draw-redo'; export const CLASS_ANNOTATION_BUTTON_DRAW_POST = 'bp-btn-annotate-draw-post'; export const CLASS_ANNOTATION_BUTTON_DRAW_CANCEL = 'bp-btn-annotate-draw-cancel'; export const CLASS_ANNOTATION_BUTTON_DRAW_ENTER = 'bp-btn-annotate-draw-enter'; +export const CLASS_ANNOTATION_DRAWING_LABEL = 'bp-annotation-drawing-label'; +export const CLASS_ADD_DRAWING_BTN = 'bp-btn-annotate-draw-add'; +export const CLASS_DELETE_DRAWING_BTN = 'bp-btn-annotate-draw-delete'; export const DATA_TYPE_ANNOTATION_DIALOG = 'annotation-dialog'; export const DATA_TYPE_ANNOTATION_INDICATOR = 'annotation-indicator'; diff --git a/src/lib/annotations/annotatorUtil.js b/src/lib/annotations/annotatorUtil.js index d9984da68..b5cf46bb7 100644 --- a/src/lib/annotations/annotatorUtil.js +++ b/src/lib/annotations/annotatorUtil.js @@ -439,7 +439,7 @@ export function validateThreadParams(thread) { } /** - * Returns a function that passes a callback a location when given an event + * Returns a function that passes a callback a location when given an event on the document text layer * * @param {Function} locationFunction - The function to get a location from an event * @param {Function} callback - Callback to be called upon receiving an event @@ -448,7 +448,9 @@ export function validateThreadParams(thread) { export function eventToLocationHandler(locationFunction, callback) { return (event) => { const evt = event || window.event; - if (!evt) { + // Do nothing when the target isn't the text layer in case the text layer receives event precedence over buttons + // NOTE: Currently only applicable to documents. Need to find a better way to ensure button event precedence. + if (!evt || (event.target && event.target.className !== 'textLayer')) { return; } @@ -459,6 +461,20 @@ export function eventToLocationHandler(locationFunction, callback) { }; } +/** + * Call preventDefault and stopPropagation on an event + * + * @param {event} event - Event object to stop event bubbling + * @return {void} + */ +export function prevDefAndStopProp(event) { + if (!event) { + return; + } + + event.preventDefault(); + event.stopPropagation(); +} /** * Create a JSON object containing x/y coordinates and optionally dimensional information * diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index 754c32557..d74e27471 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -485,6 +485,8 @@ class DocAnnotator extends Annotator { this.highlightThrottleHandle = null; } + Object.values(this.modeControllers).forEach((controller) => controller.removeSelection()); + if (this.hasTouch && this.isMobile) { document.removeEventListener('selectionchange', this.onSelectionChange); this.annotatedElement.removeEventListener('touchstart', this.drawingSelectionHandler); diff --git a/src/lib/annotations/doc/DocDrawingDialog.js b/src/lib/annotations/doc/DocDrawingDialog.js index f2f6fb37d..940228c94 100644 --- a/src/lib/annotations/doc/DocDrawingDialog.js +++ b/src/lib/annotations/doc/DocDrawingDialog.js @@ -1,6 +1,8 @@ import AnnotationDialog from '../AnnotationDialog'; class DocDrawingDialog extends AnnotationDialog { + visible = false; + /** * Empty stub to avoid unexpected behavior. * @@ -36,6 +38,10 @@ class DocDrawingDialog extends AnnotationDialog { * @return {void} */ show() {} + + isVisible() { + return this.visible; + } } export default DocDrawingDialog; diff --git a/src/lib/annotations/doc/DocDrawingThread.js b/src/lib/annotations/doc/DocDrawingThread.js index 537ae6516..f402475ed 100644 --- a/src/lib/annotations/doc/DocDrawingThread.js +++ b/src/lib/annotations/doc/DocDrawingThread.js @@ -30,6 +30,7 @@ class DocDrawingThread extends DrawingThread { this.onPageChange = this.onPageChange.bind(this); this.reconstructBrowserCoordFromLocation = this.reconstructBrowserCoordFromLocation.bind(this); } + /** * Handle a pointer movement * @@ -84,6 +85,10 @@ class DocDrawingThread extends DrawingThread { this.pendingPath = new DrawingPath(); } + if (this.dialog && this.dialog.isVisible()) { + this.dialog.hide(); + } + // Start drawing rendering this.lastAnimationRequestId = window.requestAnimationFrame(this.render); } @@ -134,14 +139,10 @@ class DocDrawingThread extends DrawingThread { * @return {void} */ saveAnnotation(type, text) { - this.emit('annotationevent', { - type: 'drawcommit' - }); this.reset(); // Only make save request to server if there exist paths to save - const { undoCount } = this.pathContainer.getNumberOfItems(); - if (undoCount === 0) { + if (this.pathContainer.isUndoEmpty()) { return; } @@ -149,8 +150,7 @@ class DocDrawingThread extends DrawingThread { super.saveAnnotation(type, text); // Move the in-progress drawing to the concrete context - const inProgressCanvas = this.drawingContext.canvas; - this.drawingContext.clearRect(0, 0, inProgressCanvas.width, inProgressCanvas.height); + this.createDialog(); this.show(); } @@ -167,7 +167,7 @@ class DocDrawingThread extends DrawingThread { // Get the annotation layer context to draw with const context = this.selectContext(); - if (this.state === STATES.pending) { + if (this.dialog && this.dialog.isVisible()) { this.drawBoundary(); } @@ -183,6 +183,53 @@ class DocDrawingThread extends DrawingThread { this.draw(context, false); } + /** + * Binds custom event listeners for the dialog. + * + * @protected + * @return {void} + */ + bindCustomListenersOnDialog() { + if (!this.dialog) { + return; + } + + this.dialog.addListener('annotationcreate', () => { + this.emit('annotationevent', { + type: 'softcommit' + }); + }); + this.dialog.addListener('annotationdelete', () => { + this.emit('annotationevent', { + type: 'dialogdelete' + }); + }); + } + + /** + * Creates the document drawing annotation dialog for the thread. + * + * @protected + * @override + * @return {void} + */ + createDialog() { + if (this.dialog) { + this.dialog.destroy(); + } + + this.dialog = new DocDrawingDialog({ + annotatedElement: this.annotatedElement, + container: this.container, + annotations: this.annotations, + locale: this.locale, + location: this.location, + canAnnotate: this.annotationService.canAnnotate + }); + + this.bindCustomListenersOnDialog(); + } + /** * Prepare the pending drawing canvas if the scale factor has changed since the last render. Will do nothing if * the thread has not been assigned a page. @@ -215,7 +262,7 @@ class DocDrawingThread extends DrawingThread { onPageChange(location) { this.handleStop(); this.emit('annotationevent', { - type: 'pagechanged', + type: 'softcommit', location }); } @@ -277,23 +324,6 @@ class DocDrawingThread extends DrawingThread { return [x1, y1, width, height]; } - - /** - * Creates the document drawing annotation dialog for the thread. - * - * @override - * @return {void} - */ - createDialog() { - this.dialog = new DocDrawingDialog({ - annotatedElement: this.annotatedElement, - container: this.container, - annotations: this.annotations, - locale: this.locale, - location: this.location, - canAnnotate: this.annotationService.canAnnotate - }); - } } export default DocDrawingThread; diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js index 1a57cff05..ef2470d47 100644 --- a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js +++ b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js @@ -538,7 +538,7 @@ describe('lib/annotations/doc/DocAnnotator', () => { }); it('should bind DOM listeners if user can annotate', () => { - annotator.canAnnotate = true; + annotator.permissions.canAnnotate = true; stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func); stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func); @@ -573,7 +573,7 @@ describe('lib/annotations/doc/DocAnnotator', () => { }); it('should unbind DOM listeners if user can annotate', () => { - annotator.canAnnotate = true; + annotator.permissions.canAnnotate = true; stubs.elMock.expects('removeEventListener').withArgs('mouseup', sinon.match.func); stubs.elMock.expects('removeEventListener').withArgs('mousedown', sinon.match.func); @@ -586,7 +586,7 @@ describe('lib/annotations/doc/DocAnnotator', () => { it('should stop and destroy the requestAnimationFrame handle created by getHighlightMousemoveHandler()', () => { const rafHandle = 12; // RAF handles are integers - annotator.canAnnotate = true; + annotator.permissions.canAnnotate = true; annotator.highlightThrottleHandle = rafHandle; sandbox.stub(annotator, 'getHighlightMouseMoveHandler').returns(sandbox.stub()); @@ -609,6 +609,18 @@ describe('lib/annotations/doc/DocAnnotator', () => { expect(docStopListen).to.be.calledWith('selectionchange', sinon.match.func); expect(annotatedElementStopListen).to.be.calledWith('touchstart', sinon.match.func); }); + + it('should tell controllers to clean up selections', () => { + annotator.permissions.canAnnotate = true; + annotator.modeControllers = { + 'test': { + removeSelection: sandbox.stub() + } + }; + + annotator.unbindDOMListeners(); + expect(annotator.modeControllers['test'].removeSelection).to.be.called; + }); }); describe('bindCustomListenersOnThread()', () => { @@ -1299,7 +1311,8 @@ describe('lib/annotations/doc/DocAnnotator', () => { describe('drawingSelectionHandler()', () => { it('should use the controller to select with the event', () => { const drawController = { - handleSelection: sandbox.stub() + handleSelection: sandbox.stub(), + removeSelection: sandbox.stub() }; annotator.modeControllers = { [TYPES.draw]: drawController diff --git a/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js b/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js index f465dedfa..04660f2f0 100644 --- a/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js +++ b/src/lib/annotations/doc/__tests__/DocDrawingThread-test.js @@ -1,6 +1,7 @@ import * as docAnnotatorUtil from '../docAnnotatorUtil'; import * as annotatorUtil from '../../annotatorUtil'; import DocDrawingThread from '../DocDrawingThread'; +import DocDrawingDialog from '../DocDrawingDialog'; import AnnotationThread from '../../AnnotationThread'; import DrawingPath from '../../drawing/DrawingPath'; import { @@ -113,6 +114,7 @@ describe('lib/annotations/doc/DocDrawingThread', () => { expect(docDrawingThread.onPageChange).to.be.called; expect(docDrawingThread.checkAndHandleScaleUpdate).to.not.be.called; }); + }); describe('handleStop()', () => { @@ -145,7 +147,8 @@ describe('lib/annotations/doc/DocDrawingThread', () => { docDrawingThread.dialog = { value: 'non-empty', removeAllListeners: () => {}, - destroy: () => {} + destroy: () => {}, + isVisible: () => false } docDrawingThread.handleStop(); @@ -158,7 +161,7 @@ describe('lib/annotations/doc/DocDrawingThread', () => { sandbox.stub(docDrawingThread, 'handleStop'); docDrawingThread.addListener('annotationevent', (data) => { expect(docDrawingThread.handleStop).to.be.called; - expect(data.type).to.equal('pagechanged'); + expect(data.type).to.equal('softcommit'); done(); }); @@ -221,6 +224,7 @@ describe('lib/annotations/doc/DocDrawingThread', () => { beforeEach(() => { stubs.setBoundary = sandbox.stub(docDrawingThread, 'setBoundary'); stubs.show = sandbox.stub(docDrawingThread, 'show'); + stubs.createDialog = sandbox.stub(docDrawingThread, 'createDialog'); Object.defineProperty(AnnotationThread.prototype, 'saveAnnotation', { value: sandbox.stub() }); }); @@ -230,20 +234,14 @@ describe('lib/annotations/doc/DocDrawingThread', () => { it('should clean up without committing when there are no paths to be saved', () => { sandbox.stub(docDrawingThread, 'reset'); - sandbox.stub(docDrawingThread, 'emit'); - sandbox.stub(docDrawingThread.pathContainer, 'getNumberOfItems').returns({ - undoCount: 0, - redoCount: 1 - }); + sandbox.stub(docDrawingThread.pathContainer, 'isUndoEmpty').returns(true); docDrawingThread.saveAnnotation('draw'); - expect(docDrawingThread.pathContainer.getNumberOfItems).to.be.called; + expect(docDrawingThread.pathContainer.isUndoEmpty).to.be.called; expect(AnnotationThread.prototype.saveAnnotation).to.not.be.called; expect(docDrawingThread.reset).to.be.called; - expect(docDrawingThread.emit).to.be.calledWith('annotationevent', { - type: 'drawcommit' - }); expect(stubs.show).to.not.be.called; + expect(stubs.createDialog).to.not.be.called; }); it('should clean up and commit in-progress drawings when there are paths to be saved', () => { @@ -259,17 +257,15 @@ describe('lib/annotations/doc/DocDrawingThread', () => { clearRect: sandbox.stub() }; - sandbox.stub(docDrawingThread.pathContainer, 'getNumberOfItems').returns({ - undoCount: 1, - redoCount: 0 - }); + sandbox.stub(docDrawingThread.pathContainer, 'isUndoEmpty').returns(false); docDrawingThread.saveAnnotation('draw'); - expect(stubs.show).to.be.called; - expect(stubs.setBoundary).to.be.called; - expect(docDrawingThread.pathContainer.getNumberOfItems).to.be.called; + expect(docDrawingThread.pathContainer.isUndoEmpty).to.be.called; expect(docDrawingThread.drawingContext.clearRect).to.be.called; expect(AnnotationThread.prototype.saveAnnotation).to.be.called; + expect(stubs.show).to.be.called; + expect(stubs.setBoundary).to.be.called; + expect(stubs.createDialog).to.be.called; }); }); @@ -317,31 +313,43 @@ describe('lib/annotations/doc/DocDrawingThread', () => { docDrawingThread.pathContainer = { applyToItems: sandbox.stub() }; + + docDrawingThread.annotatedElement = 'annotatedEl'; + docDrawingThread.location = 'loc'; }); it('should do nothing when no element is assigned to the DocDrawingThread', () => { docDrawingThread.annotatedElement = undefined; - docDrawingThread.location = 'loc'; docDrawingThread.show(); expect(docDrawingThread.selectContext).to.not.be.called; }); it('should do nothing when no location is assigned to the DocDrawingThread', () => { - docDrawingThread.annotatedElement = 'annotatedEl'; docDrawingThread.location = undefined; docDrawingThread.show(); expect(docDrawingThread.selectContext).to.not.be.called; }); it('should draw the paths in the thread', () => { - docDrawingThread.annotatedElement = 'annotatedEl'; - docDrawingThread.location = 'loc'; docDrawingThread.state = 'not pending'; - - docDrawingThread.show() + docDrawingThread.show(); expect(docDrawingThread.selectContext).to.be.called; expect(docDrawingThread.draw).to.be.called; }); + + it('should draw the boundary when a dialog exists and is visible', () => { + sandbox.stub(docDrawingThread, 'drawBoundary'); + docDrawingThread.dialog = { + isVisible: sandbox.stub().returns(true), + destroy: () => {}, + removeAllListeners: () => {}, + hide: () => {} + } + + docDrawingThread.show(); + expect(docDrawingThread.dialog.isVisible).to.be.called; + expect(docDrawingThread.drawBoundary).to.be.called; + }) }); describe('selectContext()', () => { @@ -412,4 +420,40 @@ describe('lib/annotations/doc/DocDrawingThread', () => { expect(value).to.deep.equal([5, 5, 45, 40]); }); }); + + describe('createDialog()', () => { + it('should create a new doc drawing dialog', () => { + const existingDialog = { + destroy: sandbox.stub() + }; + + sandbox.stub(docDrawingThread, 'bindCustomListenersOnDialog'); + docDrawingThread.dialog = existingDialog; + docDrawingThread.annotationService = { + canAnnotate: true + }; + + docDrawingThread.createDialog(); + + expect(existingDialog.destroy).to.be.called; + expect(docDrawingThread.dialog instanceof DocDrawingDialog).to.be.truthy; + expect(docDrawingThread.bindCustomListenersOnDialog).to.be.called; + }); + }); + + describe('bindCustomListenersOnDialog', () => { + it('should bind listeners on the dialog', () => { + docDrawingThread.dialog = { + addListener: sandbox.stub(), + removeAllListeners: sandbox.stub(), + hide: sandbox.stub(), + destroy: sandbox.stub(), + isVisible: sandbox.stub() + }; + + docDrawingThread.bindCustomListenersOnDialog(); + expect(docDrawingThread.dialog.addListener).to.be.calledWith('annotationcreate', sinon.match.func); + expect(docDrawingThread.dialog.addListener).to.be.calledWith('annotationdelete', sinon.match.func); + }); + }); }); diff --git a/src/lib/annotations/drawing/DrawingContainer.js b/src/lib/annotations/drawing/DrawingContainer.js index 965a90398..55400db14 100644 --- a/src/lib/annotations/drawing/DrawingContainer.js +++ b/src/lib/annotations/drawing/DrawingContainer.js @@ -36,7 +36,7 @@ class DrawingContainer { * @return {boolean} Whether or not an undo was done. */ undo() { - if (this.undoStack.length === 0) { + if (this.isUndoEmpty()) { return false; } @@ -60,6 +60,15 @@ class DrawingContainer { return true; } + /** + * Return whether or not there are objects that can be undone + * + * @return {boolean} Whether or not there exists committed items + */ + isUndoEmpty() { + return this.undoStack.length === 0; + } + /** * Retrieve a JSON blob containing the number of undo and redo in each stack. * diff --git a/src/lib/annotations/drawing/DrawingModeController.js b/src/lib/annotations/drawing/DrawingModeController.js index 1f2a57623..b5b69b596 100644 --- a/src/lib/annotations/drawing/DrawingModeController.js +++ b/src/lib/annotations/drawing/DrawingModeController.js @@ -3,6 +3,7 @@ import AnnotationModeController from '../AnnotationModeController'; import * as annotatorUtil from '../annotatorUtil'; import { TYPES, + STATES, SELECTOR_ANNOTATION_BUTTON_DRAW_POST, SELECTOR_ANNOTATION_BUTTON_DRAW_UNDO, SELECTOR_ANNOTATION_BUTTON_DRAW_REDO, @@ -95,6 +96,34 @@ class DrawingModeController extends AnnotationModeController { thread.addListener('threaddeleted', () => this.unregisterThread(thread)); } + /** + * Unbind drawing mode listeners. Resets the undo and redo buttons to be disabled if they exist + * + * @inheritdoc + * @protected + * @return {void} + */ + unbindModeListeners() { + super.unbindModeListeners(); + + annotatorUtil.disableElement(this.undoButtonEl); + annotatorUtil.disableElement(this.redoButtonEl); + } + + /** + * Deselect a saved and selected thread + * + * @return {void} + */ + removeSelection() { + if (!this.selectedThread) { + return; + } + + this.selectedThread.clearBoundary(); + this.selectedThread = undefined; + } + /** * Set up and return the necessary handlers for the annotation mode * @@ -151,19 +180,33 @@ class DrawingModeController extends AnnotationModeController { // Register the thread to the threadmap when a starting location is assigned. Should only occur once. this.annotator.addThreadToMap(thread); break; - case 'drawcommit': - // Upon a commit, remove the listeners on the thread. - // Adding the thread to the Rbush only happens upon a successful save - thread.removeAllListeners('annotationevent'); - break; - case 'pagechanged': - // On page change, save the original thread, create a new thread and + case 'softcommit': + // Save the original thread, create a new thread and // start drawing at the location indicating the page change this.currentThread = undefined; thread.saveAnnotation(TYPES.draw); this.unbindModeListeners(); - this.bindModeListeners(TYPES.draw); - this.currentThread.handleStart(data.location); + this.bindModeListeners(); + + // Given a location (page change) start drawing at the provided location + if (data.location) { + this.currentThread.handleStart(data.location); + } + + break; + case 'dialogdelete': + if (thread.state === STATES.pending) { + // Soft delete, in-progress thread doesn't require a redraw or a delete on the server + // Clear in-progress thread and restart drawing + thread.destroy(); + this.unbindModeListeners(); + this.bindModeListeners(); + } else { + // Redraw any threads that the deleted thread could have been overlapping + thread.deleteThread(); + this.threads.search(thread).forEach((drawingThread) => drawingThread.show()); + } + break; case 'availableactions': this.updateUndoRedoButtonEls(data.undo, data.redo); @@ -202,10 +245,7 @@ class DrawingModeController extends AnnotationModeController { .filter((drawingThread) => drawingThread.location.page === location.page); // Clear boundary on previously selected thread - if (this.selectedThread) { - const canvas = this.selectedThread.drawingContext.canvas; - this.selectedThread.drawingContext.clearRect(0, 0, canvas.width, canvas.height); - } + this.removeSelection(); // Selected a region with no drawing threads, remove the reference to the previously selected thread if (intersectingThreads.length === 0) { @@ -213,7 +253,7 @@ class DrawingModeController extends AnnotationModeController { return; } - // Randomly select a thread in case there are multiple + // Randomly select a thread in case there are multiple overlapping threads (use canvas hitmap to avoid this) const index = Math.floor(Math.random() * intersectingThreads.length); const selected = intersectingThreads[index]; this.select(selected); @@ -227,20 +267,8 @@ class DrawingModeController extends AnnotationModeController { * @return {void} */ select(selectedDrawingThread) { - if (this.selectedThread && this.selectedThread === selectedDrawingThread) { - // Selected the same thread twice, delete the thread - const toDelete = this.selectedThread; - toDelete.deleteThread(); - - // Redraw any threads that the deleted thread could have been covering - const toRedraw = this.threads.search(toDelete); - toRedraw.forEach((drawingThread) => drawingThread.show()); - this.selectedThread = undefined; - } else { - // Selected the thread for the first time, select the thread (TODO @minhnguyen: show UI on select) - selectedDrawingThread.drawBoundary(); - this.selectedThread = selectedDrawingThread; - } + selectedDrawingThread.drawBoundary(); + this.selectedThread = selectedDrawingThread; } /** diff --git a/src/lib/annotations/drawing/DrawingThread.js b/src/lib/annotations/drawing/DrawingThread.js index f7272bfd3..2b220bcda 100644 --- a/src/lib/annotations/drawing/DrawingThread.js +++ b/src/lib/annotations/drawing/DrawingThread.js @@ -90,9 +90,9 @@ class DrawingThread extends AnnotationThread { window.cancelAnimationFrame(this.lastAnimationRequestId); } - if (this.drawingContext) { - const canvas = this.drawingContext.canvas; - this.drawingContext.clearRect(0, 0, canvas.width, canvas.height); + if (this.dialog) { + this.dialog.destroy(); + this.dialog = null; } super.destroy(); @@ -100,6 +100,11 @@ class DrawingThread extends AnnotationThread { this.emit('threadcleanup'); } + reset() { + super.reset(); + + this.clearBoundary(); + } /* eslint-disable no-unused-vars */ /** * Handle a pointer movement @@ -140,7 +145,7 @@ class DrawingThread extends AnnotationThread { // Calculate the bounding rectangle const [x, y, width, height] = this.getBrowserRectangularBoundary(); - // Clear the drawn thread and destroy it + // Clear the drawn thread and its boundary this.concreteContext.clearRect( x - DRAW_BORDER_OFFSET, y + DRAW_BORDER_OFFSET, @@ -148,8 +153,7 @@ class DrawingThread extends AnnotationThread { height - DRAW_BORDER_OFFSET * 2 ); - // Notifies that the thread was destroyed so that observers can react accordingly - this.destroy(); + this.clearBoundary(); } /** @@ -307,6 +311,14 @@ class DrawingThread extends AnnotationThread { // Restore context style this.drawingContext.restore(); + + if (this.dialog) { + if (!this.dialog.isVisible() && !this.pathContainer.isUndoEmpty()) { + this.showDialog(); + } + + this.dialog.position(x + width, y); + } } /** @@ -372,6 +384,10 @@ class DrawingThread extends AnnotationThread { this.maxX = boundaryData.maxX; this.minY = boundaryData.minY; this.maxY = boundaryData.maxY; + + if (this.dialog && this.pathContainer.isUndoEmpty()) { + this.dialog.hide(); + } } /** @@ -382,6 +398,24 @@ class DrawingThread extends AnnotationThread { * @return {void} */ getBrowserRectangularBoundary() {} + + /** + * Clear any drawn boundary and associated dialog + * + * @return {void} + */ + clearBoundary() { + if (this.drawingContext) { + const canvas = this.drawingContext.canvas; + this.drawingContext.clearRect(0, 0, canvas.width, canvas.height); + } + + if (!this.dialog || !this.dialog.isVisible()) { + return; + } + + this.dialog.hide(); + } } export default DrawingThread; diff --git a/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js b/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js index dd297ab64..2dc79040c 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingContainer-test.js @@ -167,4 +167,16 @@ describe('lib/annotations/drawing/DrawingContainer', () => { }); }); }); + + describe('isUndoEmpty()', () => { + it('should return true when no items are in the undoStack', () => { + drawingContainer.undoStack = []; + expect(drawingContainer.isUndoEmpty()).to.be.truthy; + }); + + it('should return false when there are items are in the undoStack', () => { + drawingContainer.undoStack = ['one']; + expect(drawingContainer.isUndoEmpty()).to.be.falsy; + }); + }); }); diff --git a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js index 886f447f7..62e98f64f 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingModeController-test.js @@ -146,6 +146,18 @@ describe('lib/annotations/drawing/DrawingModeController', () => { }); }); + describe('unbindModeListeners()', () => { + it('should disable undo and redo buttons', () => { + sandbox.stub(annotatorUtil, 'disableElement'); + + drawingModeController.undoButtonEl = 'test1'; + drawingModeController.redoButtonEl = 'test2'; + drawingModeController.unbindModeListeners(); + expect(annotatorUtil.disableElement).to.be.calledWith(drawingModeController.undoButtonEl); + expect(annotatorUtil.disableElement).to.be.calledWith(drawingModeController.redoButtonEl); + }); + }); + describe('handleAnnotationEvent()', () => { it('should add thread to map on locationassigned', () => { const thread = 'obj'; @@ -159,15 +171,21 @@ describe('lib/annotations/drawing/DrawingModeController', () => { expect(drawingModeController.annotator.addThreadToMap).to.be.called; }); - it('should remove annotationevent listeners from the thread on drawcommit', () => { + it('should restart mode listeners from the thread on softcommit', () => { const thread = { - removeAllListeners: sandbox.stub() + saveAnnotation: sandbox.stub(), + handleStart: sandbox.stub() }; + sandbox.stub(drawingModeController, 'unbindModeListeners'); + sandbox.stub(drawingModeController, 'bindModeListeners'); drawingModeController.handleAnnotationEvent(thread, { - type: 'drawcommit' + type: 'softcommit' }); - expect(thread.removeAllListeners).to.be.calledWith('annotationevent'); + expect(drawingModeController.unbindModeListeners).to.be.called; + expect(drawingModeController.bindModeListeners).to.be.called; + expect(thread.saveAnnotation).to.be.called; + expect(thread.handleStart).to.not.be.called; }); it('should start a new thread on pagechanged', () => { @@ -178,7 +196,7 @@ describe('lib/annotations/drawing/DrawingModeController', () => { handleStart: sandbox.stub() }; const data = { - type: 'pagechanged', + type: 'softcommit', location: 'not empty' }; sandbox.stub(drawingModeController, 'unbindModeListeners'); @@ -204,6 +222,38 @@ describe('lib/annotations/drawing/DrawingModeController', () => { }); expect(drawingModeController.updateUndoRedoButtonEls).to.be.calledWith(1, 2); }); + + it('should soft delete a pending thread and restart mode listeners', () => { + const thread = { + state: 'pending', + destroy: sandbox.stub() + }; + + sandbox.stub(drawingModeController, 'unbindModeListeners'); + sandbox.stub(drawingModeController, 'bindModeListeners'); + drawingModeController.handleAnnotationEvent(thread, { + type: 'dialogdelete' + }); + expect(thread.destroy).to.be.called; + expect(drawingModeController.unbindModeListeners).to.be.called; + expect(drawingModeController.bindModeListeners).to.be.called; + }); + + it('should delete a non-pending thread', () => { + const thread = { + state: 'idle', + deleteThread: sandbox.stub() + }; + drawingModeController.threads = { + search: sandbox.stub().returns([]) + }; + + drawingModeController.handleAnnotationEvent(thread, { + type: 'dialogdelete' + }); + expect(thread.deleteThread).to.be.called; + expect(drawingModeController.threads.search).to.be.called; + }); }); describe('handleSelection()', () => { @@ -221,6 +271,7 @@ describe('lib/annotations/drawing/DrawingModeController', () => { it('should call select on an thread found in the data store', () => { stubs.select = sandbox.stub(drawingModeController, 'select'); + stubs.clean = sandbox.stub(drawingModeController, 'removeSelection'); stubs.getLoc.returns({ x: 5, y: 5 @@ -237,10 +288,24 @@ describe('lib/annotations/drawing/DrawingModeController', () => { drawingModeController.handleSelection('event'); expect(drawingModeController.threads.search).to.be.called; expect(filterObjects.filter).to.be.called; + expect(stubs.clean).to.be.called; expect(stubs.select).to.be.calledWith(filteredObject); }); }); + describe('removeSelection()', () => { + it('should clean a selected thread boundary', () => { + const thread = { + clearBoundary: sandbox.stub() + }; + drawingModeController.selectedThread = thread; + + drawingModeController.removeSelection(); + expect(thread.clearBoundary).to.be.called; + expect(drawingModeController.selectedThread).to.be.undefined; + }); + }); + describe('select()', () => { it('should draw the boundary', () => { const thread = { diff --git a/src/lib/annotations/drawing/__tests__/DrawingThread-test.js b/src/lib/annotations/drawing/__tests__/DrawingThread-test.js index c98ab687b..cd5df6156 100644 --- a/src/lib/annotations/drawing/__tests__/DrawingThread-test.js +++ b/src/lib/annotations/drawing/__tests__/DrawingThread-test.js @@ -58,26 +58,33 @@ describe('lib/annotations/drawing/DrawingThread', () => { expect(window.cancelAnimationFrame).to.be.calledWith(1); expect(drawingThread.reset).to.be.called; - expect(drawingThread.drawingContext.clearRect).to.be.called; }) }); + describe('reset()', () => { + it('should clear the boundary', () => { + sandbox.stub(drawingThread, 'clearBoundary'); + drawingThread.reset(); + expect(drawingThread.clearBoundary).to.be.called; + }); + }) + describe('deleteThread()', () => { it('should delete all attached annotations, clear the drawn rectangle, and call destroy', () => { sandbox.stub(drawingThread, 'getBrowserRectangularBoundary').returns(['a', 'b', 'c', 'd']); - sandbox.stub(drawingThread, 'destroy'); + sandbox.stub(drawingThread, 'deleteAnnotationWithID'); + sandbox.stub(drawingThread, 'clearBoundary'); drawingThread.concreteContext = { clearRect: sandbox.stub() }; - drawingThread.annotations = { - forEach: sandbox.stub() - }; + drawingThread.annotations = ['annotation']; + drawingThread.deleteThread(); expect(drawingThread.getBrowserRectangularBoundary).to.be.called; expect(drawingThread.concreteContext.clearRect).to.be.called; - expect(drawingThread.annotations.forEach).to.be.called; - expect(drawingThread.destroy).to.be.called; + expect(drawingThread.clearBoundary).to.be.called; + expect(drawingThread.deleteAnnotationWithID).to.be.calledWith('annotation'); }); }); @@ -340,4 +347,28 @@ describe('lib/annotations/drawing/DrawingThread', () => { expect(drawingThread.maxY).to.equal(drawingThread.location.maxY); }); }); + + describe('clearBoundary()', () => { + it('should clear the drawing context and hide any dialog', () => { + drawingThread.drawingContext = { + canvas: { + width: 100, + height: 100 + }, + clearRect: sandbox.stub() + }; + + drawingThread.dialog = { + isVisible: sandbox.stub().returns(true), + hide: sandbox.stub(), + destroy: () => {}, + removeAllListeners: () => {} + }; + + drawingThread.clearBoundary(); + expect(drawingThread.drawingContext.clearRect).to.be.called; + expect(drawingThread.dialog.isVisible).to.be.called; + expect(drawingThread.dialog.hide).to.be.called; + }); + }); }); diff --git a/src/lib/icons/draw_delete.svg b/src/lib/icons/draw_delete.svg new file mode 100644 index 000000000..bb4bfdd3a --- /dev/null +++ b/src/lib/icons/draw_delete.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/src/lib/icons/draw_save.svg b/src/lib/icons/draw_save.svg new file mode 100644 index 000000000..3fd90af6c --- /dev/null +++ b/src/lib/icons/draw_save.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/src/lib/icons/icons.js b/src/lib/icons/icons.js index cc88816de..c969c9bc6 100644 --- a/src/lib/icons/icons.js +++ b/src/lib/icons/icons.js @@ -40,6 +40,8 @@ import FIND_DROP_UP from './arrow_drop_up.svg'; import CLOSE from './close.svg'; import SEARCH from './search.svg'; import PRINT_CHECKMARK from './print_checkmark.svg'; +import DRAW_SAVE from './draw_save.svg'; +import DRAW_DELETE from './draw_delete.svg'; export const ICON_ANNOTATION = ANNOTATION; export const ICON_HIGHLIGHT_COMMENT = ANNOTATION_HIGHLIGHT_COMMENT; @@ -83,3 +85,5 @@ export const ICON_FIND_DROP_UP = FIND_DROP_UP; export const ICON_CLOSE = CLOSE; export const ICON_SEARCH = SEARCH; export const ICON_PRINT_CHECKMARK = PRINT_CHECKMARK; +export const ICON_DRAW_SAVE = DRAW_SAVE; +export const ICON_DRAW_DELETE = DRAW_DELETE;