Skip to content

Commit

Permalink
fix(image): Disallow creation of annotations on rotated images (#1232)
Browse files Browse the repository at this point in the history 
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
  • Loading branch information
@jstoffan @mergify
jstoffan and mergify[bot] authored Jul 1, 2020
1 parent a9edcd4 commit 2e99220
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 70 deletions.
58 changes: 18 additions & 40 deletions src/lib/AnnotationControls.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -53,8 +52,6 @@ export default class AnnotationControls {
this.controlsMap = {
[AnnotationMode.REGION]: this.updateRegionButton,
};

this.attachEventHandlers();
}

/**
Expand All @@ -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
*/
Expand All @@ -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
*/
Expand Down
33 changes: 12 additions & 21 deletions src/lib/__tests__/AnnotationControls-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import AnnotationControls, {
CLASS_REGION_BUTTON,
} from '../AnnotationControls';
import Controls from '../Controls';
import fullscreen from '../Fullscreen';

let annotationControls;
let stubs = {};
Expand All @@ -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: {
Expand Down Expand Up @@ -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');
});
Expand All @@ -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);
});
Expand All @@ -81,7 +73,6 @@ describe('lib/AnnotationControls', () => {

annotationControls.destroy();

expect(stubs.fullscreenRemoveListener).not.to.be.called;
expect(document.removeEventListener).not.to.be.called;
});
});
Expand Down Expand Up @@ -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();
Expand All @@ -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);
});
});
});
23 changes: 19 additions & 4 deletions src/lib/viewers/BaseViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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
*
Expand Down
54 changes: 49 additions & 5 deletions src/lib/viewers/__tests__/BaseViewer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});

Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions src/lib/viewers/image/ImageViewer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 2e99220

Please sign in to comment.