Skip to content

Commit f16a273

Browse files
pramodsumpramodsum
authored
Fix: reset selection when entering annotation mode (#190)
* Chore: Separate hiding of create dialog and text selection * Chore: Add functional tests to ensure highlights are cleared
1 parent 71aea4b commit f16a273

File tree

4 files changed

+56
-87
lines changed

4 files changed

+56
-87
lines changed

‎functional-tests/tests/draw.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,11 @@ const {
1313
SELECTOR_ANNOTATION_DRAWING_DIALOG,
1414
SELECTOR_ADD_DRAWING_BTN,
1515
SELECTOR_ANNOTATION_DRAWING_LABEL,
16-
SELECTOR_DELETE_DRAWING_BTN
16+
SELECTOR_DELETE_DRAWING_BTN,
17+
SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG
1718
} = require('../helpers/constants');
1819

19-
const { draw, clickAtLocation } = require('../helpers/mouseEvents');
20+
const { draw, clickAtLocation, selectText } = require('../helpers/mouseEvents');
2021
const { cleanupAnnotations } = require('../helpers/cleanup');
2122

2223
Feature('Draw Annotation Sanity');
@@ -36,8 +37,13 @@ Scenario('Create/Delete drawing @desktop', function(I) {
3637
I.waitForVisible(SELECTOR_ANNOTATIONS_LOADED);
3738
I.waitForVisible(SELECTOR_ANNOTATION_BUTTON_DRAW);
3839

40+
I.say('Selected text will be cleared on entering draw mode');
41+
selectText(I, SELECTOR_TEXT_LAYER);
42+
I.waitForVisible(SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG);
43+
3944
I.say('Enter draw annotation mode');
4045
I.click(SELECTOR_ANNOTATION_BUTTON_DRAW);
46+
I.waitForDetached(SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG);
4147
I.waitForVisible('.bp-notification');
4248
I.waitForVisible(SELECTOR_ANNNOTATION_MODE_BACKGROUND);
4349
I.waitForVisible(SELECTOR_ANNOTATION_BUTTON_DRAW_POST);

‎functional-tests/tests/point.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,11 @@ const {
1111
SELECTOR_ANNOTATION_TEXTAREA,
1212
SELECTOR_ANNOTATION_BUTTON_POST,
1313
SELECTOR_ANNOTATION_BUTTON_CANCEL,
14-
SELECTOR_ANNOTATION_COMMENT
14+
SELECTOR_ANNOTATION_COMMENT,
15+
SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG
1516
} = require('../helpers/constants');
1617

17-
const { clickAtLocation } = require('../helpers/mouseEvents');
18+
const { clickAtLocation, selectText } = require('../helpers/mouseEvents');
1819
const { validateTextarea, validateAnnotation } = require('../helpers/validation');
1920
const { deleteAnnotation } = require('../helpers/actions');
2021
const { cleanupAnnotations } = require('../helpers/cleanup');
@@ -33,8 +34,13 @@ Scenario('Create/Delete point annotation @desktop', function(I) {
3334
I.waitForVisible(SELECTOR_ANNOTATIONS_LOADED);
3435
I.waitForVisible(SELECTOR_ANNOTATION_BUTTON_POINT);
3536

37+
I.say('Selected text will be cleared on entering point mode');
38+
selectText(I, SELECTOR_TEXT_LAYER);
39+
I.waitForVisible(SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG);
40+
3641
I.say('Enter point annotation mode');
3742
I.click(SELECTOR_ANNOTATION_BUTTON_POINT);
43+
I.waitForDetached(SELECTOR_ANNOTATION_HIGHLIGHT_DIALOG);
3844
I.waitForVisible('.bp-notification');
3945
I.waitForVisible(SELECTOR_ANNNOTATION_MODE_BACKGROUND);
4046
I.waitForVisible(SELECTOR_POINT_MODE_HEADER);

‎src/doc/DocAnnotator.js‎

Lines changed: 18 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ class DocAnnotator extends Annotator {
328328
this.highlightCreateHandler = this.highlightCreateHandler.bind(this);
329329
this.highlightMouseupHandler = this.highlightMouseupHandler.bind(this);
330330
this.highlightMousedownHandler = this.highlightMousedownHandler.bind(this);
331-
this.resetHighlightSelection = this.resetHighlightSelection.bind(this);
331+
this.hideCreateDialog = this.hideCreateDialog.bind(this);
332332

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

@@ -386,10 +386,10 @@ class DocAnnotator extends Annotator {
386386
// Highlight listeners on desktop & mobile
387387
if (this.plainHighlightEnabled || this.commentHighlightEnabled) {
388388
this.annotatedElement.addEventListener('mouseup', this.highlightMouseupHandler);
389-
this.annotatedElement.addEventListener('wheel', this.resetHighlightSelection);
389+
this.annotatedElement.addEventListener('wheel', this.hideCreateDialog);
390390

391391
if (this.hasTouch) {
392-
this.annotatedElement.addEventListener('touchend', this.resetHighlightSelection);
392+
this.annotatedElement.addEventListener('touchend', this.hideCreateDialog);
393393
}
394394
}
395395

@@ -424,7 +424,8 @@ class DocAnnotator extends Annotator {
424424
super.unbindDOMListeners();
425425

426426
this.annotatedElement.removeEventListener('mouseup', this.highlightMouseupHandler);
427-
this.annotatedElement.removeEventListener('wheel', this.resetHighlightSelection);
427+
this.annotatedElement.removeEventListener('wheel', this.hideCreateDialog);
428+
this.annotatedElement.removeEventListener('touchend', this.hideCreateDialog);
428429

429430
if (this.highlightThrottleHandle) {
430431
cancelAnimationFrame(this.highlightThrottleHandle);
@@ -460,15 +461,13 @@ class DocAnnotator extends Annotator {
460461
super.removeThreadFromSharedDialog();
461462
}
462463

463-
/**
464-
* Exits all annotation modes except the specified mode
465-
*
466-
* @param {string} mode - Current annotation mode
467-
* @return {void}
468-
*/
469-
toggleAnnotationMode(mode) {
470-
this.resetHighlightSelection();
471-
super.toggleAnnotationMode(mode);
464+
hideCreateDialog(event) {
465+
const isCreateDialogVisible = this.createHighlightDialog && this.createHighlightDialog.isVisible;
466+
if (!isCreateDialogVisible || !event || util.isInDialog(event)) {
467+
return;
468+
}
469+
470+
this.createHighlightDialog.hide();
472471
}
473472

474473
/**
@@ -478,16 +477,9 @@ class DocAnnotator extends Annotator {
478477
* @return {void}
479478
*/
480479
resetHighlightSelection(event) {
481-
const isCreateDialogVisible = this.createHighlightDialog && this.createHighlightDialog.isVisible;
482-
if (!isCreateDialogVisible || !event || util.isInDialog(event)) {
483-
return;
484-
}
485-
486-
this.createHighlightDialog.hide();
487-
488-
if (!this.isCreatingHighlight) {
489-
document.getSelection().removeAllRanges();
490-
}
480+
this.isCreatingHighlight = false;
481+
this.hideCreateDialog(event);
482+
document.getSelection().removeAllRanges();
491483
}
492484

493485
//--------------------------------------------------------------------------
@@ -706,8 +698,6 @@ class DocAnnotator extends Annotator {
706698
this.highlighter.removeAllHighlights();
707699
}
708700

709-
this.resetHighlightSelection(event);
710-
this.isCreatingHighlight = false;
711701
const hasMouseMoved =
712702
(this.mouseX && this.mouseX !== event.clientX) || (this.mouseY && this.mouseY !== event.clientY);
713703

@@ -787,6 +777,8 @@ class DocAnnotator extends Annotator {
787777
this.activeThread.show();
788778
} else if (this.isMobile) {
789779
this.removeThreadFromSharedDialog();
780+
} else {
781+
this.resetHighlightSelection(event);
790782
}
791783
}
792784

@@ -874,16 +866,12 @@ class DocAnnotator extends Annotator {
874866
* @return {void}
875867
*/
876868
handleControllerEvents(data) {
877-
const isCreateDialogVisible = this.createHighlightDialog && this.createHighlightDialog.isVisible;
878-
879869
switch (data.event) {
880870
case CONTROLLER_EVENT.toggleMode:
881871
this.resetHighlightSelection(data.event);
882872
break;
883873
case CONTROLLER_EVENT.bindDOMListeners:
884-
if (isCreateDialogVisible) {
885-
this.createHighlightDialog.hide();
886-
}
874+
this.hideCreateDialog(data);
887875
break;
888876
case CONTROLLER_EVENT.renderPage:
889877
this.renderPage(data.data);

‎src/doc/__tests__/DocAnnotator-test.js‎

Lines changed: 22 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -659,7 +659,7 @@ describe('doc/DocAnnotator', () => {
659659

660660
it('should bind DOM listeners if user can annotate and highlight', () => {
661661
stubs.elMock.expects('addEventListener').withArgs('mouseup', annotator.highlightMouseupHandler);
662-
stubs.elMock.expects('addEventListener').withArgs('wheel', annotator.resetHighlightSelection);
662+
stubs.elMock.expects('addEventListener').withArgs('wheel', annotator.hideCreateDialog);
663663
stubs.elMock.expects('addEventListener').withArgs('dblclick', annotator.highlightMouseupHandler);
664664
stubs.elMock.expects('addEventListener').withArgs('mousedown', annotator.highlightMousedownHandler);
665665
stubs.elMock.expects('addEventListener').withArgs('contextmenu', annotator.highlightMousedownHandler);
@@ -689,7 +689,7 @@ describe('doc/DocAnnotator', () => {
689689
annotator.drawEnabled = false;
690690

691691
stubs.elMock.expects('addEventListener').withArgs('mouseup', annotator.highlightMouseupHandler);
692-
stubs.elMock.expects('addEventListener').withArgs('wheel', annotator.resetHighlightSelection);
692+
stubs.elMock.expects('addEventListener').withArgs('wheel', annotator.hideCreateDialog);
693693
annotator.bindDOMListeners();
694694
});
695695

@@ -734,7 +734,7 @@ describe('doc/DocAnnotator', () => {
734734
.never();
735735
stubs.elMock
736736
.expects('addEventListener')
737-
.withArgs('wheel', annotator.resetHighlightSelection)
737+
.withArgs('wheel', annotator.hideCreateDialog)
738738
.never();
739739
stubs.elMock
740740
.expects('addEventListener')
@@ -770,7 +770,8 @@ describe('doc/DocAnnotator', () => {
770770
annotator.permissions.canAnnotate = true;
771771

772772
stubs.elMock.expects('removeEventListener').withArgs('mouseup', annotator.highlightMouseupHandler);
773-
stubs.elMock.expects('removeEventListener').withArgs('wheel', annotator.resetHighlightSelection);
773+
stubs.elMock.expects('removeEventListener').withArgs('wheel', annotator.hideCreateDialog);
774+
stubs.elMock.expects('removeEventListener').withArgs('touchend', annotator.hideCreateDialog);
774775
stubs.elMock.expects('removeEventListener').withArgs('dblclick', annotator.highlightMouseupHandler);
775776
stubs.elMock.expects('removeEventListener').withArgs('mousedown', annotator.highlightMousedownHandler);
776777
stubs.elMock.expects('removeEventListener').withArgs('contextmenu', annotator.highlightMousedownHandler);
@@ -846,44 +847,18 @@ describe('doc/DocAnnotator', () => {
846847

847848
describe('resetHighlightSelection()', () => {
848849
beforeEach(() => {
849-
annotator.createHighlightDialog = {
850-
isVisible: false,
851-
hide: sandbox.stub(),
852-
destroy: sandbox.stub()
853-
};
854-
stubs.isInDialog = sandbox.stub(util, 'isInDialog').returns(false);
855-
});
856-
857-
it('should do nothing if the createHighlightDialog is hidden', () => {
858-
annotator.resetHighlightSelection({});
859-
expect(annotator.createHighlightDialog.hide).to.not.be.called;
860-
});
861-
862-
it('should do nothing if the mouse event was triggered in an annotation dialog', () => {
863-
stubs.isInDialog.returns(true);
864-
annotator.resetHighlightSelection({});
865-
expect(annotator.createHighlightDialog.hide).to.not.be.called;
850+
sandbox.stub(annotator, 'hideCreateDialog');
866851
});
867852

868853
it('should hide the visible createHighlightDialog and clear the text selection', () => {
869-
annotator.createHighlightDialog.isVisible = true;
870-
annotator.resetHighlightSelection({});
871-
expect(annotator.createHighlightDialog.hide).to.be.called;
872-
});
873-
874-
it('should only clear the selected text if the user is creating a new highlight', () => {
875854
const selection = {
876855
removeAllRanges: sandbox.stub()
877856
};
878857
sandbox.stub(document, 'getSelection').returns(selection);
879-
annotator.createHighlightDialog.isVisible = true;
880-
881-
annotator.isCreatingHighlight = true;
882-
annotator.resetHighlightSelection({});
883-
expect(selection.removeAllRanges).to.not.be.called;
884858

885-
annotator.isCreatingHighlight = false;
886859
annotator.resetHighlightSelection({});
860+
expect(annotator.hideCreateDialog).to.be.called;
861+
expect(annotator.isCreatingHighlight).to.be.false;
887862
expect(selection.removeAllRanges).to.be.called;
888863
});
889864
});
@@ -943,7 +918,6 @@ describe('doc/DocAnnotator', () => {
943918
beforeEach(() => {
944919
stubs.create = sandbox.stub(annotator, 'highlightCreateHandler');
945920
stubs.click = sandbox.stub(annotator, 'highlightClickHandler');
946-
sandbox.stub(annotator, 'resetHighlightSelection');
947921
annotator.mouseX = undefined;
948922
annotator.mouseY = undefined;
949923
});
@@ -954,16 +928,12 @@ describe('doc/DocAnnotator', () => {
954928
annotator.highlightMouseupHandler({ x: 0, y: 0 });
955929
expect(stubs.create).to.be.called;
956930
expect(stubs.click).to.not.be.called;
957-
expect(annotator.isCreatingHighlight).to.be.false;
958-
expect(annotator.resetHighlightSelection).to.be.called;
959931
});
960932

961933
it('should call highlightCreateHandler if on desktop and the user double clicked', () => {
962934
annotator.highlightMouseupHandler({ type: 'dblclick' });
963935
expect(stubs.create).to.be.called;
964936
expect(stubs.click).to.not.be.called;
965-
expect(annotator.isCreatingHighlight).to.be.false;
966-
expect(annotator.resetHighlightSelection).to.be.called;
967937
});
968938

969939
it('should call highlightClickHandler if on desktop and createHighlightDialog exists', () => {
@@ -972,8 +942,6 @@ describe('doc/DocAnnotator', () => {
972942
annotator.highlightMouseupHandler({ x: 0, y: 0 });
973943
expect(stubs.create).to.not.be.called;
974944
expect(stubs.click).to.be.called;
975-
expect(annotator.isCreatingHighlight).to.be.false;
976-
expect(annotator.resetHighlightSelection).to.be.called;
977945
});
978946

979947
it('should call highlighter.removeAllHighlghts', () => {
@@ -982,7 +950,6 @@ describe('doc/DocAnnotator', () => {
982950
};
983951
annotator.highlightMouseupHandler({ x: 0, y: 0 });
984952
expect(annotator.highlighter.removeAllHighlights).to.be.called;
985-
expect(annotator.resetHighlightSelection).to.be.called;
986953
});
987954
});
988955

@@ -1192,6 +1159,7 @@ describe('doc/DocAnnotator', () => {
11921159
sandbox.stub(annotator, 'clickThread');
11931160
sandbox.stub(annotator, 'removeThreadFromSharedDialog');
11941161
sandbox.stub(annotator, 'hideAnnotations');
1162+
sandbox.stub(annotator, 'resetHighlightSelection');
11951163

11961164
annotator.modeControllers = {
11971165
highlight: {
@@ -1237,16 +1205,22 @@ describe('doc/DocAnnotator', () => {
12371205
annotator.plainHighlightEnabled = false;
12381206
annotator.commentHighlightEnabled = false;
12391207
stubs.threadMock.expects('show').never();
1208+
annotator.isMobile = true;
12401209

1241-
annotator.isMobile = false;
12421210
annotator.highlightClickHandler(stubs.event);
1243-
expect(annotator.removeThreadFromSharedDialog).to.not.be.called;
1211+
expect(annotator.removeThreadFromSharedDialog).to.be.called;
1212+
expect(annotator.hideAnnotations).to.be.called;
1213+
});
12441214

1215+
it('should reset highlight selection if not on mobile and no active threads exist', () => {
12451216
annotator.plainHighlightEnabled = false;
1246-
annotator.isMobile = true;
1217+
annotator.commentHighlightEnabled = false;
1218+
stubs.threadMock.expects('show').never();
1219+
1220+
annotator.isMobile = false;
12471221
annotator.highlightClickHandler(stubs.event);
1248-
expect(annotator.removeThreadFromSharedDialog).to.be.called;
1249-
expect(annotator.hideAnnotations).to.be.called;
1222+
expect(annotator.removeThreadFromSharedDialog).to.not.be.called;
1223+
expect(annotator.resetHighlightSelection).to.be.called;
12501224
});
12511225
});
12521226

@@ -1422,6 +1396,7 @@ describe('doc/DocAnnotator', () => {
14221396
};
14231397
sandbox.stub(annotator, 'renderPage');
14241398
sandbox.stub(annotator, 'resetHighlightSelection');
1399+
sandbox.stub(annotator, 'hideCreateDialog');
14251400
});
14261401

14271402
afterEach(() => {
@@ -1435,13 +1410,7 @@ describe('doc/DocAnnotator', () => {
14351410

14361411
it('should hide the createHighlightDialog on binddomlisteners', () => {
14371412
annotator.handleControllerEvents({ event: CONTROLLER_EVENT.bindDOMListeners });
1438-
expect(annotator.createHighlightDialog.hide).to.be.called;
1439-
});
1440-
1441-
it('should do nothing if createHighlightDialog is hidden or does not exist on binddomlisteners', () => {
1442-
annotator.createHighlightDialog.isVisible = false;
1443-
annotator.handleControllerEvents({ event: CONTROLLER_EVENT.bindDOMListeners });
1444-
expect(annotator.createHighlightDialog.hide).to.not.be.called;
1413+
expect(annotator.hideCreateDialog).to.be.called;
14451414
});
14461415

14471416
it('should render the specified page on annotationsrenderpage', () => {

0 commit comments

Comments
 (0)