Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Pending highlight is not cleared on powerpoint page scroll #125

Merged
merged 6 commits into from
Mar 3, 2018
Merged

Fix: Pending highlight is not cleared on powerpoint page scroll #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
953e7c4
Fix: Pending highlight is not cleared on powerpoint page scroll
pramodsum Feb 28, 2018
58121ed
Update: tests
pramodsum Feb 28, 2018
fa09d86
Fix: Don't trigger when mouse wheel event is triggered in dialog
pramodsum Mar 1, 2018
da81914
Merge branch 'master' into bugs
pramodsum Mar 3, 2018
6e5e912
Fix: Reset highlights on mobile ppt scroll
pramodsum Mar 3, 2018
86f566e
Fix: tests
pramodsum Mar 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions src/doc/DocAnnotator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ class DocAnnotator extends Annotator {
this.highlightCreateHandler = this.highlightCreateHandler.bind(this);
this.highlightMouseupHandler = this.highlightMouseupHandler.bind(this);
this.highlightMousedownHandler = this.highlightMousedownHandler.bind(this);
this.resetHighlightSelection = this.resetHighlightSelection.bind(this);

this.clickThread = this.clickThread.bind(this);

Expand Down Expand Up @@ -396,6 +397,11 @@ class DocAnnotator extends Annotator {
// Highlight listeners on desktop & mobile
if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler);
this.annotatedElement.addEventListener('wheel', this.resetHighlightSelection);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does wheel also fire if you are scrolling through overflowed content in a dialog? That might clear things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll add a check to only reset the highlight if the mouse is not over an annotation dialog


if (this.hasTouch) {
this.annotatedElement.addEventListener('touchend', this.resetHighlightSelection);
}
}

if (this.hasTouch && this.drawEnabled) {
Expand Down Expand Up @@ -436,6 +442,7 @@ class DocAnnotator extends Annotator {
super.unbindDOMListeners();

this.annotatedElement.removeEventListener('mouseup', this.highlightMouseupHandler);
this.annotatedElement.removeEventListener('wheel', this.resetHighlightSelection);

if (this.highlightThrottleHandle) {
cancelAnimationFrame(this.highlightThrottleHandle);
Expand Down Expand Up @@ -473,6 +480,22 @@ class DocAnnotator extends Annotator {
super.resetMobileDialog();
}

/**
* Clears the text selection and hides the create highlight dialog
*
* @param {Event} event - Mouse wheel event
* @return {void}
*/
resetHighlightSelection(event) {
const isCreateDialogVisible = this.createHighlightDialog && this.createHighlightDialog.isVisible;
if (!isCreateDialogVisible || util.isInDialog(event)) {
return;
}

this.createHighlightDialog.hide();
document.getSelection().removeAllRanges();
}

//--------------------------------------------------------------------------
// Private
//--------------------------------------------------------------------------
Expand Down Expand Up @@ -829,12 +852,7 @@ class DocAnnotator extends Annotator {
this.highlighter.removeAllHighlights();
}

const isCreateDialogVisible = this.createHighlightDialog && this.createHighlightDialog.isVisible;
if (isCreateDialogVisible) {
this.createHighlightDialog.hide();
document.getSelection().removeAllRanges();
}

this.resetHighlightSelection();
this.isCreatingHighlight = false;

// Prevent the creation of highlights if the user is currently creating a point annotation
Expand Down Expand Up @@ -1008,10 +1026,7 @@ class DocAnnotator extends Annotator {

switch (data.event) {
case CONTROLLER_EVENT.toggleMode:
if (isCreateDialogVisible) {
document.getSelection().removeAllRanges();
this.createHighlightDialog.hide();
}
this.resetHighlightSelection();
break;
case CONTROLLER_EVENT.bindDOMListeners:
if (isCreateDialogVisible) {
Expand Down
90 changes: 41 additions & 49 deletions src/doc/__tests__/DocAnnotator-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import {
DATA_TYPE_ANNOTATION_DIALOG,
THREAD_EVENT,
CONTROLLER_EVENT,
CREATE_EVENT
CREATE_EVENT,
ANNOTATOR_TYPE
} from '../../constants';

let annotator;
Expand Down Expand Up @@ -648,6 +649,7 @@ 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('wheel', 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);
Expand Down Expand Up @@ -678,6 +680,7 @@ describe('doc/DocAnnotator', () => {
annotator.drawEnabled = false;

stubs.elMock.expects('addEventListener').withArgs('mouseup', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('wheel', sinon.match.func);
stubs.elMock.expects('addEventListener').withArgs('mousemove', sinon.match.func);
annotator.bindDOMListeners();
});
Expand Down Expand Up @@ -745,6 +748,7 @@ describe('doc/DocAnnotator', () => {
annotator.permissions.canAnnotate = true;

stubs.elMock.expects('removeEventListener').withArgs('mouseup', sinon.match.func);
stubs.elMock.expects('removeEventListener').withArgs('wheel', sinon.match.func);
stubs.elMock.expects('removeEventListener').withArgs('mousedown', sinon.match.func);
stubs.elMock.expects('removeEventListener').withArgs('contextmenu', sinon.match.func);
stubs.elMock.expects('removeEventListener').withArgs('mousemove', sinon.match.func);
Expand Down Expand Up @@ -821,6 +825,34 @@ describe('doc/DocAnnotator', () => {
});
});

describe('resetHighlightSelection()', () => {
beforeEach(() => {
annotator.createHighlightDialog = {
isVisible: false,
hide: sandbox.stub(),
destroy: sandbox.stub()
};
stubs.isInDialog = sandbox.stub(util, 'isInDialog').returns(false);
});

it('should do nothing if the createHighlightDialog is hidden', () => {
annotator.resetHighlightSelection();
expect(annotator.createHighlightDialog.hide).to.not.be.called;
});

it('should do nothing if the mouse event was triggered in an annotation dialog', () => {
stubs.isInDialog.returns(true);
annotator.resetHighlightSelection({});
expect(annotator.createHighlightDialog.hide).to.not.be.called;
});

it('hide the visible createHighlightDialog and clear the text selection', () => {
annotator.createHighlightDialog.isVisible = true;
annotator.resetHighlightSelection();
expect(annotator.createHighlightDialog.hide).to.be.called;
});
});

describe('highlightCurrentSelection()', () => {
beforeEach(() => {
annotator.highlighter = {
Expand Down Expand Up @@ -1011,6 +1043,7 @@ describe('doc/DocAnnotator', () => {
beforeEach(() => {
stubs.create = sandbox.stub(annotator, 'highlightCreateHandler');
stubs.click = sandbox.stub(annotator, 'highlightClickHandler');
sandbox.stub(annotator, 'resetHighlightSelection');
});

it('should not trigger a highlight or creation if a point annotation is pending', () => {
Expand All @@ -1021,20 +1054,23 @@ describe('doc/DocAnnotator', () => {
expect(stubs.create).to.not.be.called;
expect(stubs.click).to.not.be.called;
expect(annotator.isCreatingHighlight).to.be.false;
expect(annotator.resetHighlightSelection).to.be.called;
})

it('should call highlightCreateHandler if not on mobile, and the user double clicked', () => {
annotator.highlightMouseupHandler({ type: 'dblclick' });
expect(stubs.create).to.be.called;
expect(stubs.click).to.not.be.called;
expect(annotator.isCreatingHighlight).to.be.false;
expect(annotator.resetHighlightSelection).to.be.called;
});

it('should call highlightClickHandler if not on mobile, and the mouse did not move', () => {
annotator.highlightMouseupHandler({ x: 0, y: 0 });
expect(stubs.create).to.not.be.called;
expect(stubs.click).to.be.called;
expect(annotator.isCreatingHighlight).to.be.false;
expect(annotator.resetHighlightSelection).to.be.called;
});

it('should call highlighter.removeAllHighlghts', () => {
Expand All @@ -1043,40 +1079,7 @@ describe('doc/DocAnnotator', () => {
};
annotator.highlightMouseupHandler({ x: 0, y: 0 });
expect(annotator.highlighter.removeAllHighlights).to.be.called;
});

it('should not hide the highlight dialog and clear selection if the CreateHighlightDialog is not visible', () => {
annotator.createHighlightDialog = {
isVisible: false,
hide: sandbox.stub(),
removeListener: sandbox.stub(),
destroy: sandbox.stub()
}

const getSelectionStub = sandbox.stub(document, 'getSelection').returns({
removeAllRanges: sandbox.stub()
});

annotator.highlightMouseupHandler({ x: 0, y: 0 });
expect(annotator.createHighlightDialog.hide).to.not.be.called;
expect(getSelectionStub).to.not.be.called;
});

it('should hide the highlight dialog and clear selection if the CreateHighlightDialog is visible', () => {
annotator.createHighlightDialog = {
isVisible: true,
hide: sandbox.stub(),
removeListener: sandbox.stub(),
destroy: sandbox.stub()
}

const getSelectionStub = sandbox.stub(document, 'getSelection').returns({
removeAllRanges: sandbox.stub()
});

annotator.highlightMouseupHandler({ x: 0, y: 0 });
expect(annotator.createHighlightDialog.hide).to.be.called;
expect(getSelectionStub).to.be.called;
expect(annotator.resetHighlightSelection).to.be.called;
});
});

Expand Down Expand Up @@ -1504,33 +1507,22 @@ describe('doc/DocAnnotator', () => {

beforeEach(() => {
const selection = document.getSelection();
stubs.removeSelection = sandbox.stub(selection, 'removeAllRanges');
sandbox.stub(annotator, 'toggleAnnotationMode');
annotator.createHighlightDialog = {
isVisible: true,
hide: sandbox.stub(),
};
sandbox.stub(annotator, 'renderPage');
sandbox.stub(annotator, 'resetHighlightSelection');
});

afterEach(() => {
annotator.createHighlightDialog = null;
})

it('should clear selections and hide the createHighlightDialog on togglemode', () => {
annotator.handleControllerEvents({ event: CONTROLLER_EVENT.toggleMode, mode });
expect(stubs.removeSelection).to.be.called;
expect(annotator.createHighlightDialog.hide).to.be.called;
});

it('should do nothing if createHighlightDialog is hidden or does not exist on togglemode', () => {
annotator.createHighlightDialog = undefined;
annotator.handleControllerEvents({ event: CONTROLLER_EVENT.toggleMode, mode });
expect(stubs.removeSelection).to.not.be.called;

annotator.createHighlightDialog = { isVisible: false };
it('should clear selections and hide the createHighlightDialog on togglemode if needed', () => {
annotator.handleControllerEvents({ event: CONTROLLER_EVENT.toggleMode, mode });
expect(stubs.removeSelection).to.not.be.called;
expect(annotator.resetHighlightSelection).to.be.called;
});

it('should hide the createHighlightDialog on binddomlisteners', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ export function resetTextarea(element, clearText) {
*
* @private
* @param {Event} event Mouse event
* @param {HTMLElement} dialogEl Dialog element
* @param {HTMLElement} [dialogEl] Optional annotation dialog element
* @return {boolean} Whether or not mouse is inside dialog
*/
export function isInDialog(event, dialogEl) {
Expand Down