Skip to content

Commit 7bb178e

Browse files
author
Mingze
authored
feat(highlight): Improve selection logic (#559)
* feat(highlight): Improve selection logic * feat(highlight): Remove global store mock * feat(highlight): Address comments
1 parent d75c483 commit 7bb178e

File tree

8 files changed

+305
-82
lines changed

8 files changed

+305
-82
lines changed

‎src/document/DocumentAnnotator.ts‎

Lines changed: 19 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,39 +1,44 @@
1-
import debounce from 'lodash/debounce';
21
import BaseAnnotator, { Options } from '../common/BaseAnnotator';
32
import BaseManager from '../common/BaseManager';
43
import { centerRegion, isRegion, RegionManager } from '../region';
54
import { Event } from '../@types';
65
import { getAnnotation } from '../store/annotations';
76
import { getSelection } from './docUtil';
8-
import { HighlightManager } from '../highlight';
9-
import { Mode, setSelectionAction } from '../store';
7+
import { HighlightListener, HighlightManager } from '../highlight';
8+
import { Mode } from '../store';
109
import { scrollToLocation } from '../utils/scroll';
1110
import './DocumentAnnotator.scss';
1211

1312
// Pdf.js textLayer enhancement requires waiting for 300ms (they hardcode this magic number)
1413
const TEXT_LAYER_ENHANCEMENT = 300;
15-
// Selection change event is quite noisy, debounce it to avoid unnessary selection changes in store
16-
const SELECTION_CHANGE_DEBOUNCE = 100;
1714

1815
export default class DocumentAnnotator extends BaseAnnotator {
1916
annotatedEl?: HTMLElement;
2017

21-
managers: Map<number, Set<BaseManager>> = new Map();
18+
highlightListener?: HighlightListener;
2219

23-
selectionChangeTimer?: number;
20+
managers: Map<number, Set<BaseManager>> = new Map();
2421

2522
constructor(options: Options) {
2623
super(options);
2724

25+
if (this.isFeatureEnabled('highlightText')) {
26+
this.highlightListener = new HighlightListener({
27+
getSelection,
28+
selectionChangeDelay: TEXT_LAYER_ENHANCEMENT,
29+
store: this.store,
30+
});
31+
}
32+
2833
this.addListener(Event.ANNOTATIONS_MODE_CHANGE, this.handleChangeMode);
29-
document.addEventListener('selectionchange', this.debounceHandleSelectionChange);
3034
}
3135

3236
destroy(): void {
33-
clearTimeout(this.selectionChangeTimer);
34-
3537
this.removeListener(Event.ANNOTATIONS_MODE_CHANGE, this.handleChangeMode);
36-
document.removeEventListener('selectionchange', this.debounceHandleSelectionChange);
38+
39+
if (this.highlightListener) {
40+
this.highlightListener.destroy();
41+
}
3742

3843
super.destroy();
3944
}
@@ -98,27 +103,10 @@ export default class DocumentAnnotator extends BaseAnnotator {
98103
}
99104
};
100105

101-
handleSelectionChange = (): void => {
102-
// Clear previous selection immediately
103-
this.store.dispatch(setSelectionAction(null));
104-
clearTimeout(this.selectionChangeTimer);
105-
106-
this.selectionChangeTimer = window.setTimeout(() => {
107-
this.store.dispatch(setSelectionAction(getSelection()));
108-
}, TEXT_LAYER_ENHANCEMENT);
109-
};
110-
111-
debounceHandleSelectionChange = debounce(this.handleSelectionChange, SELECTION_CHANGE_DEBOUNCE, {
112-
// The previous selection needs to be cleared immediately when selection changes
113-
// Below options make sure the function is triggered on the leading edge of the timeout,
114-
// instead of on the trailing edge
115-
leading: true,
116-
trailing: false,
117-
});
118-
119106
render(): void {
120-
// Clear previous selection
121-
this.store.dispatch(setSelectionAction(null));
107+
if (this.highlightListener && this.annotatedEl) {
108+
this.highlightListener.init(this.annotatedEl);
109+
}
122110

123111
this.getPages()
124112
.filter(({ dataset }) => dataset.loaded && dataset.pageNumber)

‎src/document/__tests__/DocumentAnnotator-test.ts‎

Lines changed: 15 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,17 @@
11
import BaseManager from '../../common/BaseManager';
22
import DocumentAnnotator from '../DocumentAnnotator';
3+
import HighlightListener from '../../highlight/HighlightListener';
34
import RegionManager from '../../region/RegionManager';
45
import { Annotation, Event } from '../../@types';
56
import { annotations as regions } from '../../region/__mocks__/data';
6-
import { fetchAnnotationsAction, setSelectionAction } from '../../store';
7+
import { fetchAnnotationsAction } from '../../store';
78
import { HighlightManager } from '../../highlight';
8-
import { mockRange } from '../../store/selection/__mocks__/range';
99
import { scrollToLocation } from '../../utils/scroll';
1010

11-
jest.mock('lodash/debounce', () => (func: Function) => func);
1211
jest.mock('../../highlight/HighlightManager');
13-
jest.mock('../docUtil', () => ({
14-
getSelection: () => ({ location: 1, range: mockRange }),
15-
}));
1612
jest.mock('../../region/RegionManager');
1713
jest.mock('../../utils/scroll');
1814

19-
jest.useFakeTimers();
20-
2115
describe('DocumentAnnotator', () => {
2216
const container = document.createElement('div');
2317
const defaults = {
@@ -75,32 +69,27 @@ describe('DocumentAnnotator', () => {
7569
});
7670

7771
describe('constructor()', () => {
78-
test('should add selectionchange event listener', () => {
79-
jest.spyOn(document, 'addEventListener');
80-
81-
annotator = getAnnotator();
72+
test('should create HighlightListener if feature is enabled', () => {
73+
annotator = getAnnotator({ features: { highlightText: true } });
8274

83-
expect(document.addEventListener).toHaveBeenCalledWith('selectionchange', annotator.handleSelectionChange);
75+
expect(annotator.highlightListener).toBeInstanceOf(HighlightListener);
8476
});
8577
});
8678

8779
describe('destroy()', () => {
88-
test('should clear timeout and remove event handlers', () => {
89-
annotator.selectionChangeTimer = 1;
80+
test('should remove event handler ', () => {
81+
annotator = getAnnotator({ features: { highlightText: true } });
82+
9083
jest.spyOn(annotator, 'removeListener');
91-
jest.spyOn(document, 'removeEventListener');
84+
jest.spyOn(annotator.highlightListener as HighlightListener, 'destroy');
9285

9386
annotator.destroy();
9487

95-
expect(clearTimeout).toHaveBeenCalledWith(1);
9688
expect(annotator.removeListener).toHaveBeenCalledWith(
9789
'annotations_mode_change',
9890
annotator.handleChangeMode,
9991
);
100-
expect(document.removeEventListener).toHaveBeenCalledWith(
101-
'selectionchange',
102-
annotator.handleSelectionChange,
103-
);
92+
expect(annotator.highlightListener?.destroy).toHaveBeenCalled();
10493
});
10594
});
10695

@@ -205,24 +194,6 @@ describe('DocumentAnnotator', () => {
205194
});
206195
});
207196

208-
describe('handleSelectionChange()', () => {
209-
test('should clear selection and dispatch new selection', () => {
210-
jest.spyOn(annotator.store, 'dispatch');
211-
212-
annotator.handleSelectionChange();
213-
214-
expect(annotator.store.dispatch).toHaveBeenLastCalledWith(setSelectionAction(null));
215-
expect(annotator.selectionChangeTimer).not.toBeUndefined();
216-
expect(clearTimeout).toHaveBeenCalled();
217-
218-
jest.runAllTimers();
219-
220-
expect(annotator.store.dispatch).toHaveBeenLastCalledWith(
221-
setSelectionAction({ location: 1, range: mockRange }),
222-
);
223-
});
224-
});
225-
226197
describe('init()', () => {
227198
test('should set the root and annotated element based on class name', () => {
228199
annotator.init(5);
@@ -241,12 +212,14 @@ describe('DocumentAnnotator', () => {
241212
expect(annotator.renderPage).toHaveBeenCalledTimes(3);
242213
});
243214

244-
test('should clear previous selection', () => {
245-
jest.spyOn(annotator.store, 'dispatch');
215+
test('should init HighlightListener', () => {
216+
annotator = getAnnotator({ features: { highlightText: true } });
217+
annotator.annotatedEl = document.createElement('div');
218+
jest.spyOn(annotator.highlightListener as HighlightListener, 'init');
246219

247220
annotator.render();
248221

249-
expect(annotator.store.dispatch).toHaveBeenCalledWith(setSelectionAction(null));
222+
expect(annotator.highlightListener?.init).toHaveBeenCalledWith(annotator.annotatedEl);
250223
});
251224
});
252225

‎src/document/__tests__/docUtil-test.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,12 @@ describe('docUtil', () => {
3939

4040
describe('getRange()', () => {
4141
test.each`
42-
selection | result
43-
${null} | ${null}
44-
${{ isCollapsed: true, type: 'Range' }} | ${null}
45-
${{ type: 'Caret' }} | ${null}
46-
${{ getRangeAt: () => 'range', type: 'Range' }} | ${'range'}
42+
selection | result
43+
${null} | ${null}
44+
${{ type: 'Caret' }} | ${null}
45+
${{ isCollapsed: true, type: 'Range' }} | ${null}
46+
${{ isCollapsed: true, rangeCount: 0, type: 'Range' }} | ${null}
47+
${{ getRangeAt: () => 'range', rangeCount: 1, type: 'Range' }} | ${'range'}
4748
`('should return range as $result', ({ selection, result }) => {
4849
jest.spyOn(window, 'getSelection').mockImplementationOnce(() => selection);
4950

@@ -73,6 +74,7 @@ describe('docUtil', () => {
7374
range.getClientRects = jest.fn().mockReturnValueOnce([]);
7475
return range;
7576
},
77+
rangeCount: 1,
7678
type: 'Range',
7779
} as unknown) as Selection;
7880
};

‎src/document/docUtil.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,13 @@ export function getPageNumber(element: Element | null): number | undefined {
2828

2929
export function getRange(): Range | null {
3030
const selection = window.getSelection();
31-
if (!selection || selection.type !== 'Range' || selection.isCollapsed) {
31+
if (!selection || selection.type !== 'Range' || selection.isCollapsed || !selection.rangeCount) {
3232
return null;
3333
}
3434

35-
return selection.getRangeAt(0);
35+
// Firefox allows to select multiple ranges in the document by using Ctrl/Cmd+click
36+
// We only care about the last range
37+
return selection.getRangeAt(selection.rangeCount - 1);
3638
}
3739

3840
export function getSelection(): Selection | null {

‎src/highlight/HighlightListener.ts‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
import debounce from 'lodash/debounce';
2+
import { AppStore, getIsInitialized, SelectionArg as Selection, setSelectionAction } from '../store';
3+
import { MOUSE_PRIMARY } from '../constants';
4+
5+
export type Options = {
6+
getSelection: () => Selection | null;
7+
selectionChangeDelay?: number;
8+
store: AppStore;
9+
};
10+
11+
// Debounce 500ms for keyboard selection
12+
const SELECTION_CHANGE_DEBOUNCE = 500;
13+
14+
export default class HighlightListener {
15+
annotatedEl?: HTMLElement;
16+
17+
getSelection: () => Selection | null;
18+
19+
isMouseSelecting = false;
20+
21+
selectionChangeDelay?: number;
22+
23+
selectionChangeTimer?: number;
24+
25+
store: AppStore;
26+
27+
constructor({ getSelection, selectionChangeDelay = 0, store }: Options) {
28+
this.getSelection = getSelection;
29+
this.selectionChangeDelay = selectionChangeDelay;
30+
this.store = store;
31+
32+
document.addEventListener('selectionchange', this.debounceHandleSelectionChange);
33+
}
34+
35+
destroy(): void {
36+
clearTimeout(this.selectionChangeTimer);
37+
38+
document.removeEventListener('selectionchange', this.debounceHandleSelectionChange);
39+
40+
if (this.annotatedEl) {
41+
this.annotatedEl.removeEventListener('mousedown', this.handleMouseDown);
42+
this.annotatedEl.removeEventListener('mouseup', this.handleMouseUp);
43+
}
44+
}
45+
46+
init(annotatedEl: HTMLElement): void {
47+
this.annotatedEl = annotatedEl;
48+
49+
// Clear previous selection
50+
this.store.dispatch(setSelectionAction(null));
51+
52+
if (!getIsInitialized(this.store.getState())) {
53+
this.annotatedEl.addEventListener('mousedown', this.handleMouseDown);
54+
this.annotatedEl.addEventListener('mouseup', this.handleMouseUp);
55+
}
56+
}
57+
58+
setSelection = (): void => {
59+
this.store.dispatch(setSelectionAction(this.getSelection()));
60+
};
61+
62+
handleMouseDown = ({ buttons }: MouseEvent): void => {
63+
if (buttons !== MOUSE_PRIMARY) {
64+
return;
65+
}
66+
67+
this.isMouseSelecting = true;
68+
69+
clearTimeout(this.selectionChangeTimer);
70+
this.store.dispatch(setSelectionAction(null));
71+
};
72+
73+
handleMouseUp = (): void => {
74+
if (!this.isMouseSelecting) {
75+
return;
76+
}
77+
78+
this.isMouseSelecting = false;
79+
80+
this.selectionChangeTimer = window.setTimeout(this.setSelection, this.selectionChangeDelay);
81+
};
82+
83+
handleSelectionChange = (): void => {
84+
if (this.isMouseSelecting) {
85+
return;
86+
}
87+
88+
this.setSelection();
89+
};
90+
91+
debounceHandleSelectionChange = debounce(this.handleSelectionChange, SELECTION_CHANGE_DEBOUNCE);
92+
}

0 commit comments

Comments
 (0)