Skip to content

Commit

Permalink
Fix: Enable drawingSelection & highlightMouseMove handlers for read-o…
Browse files Browse the repository at this point in the history 
…nly permissions (#61)

* Fix: Add highlightmousemove handler for read-only permissions
* Fix: Enable selection of draw annotations in read-only mode
* Update: tests
  • Loading branch information
@pramodsum
pramodsum authored Dec 8, 2017
1 parent f501edc commit 33bda6a
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 29 deletions.
4 changes: 0 additions & 4 deletions src/Annotator.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,10 +479,6 @@ $tablet: 'max-width: 768px';
white-space: normal;
}

.bp-annotation-drawing-dialog {
padding: 0;
}

.bp-use-default-cursor {
cursor: default;

Expand Down
35 changes: 17 additions & 18 deletions src/doc/DocAnnotator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,37 +425,36 @@ class DocAnnotator extends Annotator {
bindDOMListeners() {
super.bindDOMListeners();

// Highlight listeners on desktop & mobile
if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler);
}

// Prevent all forms of highlight annotations if annotating (or plain AND comment highlights) is disabled
if (!this.permissions.canAnnotate) {
return;
}

if (this.hasTouch && this.isMobile) {
if (this.drawEnabled) {
this.annotatedElement.addEventListener('touchstart', this.drawingSelectionHandler);
}

// Highlight listeners
if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
document.addEventListener('selectionchange', this.onSelectionChange);
}
if (this.hasTouch && this.isMobile && this.drawEnabled) {
this.annotatedElement.addEventListener('touchstart', this.drawingSelectionHandler);
} else {
if (this.drawEnabled) {
this.annotatedElement.addEventListener('click', this.drawingSelectionHandler);
}

// Highlight listeners
// Desktop-only highlight listeners
if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler);
this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler);
this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler);
this.annotatedElement.addEventListener('mousemove', this.getHighlightMouseMoveHandler());
}
}

// Prevent highlight creation if annotating (or plain AND comment highlights) is disabled
if (!this.permissions.canAnnotate || !(this.plainHighlightEnabled || this.commentHighlightEnabled)) {
return;
}

if (this.hasTouch && this.isMobile) {
document.addEventListener('selectionchange', this.onSelectionChange);
} else {
this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler);
this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler);
this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler);
}
}

/**
Expand Down
37 changes: 30 additions & 7 deletions src/doc/__tests__/DocAnnotator-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -678,15 +678,41 @@ describe('doc/DocAnnotator', () => {

it('should bind DOM listeners if user can annotate and highlight', () => {
stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('mousemove', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('dblclick', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('mousedown', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('contextmenu', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('mousemove', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('click', sinon.match.func)
annotator.bindDOMListeners();
});

it('should bind selectionchange and touchstart event, on the document, if on mobile and can annotate', () => {
it('should bind draw selection handlers regardless of if the user can annotate ', () => {
annotator.permissions.canAnnotate = false;
const annotatedElementListen = sandbox.spy(annotator.annotatedElement, 'addEventListener');

// Desktop draw selection handlers
annotator.bindDOMListeners();
expect(annotatedElementListen).to.be.calledWith('click', annotator.drawingSelectionHandler);

// Mobile draw selection handlers
annotator.isMobile = true;
annotator.hasTouch = true;
annotator.bindDOMListeners();
expect(annotatedElementListen).to.be.calledWith('touchstart', annotator.drawingSelectionHandler);
});

it('should bind highlight mouse move handlers regardless of if the user can annotate only on desktop', () => {
annotator.permissions.canAnnotate = false;
annotator.plainHighlightEnabled = true;
annotator.commentHighlightEnabled = true;
annotator.drawEnabled = false;

stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('mousemove', sinon.match.func);
annotator.bindDOMListeners();
});

it('should bind selectionchange event on the document, if on mobile and can annotate', () => {
annotator.isMobile = true;
annotator.hasTouch = true;
annotator.drawEnabled = true;
Expand All @@ -697,10 +723,9 @@ describe('doc/DocAnnotator', () => {
annotator.bindDOMListeners();

expect(docListen).to.be.calledWith('selectionchange', sinon.match.func);
expect(annotatedElementListen).to.be.calledWith('touchstart', sinon.match.func);
});

it('should not bind selection change event if both annotation types are disabled, and touch enabled, mobile enabled', () => {
it('should not bind selection change event if both annotation types are disabled, and touch and mobile enabled', () => {
annotator.isMobile = true;
annotator.hasTouch = true;
annotator.plainHighlightEnabled = false;
Expand All @@ -713,10 +738,9 @@ describe('doc/DocAnnotator', () => {
annotator.bindDOMListeners();

expect(docListen).to.not.be.calledWith('selectionchange', sinon.match.func);
expect(annotatedElementListen).to.be.calledWith('touchstart', sinon.match.func);
});

it('should not bind selection change event if both annotation types are disabled, and touch disabled, mobile disabled', () => {
it('should not bind selection change event if both annotation types are disabled, and touch and mobile disabled', () => {
annotator.isMobile = false;
annotator.hasTouch = false;
annotator.plainHighlightEnabled = false;
Expand All @@ -728,7 +752,6 @@ describe('doc/DocAnnotator', () => {
stubs.elMock.expects('addEventListener').never().withArgs('dblclick', sinon.match.func);
stubs.elMock.expects('addEventListener').never().withArgs('mousedown', sinon.match.func);
stubs.elMock.expects('addEventListener').never().withArgs('contextmenu', sinon.match.func);
stubs.elMock.expects('addEventListener').never().withArgs('mousemove', sinon.match.func);

annotator.bindDOMListeners();
});
Expand Down

0 comments on commit 33bda6a

Please sign in to comment.