From abac192103e25173e7b2d9dd27277bd9423f73e1 Mon Sep 17 00:00:00 2001 From: Mingze Xiao Date: Mon, 22 Jun 2020 13:34:47 -0700 Subject: [PATCH] fix(annotations): Fix region toggle button does not exit region mode --- src/lib/AnnotationControls.ts | 31 +++++++++---------- src/lib/__tests__/AnnotationControls-test.js | 30 +++++++++--------- src/lib/viewers/BaseViewer.js | 4 +-- src/lib/viewers/__tests__/BaseViewer-test.js | 16 ++++++++-- src/lib/viewers/doc/DocBaseViewer.js | 2 +- .../doc/__tests__/DocBaseViewer-test.js | 2 +- src/lib/viewers/image/ImageViewer.js | 2 +- .../image/__tests__/ImageViewer-test.js | 2 +- 8 files changed, 49 insertions(+), 40 deletions(-) diff --git a/src/lib/AnnotationControls.ts b/src/lib/AnnotationControls.ts index 252abfb96..aa3ce69db 100644 --- a/src/lib/AnnotationControls.ts +++ b/src/lib/AnnotationControls.ts @@ -17,7 +17,7 @@ export enum AnnotationMode { export type ClickHandler = ({ activeControl, event }: { activeControl: AnnotationMode; event: MouseEvent }) => void; export type Options = { onRegionClick?: ClickHandler; - onReset?: () => void; + onEscape?: () => void; }; declare const __: (key: string) => string; @@ -33,11 +33,11 @@ export default class AnnotationControls { private controlsMap: ControlsMap; - private currentActiveControl: AnnotationMode = AnnotationMode.NONE; + private currentMode: AnnotationMode = AnnotationMode.NONE; private hasInit = false; - private onReset: () => void = noop; + private onEscape: () => void = noop; /** * [constructor] @@ -106,23 +106,21 @@ export default class AnnotationControls { private handleFullscreenExit = (): void => this.handleFullscreenChange(false); public getActiveMode = (): AnnotationMode => { - return this.currentActiveControl; + return this.currentMode; }; /** * Deactivate current control button */ public resetControls = (): void => { - if (this.currentActiveControl === AnnotationMode.NONE) { + if (this.currentMode === AnnotationMode.NONE) { return; } - const updateButton = this.controlsMap[this.currentActiveControl]; + const updateButton = this.controlsMap[this.currentMode]; - this.currentActiveControl = AnnotationMode.NONE; + this.currentMode = AnnotationMode.NONE; updateButton(); - - this.onReset(); }; /** @@ -135,7 +133,7 @@ export default class AnnotationControls { return; } - if (this.currentActiveControl === AnnotationMode.REGION) { + if (this.currentMode === AnnotationMode.REGION) { regionButtonElement.classList.add(CLASS_BUTTON_ACTIVE); } else { regionButtonElement.classList.remove(CLASS_BUTTON_ACTIVE); @@ -146,16 +144,16 @@ export default class AnnotationControls { * Region comment button click handler */ private handleClick = (onClick: ClickHandler, mode: AnnotationMode) => (event: MouseEvent): void => { - const prevActiveControl = this.currentActiveControl; + const prevActiveControl = this.currentMode; this.resetControls(); if (prevActiveControl !== mode) { - this.currentActiveControl = mode as AnnotationMode; + this.currentMode = mode as AnnotationMode; this.controlsMap[mode](); } - onClick({ activeControl: this.currentActiveControl, event }); + onClick({ activeControl: this.currentMode, event }); }; /** @@ -163,11 +161,12 @@ export default class AnnotationControls { * and stop propagation to prevent preview modal from exiting */ private handleKeyDown = (event: KeyboardEvent): void => { - if (event.key !== 'Escape' || this.currentActiveControl === AnnotationMode.NONE) { + if (event.key !== 'Escape' || this.currentMode === AnnotationMode.NONE) { return; } this.resetControls(); + this.onEscape(); event.preventDefault(); event.stopPropagation(); @@ -176,7 +175,7 @@ export default class AnnotationControls { /** * Initialize the annotation controls with options. */ - public init({ onRegionClick = noop, onReset = noop }: Options = {}): void { + public init({ onRegionClick = noop, onEscape = noop }: Options = {}): void { if (this.hasInit) { return; } @@ -192,7 +191,7 @@ export default class AnnotationControls { regionButton.setAttribute('data-testid', 'bp-AnnotationsControls-regionBtn'); - this.onReset = onReset; + this.onEscape = onEscape; document.addEventListener('keydown', this.handleKeyDown); this.hasInit = true; diff --git a/src/lib/__tests__/AnnotationControls-test.js b/src/lib/__tests__/AnnotationControls-test.js index 100370151..66051d5c6 100644 --- a/src/lib/__tests__/AnnotationControls-test.js +++ b/src/lib/__tests__/AnnotationControls-test.js @@ -52,7 +52,7 @@ describe('lib/AnnotationControls', () => { expect(annotationControls.controls).not.to.be.undefined; expect(annotationControls.controlsElement).not.to.be.undefined; expect(annotationControls.controlsMap).not.to.be.undefined; - expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE); + expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE); }); it('should attach event listeners', () => { @@ -119,11 +119,11 @@ describe('lib/AnnotationControls', () => { }); it('should set onRest and hasInit', () => { - const onResetMock = sandbox.stub(); + const onEscapeMock = sandbox.stub(); - annotationControls.init({ onReset: onResetMock }); + annotationControls.init({ onEscape: onEscapeMock }); - expect(annotationControls.onReset).to.equal(onResetMock); + expect(annotationControls.onEscape).to.equal(onEscapeMock); expect(annotationControls.hasInit).to.equal(true); }); @@ -144,7 +144,7 @@ describe('lib/AnnotationControls', () => { beforeEach(() => { annotationControls.resetControls = sandbox.stub(); - annotationControls.currentActiveControl = 'region'; + annotationControls.currentMode = 'region'; eventMock = { key: 'Escape', preventDefault: sandbox.stub(), @@ -157,7 +157,7 @@ describe('lib/AnnotationControls', () => { expect(annotationControls.resetControls).not.to.be.called; - annotationControls.currentActiveControl = 'none'; + annotationControls.currentMode = 'none'; annotationControls.handleKeyDown({ key: 'Escape' }); expect(annotationControls.resetControls).not.to.be.called; @@ -177,14 +177,14 @@ describe('lib/AnnotationControls', () => { }); it('should activate region button then deactivate', () => { - expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE); + expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE); annotationControls.handleClick(stubs.onRegionClick, AnnotationMode.REGION)(stubs.event); - expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.REGION); + expect(annotationControls.currentMode).to.equal(AnnotationMode.REGION); expect(stubs.classListAdd).to.be.calledWith(CLASS_BUTTON_ACTIVE); annotationControls.handleClick(stubs.onRegionClick, AnnotationMode.REGION)(stubs.event); - expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE); + expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE); expect(stubs.classListRemove).to.be.calledWith(CLASS_BUTTON_ACTIVE); }); @@ -213,34 +213,32 @@ describe('lib/AnnotationControls', () => { describe('resetControls()', () => { beforeEach(() => { stubs.updateRegionButton = sandbox.stub(); - stubs.onReset = sandbox.stub(); + stubs.onEscape = sandbox.stub(); annotationControls.controlsMap = { [AnnotationMode.REGION]: stubs.updateRegionButton, }; - annotationControls.onReset = stubs.onReset; }); it('should not change if no current active control', () => { annotationControls.resetControls(); - expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE); + expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE); expect(stubs.updateRegionButton).not.to.be.called; }); it('should call updateRegionButton if current control is region', () => { - annotationControls.currentActiveControl = AnnotationMode.REGION; + annotationControls.currentMode = AnnotationMode.REGION; annotationControls.resetControls(); - expect(annotationControls.currentActiveControl).to.equal(AnnotationMode.NONE); + expect(annotationControls.currentMode).to.equal(AnnotationMode.NONE); expect(stubs.updateRegionButton).to.be.called; - expect(stubs.onReset).to.be.called; }); }); describe('getActiveMode()', () => { it('should return the current active mode', () => { - annotationControls.currentActiveControl = 'region'; + annotationControls.currentMode = 'region'; expect(annotationControls.getActiveMode()).to.equal('region'); }); }); diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js index ecca7cc68..33f0ca90e 100644 --- a/src/lib/viewers/BaseViewer.js +++ b/src/lib/viewers/BaseViewer.js @@ -151,7 +151,7 @@ class BaseViewer extends EventEmitter { this.mobileZoomEndHandler = this.mobileZoomEndHandler.bind(this); this.handleAnnotatorEvents = this.handleAnnotatorEvents.bind(this); this.handleAnnotationCreateEvent = this.handleAnnotationCreateEvent.bind(this); - this.handleAnnotationControlsReset = this.handleAnnotationControlsReset.bind(this); + this.handleAnnotationControlsEscape = this.handleAnnotationControlsEscape.bind(this); this.handleFullscreenEnter = this.handleFullscreenEnter.bind(this); this.handleFullscreenExit = this.handleFullscreenExit.bind(this); this.handleRegionClick = this.handleRegionClick.bind(this); @@ -1047,7 +1047,7 @@ class BaseViewer extends EventEmitter { * @private * @return {void} */ - handleAnnotationControlsReset() { + handleAnnotationControlsEscape() { this.annotator.toggleAnnotationMode(AnnotationMode.NONE); } diff --git a/src/lib/viewers/__tests__/BaseViewer-test.js b/src/lib/viewers/__tests__/BaseViewer-test.js index a0c5d7ab0..b82da9635 100644 --- a/src/lib/viewers/__tests__/BaseViewer-test.js +++ b/src/lib/viewers/__tests__/BaseViewer-test.js @@ -1749,15 +1749,27 @@ describe('lib/viewers/BaseViewer', () => { }); }); - describe('handleAnnotationControlsReset()', () => { + describe('handleAnnotationControlsEscape()', () => { it('should call toggleAnnotationMode', () => { base.annotator = { toggleAnnotationMode: sandbox.stub(), }; - base.handleAnnotationControlsReset(); + base.handleAnnotationControlsEscape(); expect(base.annotator.toggleAnnotationMode).to.be.calledWith('none'); }); }); + + describe('handleRegionClick', () => { + it('should call toggleAnnotationMode', () => { + base.annotator = { + toggleAnnotationMode: sandbox.stub(), + }; + + base.handleRegionClick(); + + expect(base.annotator.toggleAnnotationMode).to.be.calledWith('region'); + }); + }); }); diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index d3fdcacc4..11ee20cb3 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -1100,7 +1100,7 @@ class DocBaseViewer extends BaseViewer { if (this.areNewAnnotationsEnabled() && this.hasAnnotationCreatePermission()) { this.annotationControls.init({ onRegionClick: this.handleRegionClick, - onReset: this.handleAnnotationControlsReset, + onEscape: this.handleAnnotationControlsEscape, }); } } diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js index d83363d1c..d4323fa9d 100644 --- a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js @@ -2304,7 +2304,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { ); expect(docBase.annotationControls.init).to.be.calledWith({ onRegionClick: docBase.handleRegionClick, - onReset: docBase.handleAnnotationControlsReset, + onEscape: docBase.handleAnnotationControlsEscape, }); }); diff --git a/src/lib/viewers/image/ImageViewer.js b/src/lib/viewers/image/ImageViewer.js index 4f637a6ca..ce292ac45 100644 --- a/src/lib/viewers/image/ImageViewer.js +++ b/src/lib/viewers/image/ImageViewer.js @@ -290,7 +290,7 @@ class ImageViewer extends ImageBaseViewer { this.annotationControls = new AnnotationControls(this.controls); this.annotationControls.init({ onRegionClick: this.handleRegionClick, - onReset: this.handleAnnotationControlsReset, + onEscape: this.handleAnnotationControlsEscape, }); } } diff --git a/src/lib/viewers/image/__tests__/ImageViewer-test.js b/src/lib/viewers/image/__tests__/ImageViewer-test.js index e534ab3a7..86a4f6441 100644 --- a/src/lib/viewers/image/__tests__/ImageViewer-test.js +++ b/src/lib/viewers/image/__tests__/ImageViewer-test.js @@ -372,7 +372,7 @@ describe('lib/viewers/image/ImageViewer', () => { expect(AnnotationControls.prototype.init).to.be.calledWith({ onRegionClick: image.handleRegionClick, - onReset: image.handleAnnotationControlsReset, + onEscape: image.handleAnnotationControlsEscape, }); }); });