From 2e9922014483ba69226aa644f79b9f8208717b8f Mon Sep 17 00:00:00 2001 From: Jared Stoffan Date: Wed, 1 Jul 2020 11:17:52 -0700 Subject: [PATCH] fix(image): Disallow creation of annotations on rotated images (#1232) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- src/lib/AnnotationControls.ts | 58 ++++++-------------- src/lib/__tests__/AnnotationControls-test.js | 33 ++++------- src/lib/viewers/BaseViewer.js | 23 ++++++-- src/lib/viewers/__tests__/BaseViewer-test.js | 54 ++++++++++++++++-- src/lib/viewers/image/ImageViewer.js | 7 +++ 5 files changed, 105 insertions(+), 70 deletions(-) diff --git a/src/lib/AnnotationControls.ts b/src/lib/AnnotationControls.ts index d3ce62fd5..009eb719e 100644 --- a/src/lib/AnnotationControls.ts +++ b/src/lib/AnnotationControls.ts @@ -2,7 +2,6 @@ import noop from 'lodash/noop'; import { ICON_REGION_COMMENT } from './icons/icons'; import Controls from './Controls'; -import fullscreen from './Fullscreen'; export const CLASS_ANNOTATIONS_GROUP = 'bp-AnnotationControls-group'; export const CLASS_REGION_BUTTON = 'bp-AnnotationControls-regionBtn'; @@ -53,8 +52,6 @@ export default class AnnotationControls { this.controlsMap = { [AnnotationMode.REGION]: this.updateRegionButton, }; - - this.attachEventHandlers(); } /** @@ -64,48 +61,12 @@ export default class AnnotationControls { if (!this.hasInit) { return; } - fullscreen.removeListener('enter', this.handleFullscreenEnter); - fullscreen.removeListener('exit', this.handleFullscreenExit); + document.removeEventListener('keydown', this.handleKeyDown); this.hasInit = false; } - /** - * Attaches event handlers - */ - private attachEventHandlers(): void { - fullscreen.addListener('enter', this.handleFullscreenEnter); - fullscreen.addListener('exit', this.handleFullscreenExit); - } - - /** - * Handle fullscreen change - */ - private handleFullscreenChange = (isFullscreen: boolean): void => { - const groupElement = this.controlsElement.querySelector(`.${CLASS_ANNOTATIONS_GROUP}`); - - if (!groupElement) { - return; - } - - if (isFullscreen) { - groupElement.classList.add(CLASS_GROUP_HIDE); - } else { - groupElement.classList.remove(CLASS_GROUP_HIDE); - } - }; - - /** - * Enter fullscreen handler - */ - private handleFullscreenEnter = (): void => this.handleFullscreenChange(true); - - /** - * Exit fullscreen handler - */ - private handleFullscreenExit = (): void => this.handleFullscreenChange(false); - /** * Deactivate current control button */ @@ -120,6 +81,23 @@ export default class AnnotationControls { updateButton(); }; + /** + * Show or hide the controls + */ + public toggle(show: boolean): void { + const groupElement = this.controlsElement.querySelector(`.${CLASS_ANNOTATIONS_GROUP}`); + + if (!groupElement) { + return; + } + + if (show) { + groupElement.classList.remove(CLASS_GROUP_HIDE); + } else { + groupElement.classList.add(CLASS_GROUP_HIDE); + } + } + /** * Update region button UI */ diff --git a/src/lib/__tests__/AnnotationControls-test.js b/src/lib/__tests__/AnnotationControls-test.js index 75fc87bdd..862432dc7 100644 --- a/src/lib/__tests__/AnnotationControls-test.js +++ b/src/lib/__tests__/AnnotationControls-test.js @@ -8,7 +8,6 @@ import AnnotationControls, { CLASS_REGION_BUTTON, } from '../AnnotationControls'; import Controls from '../Controls'; -import fullscreen from '../Fullscreen'; let annotationControls; let stubs = {}; @@ -24,8 +23,6 @@ describe('lib/AnnotationControls', () => { fixture.load('__tests__/AnnotationControls-test.html'); stubs.classListAdd = sandbox.stub(); stubs.classListRemove = sandbox.stub(); - stubs.fullscreenAddListener = sandbox.stub(fullscreen, 'addListener'); - stubs.fullscreenRemoveListener = sandbox.stub(fullscreen, 'removeListener'); stubs.onRegionClick = sandbox.stub(); stubs.querySelector = sandbox.stub().returns({ classList: { @@ -55,10 +52,6 @@ describe('lib/AnnotationControls', () => { expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE); }); - it('should attach event listeners', () => { - expect(stubs.fullscreenAddListener).to.be.calledTwice; - }); - it('should throw an exception if controls is not provided', () => { expect(() => new AnnotationControls()).to.throw(Error, 'controls must be an instance of Controls'); }); @@ -71,7 +64,6 @@ describe('lib/AnnotationControls', () => { annotationControls.destroy(); - expect(stubs.fullscreenRemoveListener).to.be.calledTwice; expect(document.removeEventListener).to.be.calledWith('keydown', annotationControls.handleKeyDown); expect(annotationControls.hasInit).to.equal(false); }); @@ -81,7 +73,6 @@ describe('lib/AnnotationControls', () => { annotationControls.destroy(); - expect(stubs.fullscreenRemoveListener).not.to.be.called; expect(document.removeEventListener).not.to.be.called; }); }); @@ -197,18 +188,6 @@ describe('lib/AnnotationControls', () => { }); }); - describe('handleFullscreenChange()', () => { - it('should hide entire group if fullscreen is active', () => { - annotationControls.handleFullscreenEnter(); - expect(stubs.querySelector).to.be.calledWith(`.${CLASS_ANNOTATIONS_GROUP}`); - expect(stubs.classListAdd).to.be.calledWith(CLASS_GROUP_HIDE); - - annotationControls.handleFullscreenExit(); - expect(stubs.querySelector).to.be.calledWith(`.${CLASS_ANNOTATIONS_GROUP}`); - expect(stubs.classListRemove).to.be.calledWith(CLASS_GROUP_HIDE); - }); - }); - describe('resetControls()', () => { beforeEach(() => { stubs.updateRegionButton = sandbox.stub(); @@ -234,4 +213,16 @@ describe('lib/AnnotationControls', () => { expect(stubs.updateRegionButton).to.be.called; }); }); + + describe('toggle()', () => { + it('should show or hide the entire button group', () => { + annotationControls.toggle(false); + expect(stubs.querySelector).to.be.calledWith(`.${CLASS_ANNOTATIONS_GROUP}`); + expect(stubs.classListAdd).to.be.calledWith(CLASS_GROUP_HIDE); + + annotationControls.toggle(true); + expect(stubs.querySelector).to.be.calledWith(`.${CLASS_ANNOTATIONS_GROUP}`); + expect(stubs.classListRemove).to.be.calledWith(CLASS_GROUP_HIDE); + }); + }); }); diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index 33f0ca90e..ccad1bc5c 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -559,11 +559,10 @@ class BaseViewer extends EventEmitter { handleFullscreenEnter() { this.resize(); - if (this.areNewAnnotationsEnabled() && this.annotator && this.annotationControls) { + if (this.annotator && this.areNewAnnotationsEnabled()) { this.annotator.emit(ANNOTATOR_EVENT.setVisibility, false); - this.annotator.toggleAnnotationMode(AnnotationMode.NONE); - this.annotationControls.resetControls(); + this.disableAnnotationControls(); } } @@ -574,8 +573,10 @@ class BaseViewer extends EventEmitter { */ handleFullscreenExit() { this.resize(); - if (this.areNewAnnotationsEnabled() && this.annotator) { + + if (this.annotator && this.areNewAnnotationsEnabled()) { this.annotator.emit(ANNOTATOR_EVENT.setVisibility, true); + this.enableAnnotationControls(); } } @@ -901,6 +902,20 @@ class BaseViewer extends EventEmitter { // Annotations //-------------------------------------------------------------------------- + disableAnnotationControls() { + if (this.annotator && this.annotationControls && this.areNewAnnotationsEnabled()) { + this.annotator.toggleAnnotationMode(AnnotationMode.NONE); + this.annotationControls.resetControls(); + this.annotationControls.toggle(false); + } + } + + enableAnnotationControls() { + if (this.annotationControls && this.areNewAnnotationsEnabled()) { + this.annotationControls.toggle(true); + } + } + /** * Loads the BoxAnnotations static assets * diff --git a/src/lib/viewers/__tests__/BaseViewer-test.js b/src/lib/viewers/__tests__/BaseViewer-test.js index b82da9635..ce4438af5 100644 --- a/src/lib/viewers/__tests__/BaseViewer-test.js +++ b/src/lib/viewers/__tests__/BaseViewer-test.js @@ -536,36 +536,47 @@ describe('lib/viewers/BaseViewer', () => { }); it('should hide annotations and toggle annotations mode', () => { + sandbox.stub(base, 'areNewAnnotationsEnabled').returns(true); + sandbox.stub(base, 'disableAnnotationControls'); base.annotator = { emit: sandbox.mock(), toggleAnnotationMode: sandbox.mock(), }; base.annotationControls = { destroy: sandbox.mock(), - resetControls: sandbox.mock(), }; - sandbox.stub(base, 'areNewAnnotationsEnabled').returns(true); base.handleFullscreenEnter(); expect(base.annotator.emit).to.be.calledWith(ANNOTATOR_EVENT.setVisibility, false); expect(base.annotator.toggleAnnotationMode).to.be.calledWith(AnnotationMode.NONE); - expect(base.annotationControls.resetControls).to.be.called; + expect(base.disableAnnotationControls).to.be.called; }); }); describe('handleFullscreenExit()', () => { it('should resize the viewer', () => { sandbox.stub(base, 'resize'); + + base.handleFullscreenExit(); + + expect(base.resize).to.be.called; + }); + + it('should show annotations', () => { + sandbox.stub(base, 'areNewAnnotationsEnabled').returns(true); + sandbox.stub(base, 'enableAnnotationControls'); base.annotator = { emit: sandbox.mock(), }; - sandbox.stub(base, 'areNewAnnotationsEnabled').returns(true); + base.annotationControls = { + destroy: sandbox.mock(), + }; base.handleFullscreenExit(); - expect(base.resize).to.be.called; expect(base.annotator.emit).to.be.calledWith(ANNOTATOR_EVENT.setVisibility, true); + expect(base.enableAnnotationControls).to.be.called; }); }); @@ -1030,6 +1041,39 @@ describe('lib/viewers/BaseViewer', () => { }); }); + describe('disableAnnotationControls()', () => { + it('should hide annotations and toggle annotations mode', () => { + sandbox.stub(base, 'areNewAnnotationsEnabled').returns(true); + base.annotator = { + toggleAnnotationMode: sandbox.stub(), + }; + base.annotationControls = { + destroy: sandbox.stub(), + resetControls: sandbox.stub(), + toggle: sandbox.stub(), + }; + + base.disableAnnotationControls(); + + expect(base.annotationControls.resetControls).to.be.called; + expect(base.annotationControls.toggle).to.be.calledWith(false); + }); + }); + + describe('enableAnnotationControls()', () => { + it('should show annotations and the controls', () => { + sandbox.stub(base, 'areNewAnnotationsEnabled').returns(true); + base.annotationControls = { + destroy: sandbox.stub(), + toggle: sandbox.stub(), + }; + + base.enableAnnotationControls(); + + expect(base.annotationControls.toggle).to.be.calledWith(true); + }); + }); + describe('loadBoxAnnotations()', () => { const conf = { annotationsEnabled: true, diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index 43e19e106..be71c9ec3 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -128,6 +128,13 @@ class ImageViewer extends ImageBaseViewer { this.imageEl.style.transform = `rotate(${this.currentRotationAngle}deg)`; this.emit('rotate'); + // Disallow creating annotations on rotated images + if (this.currentRotationAngle === 0) { + this.enableAnnotationControls(); + } else { + this.disableAnnotationControls(); + } + // Re-adjust image position after rotation this.handleOrientationChange(); this.setScale(this.imageEl.offsetwidth, this.imageEl.offsetHeight);