Skip to content

Commit a6648d1

Browse files
author
Conrad Chan
authored
fix: Ensure document level mousedown listener is last (#668)
1 parent 08685f2 commit a6648d1

File tree

13 files changed

+215
-21
lines changed

13 files changed

+215
-21
lines changed

‎src/common/BaseAnnotator.ts‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import getProp from 'lodash/get';
22
import { IntlShape } from 'react-intl';
33
import * as store from '../store';
44
import API from '../api';
5+
import DeselectManager from './DeselectManager';
56
import EventEmitter from './EventEmitter';
67
import i18n from '../utils/i18n';
78
import messages from '../messages';
@@ -55,6 +56,8 @@ export default class BaseAnnotator extends EventEmitter {
5556

5657
container: Container;
5758

59+
deselectManager: DeselectManager | null = null;
60+
5861
features: Features;
5962

6063
intl: IntlShape;
@@ -120,7 +123,9 @@ export default class BaseAnnotator extends EventEmitter {
120123
this.annotatedEl.classList.remove(CSS_LOADED_CLASS);
121124
}
122125

123-
document.removeEventListener('mousedown', this.handleMouseDown);
126+
if (this.deselectManager) {
127+
this.deselectManager.destroy();
128+
}
124129

125130
this.removeAnnotationClasses();
126131

@@ -131,12 +136,6 @@ export default class BaseAnnotator extends EventEmitter {
131136
}
132137

133138
public init(scale = 1, rotation = 0): void {
134-
// Check for containerEl prevents listener from being added on subsequent calls to init
135-
if (!this.containerEl) {
136-
// Add document listener to handle setting active annotation to null on mousedown
137-
document.addEventListener('mousedown', this.handleMouseDown);
138-
}
139-
140139
this.containerEl = this.getElement(this.container);
141140
this.annotatedEl = this.getAnnotatedElement();
142141

@@ -203,10 +202,6 @@ export default class BaseAnnotator extends EventEmitter {
203202
return typeof selector === 'string' ? document.querySelector(selector) : selector;
204203
}
205204

206-
protected handleMouseDown = (): void => {
207-
this.setActiveId(null);
208-
};
209-
210205
protected handleRemove = (annotationId: string): void => {
211206
this.removeAnnotation(annotationId);
212207
};
@@ -248,4 +243,17 @@ export default class BaseAnnotator extends EventEmitter {
248243
protected render(): void {
249244
// Must be implemented in child class
250245
}
246+
247+
public postRender(): void {
248+
if (!this.annotatedEl) {
249+
return;
250+
}
251+
252+
if (this.deselectManager) {
253+
this.deselectManager.destroy();
254+
}
255+
256+
this.deselectManager = new DeselectManager({ referenceEl: this.annotatedEl });
257+
this.deselectManager.render({ store: this.store });
258+
}
251259
}

‎src/common/DeselectListener.tsx‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import React from 'react';
2+
import { useDispatch } from 'react-redux';
3+
import { setActiveAnnotationIdAction } from '../store/annotations';
4+
5+
export default function DeselectListener(): null {
6+
const dispatch = useDispatch();
7+
8+
React.useEffect(() => {
9+
const handleMouseDown = (): void => {
10+
dispatch(setActiveAnnotationIdAction(null));
11+
};
12+
13+
document.addEventListener('mousedown', handleMouseDown);
14+
15+
return () => {
16+
document.removeEventListener('mousedown', handleMouseDown);
17+
};
18+
});
19+
20+
return null;
21+
}

‎src/common/DeselectManager.tsx‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import * as React from 'react';
2+
import * as ReactDOM from 'react-dom';
3+
import { Provider } from 'react-redux';
4+
import { Store } from 'redux';
5+
import BaseManager from './BaseManager';
6+
import DeselectListener from './DeselectListener';
7+
8+
export type Options = {
9+
referenceEl: HTMLElement;
10+
};
11+
12+
export type Props = {
13+
store: Store;
14+
};
15+
16+
export default class DeselectManager extends BaseManager {
17+
decorate(): void {
18+
this.reactEl.classList.add('ba-Layer--deselect');
19+
this.reactEl.dataset.testid = 'ba-Layer--deselect';
20+
}
21+
22+
render({ store }: Props): void {
23+
ReactDOM.render(
24+
<Provider store={store}>
25+
<DeselectListener />
26+
</Provider>,
27+
this.reactEl,
28+
);
29+
}
30+
}

‎src/common/__tests__/BaseAnnotator-test.ts‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as store from '../../store';
22
import APIFactory from '../../api';
33
import BaseAnnotator, { ANNOTATION_CLASSES, CSS_CONTAINER_CLASS, CSS_LOADED_CLASS } from '../BaseAnnotator';
4+
import DeselectManager from '../DeselectManager';
45
import { ANNOTATOR_EVENT } from '../../constants';
56
import { Event, LegacyEvent } from '../../@types';
67
import { Mode } from '../../store/common';
@@ -196,12 +197,13 @@ describe('BaseAnnotator', () => {
196197
expect(annotator.removeListener).toBeCalledWith(LegacyEvent.SCALE, expect.any(Function));
197198
});
198199

199-
test('should remove mousedown event listener', () => {
200-
const removeEventListenerSpy = jest.spyOn(document, 'removeEventListener');
200+
test('should destroy DeselectManager', () => {
201+
const mockDeselectManager = ({ destroy: jest.fn() } as unknown) as DeselectManager;
202+
annotator.deselectManager = mockDeselectManager;
201203

202204
annotator.destroy();
203205

204-
expect(removeEventListenerSpy).toHaveBeenCalledWith('mousedown', expect.any(Function));
206+
expect(mockDeselectManager.destroy).toHaveBeenCalled();
205207
});
206208
});
207209

@@ -230,13 +232,6 @@ describe('BaseAnnotator', () => {
230232
expect(annotator.emit).toBeCalledWith(ANNOTATOR_EVENT.error, expect.any(String));
231233
expect(annotator.containerEl).toBeNull();
232234
});
233-
234-
test('should add a mousedown handler for deselecting the active id', () => {
235-
const addEventListenerSpy = jest.spyOn(document, 'addEventListener');
236-
annotator.init();
237-
238-
expect(addEventListenerSpy).toHaveBeenCalledWith('mousedown', expect.any(Function));
239-
});
240235
});
241236

242237
describe('event handlers', () => {

‎src/common/__tests__/DeselectListener-test.tsx‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import React from 'react';
2+
import { shallow, ShallowWrapper } from 'enzyme';
3+
import DeselectListener from '../DeselectListener';
4+
import { setActiveAnnotationIdAction } from '../../store/annotations/actions';
5+
6+
jest.mock('react-redux', () => ({
7+
useDispatch: () => jest.fn(),
8+
}));
9+
jest.mock('../../store/annotations/actions');
10+
11+
describe('DeselectListener', () => {
12+
const getWrapper = (): ShallowWrapper => shallow(<DeselectListener />);
13+
14+
beforeEach(() => {
15+
jest.spyOn(React, 'useEffect').mockImplementation(f => f());
16+
jest.spyOn(document, 'addEventListener');
17+
});
18+
19+
test('mousedown', () => {
20+
const wrapper = getWrapper();
21+
22+
document.dispatchEvent(new MouseEvent('mousedown'));
23+
24+
expect(wrapper.isEmptyRender()).toBe(true);
25+
expect(document.addEventListener).toHaveBeenCalledWith('mousedown', expect.any(Function));
26+
expect(setActiveAnnotationIdAction).toHaveBeenCalledWith(null);
27+
});
28+
});

‎src/common/__tests__/DeselectManager-test.tsx‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import ReactDOM from 'react-dom';
2+
import DeselectManager from '../DeselectManager';
3+
import { createStore } from '../../store';
4+
import { Options } from '../BaseManager';
5+
6+
jest.mock('react-dom', () => ({
7+
render: jest.fn(),
8+
unmountComponentAtNode: jest.fn(),
9+
}));
10+
11+
describe('DeselectManager', () => {
12+
const rootEl = document.createElement('div');
13+
const getOptions = (options: Partial<Options> = {}): Options => ({
14+
referenceEl: rootEl.querySelector('.reference') as HTMLElement,
15+
...options,
16+
});
17+
const getWrapper = (options?: Partial<Options>): DeselectManager => new DeselectManager(getOptions(options));
18+
19+
beforeEach(() => {
20+
rootEl.classList.add('root');
21+
rootEl.innerHTML = '<div class="reference" />'; // referenceEl
22+
});
23+
24+
describe('decorate()', () => {
25+
test('should add class and testid', () => {
26+
const wrapper = getWrapper();
27+
wrapper.decorate();
28+
29+
expect(wrapper.reactEl.classList.contains('ba-Layer--deselect')).toBe(true);
30+
expect(wrapper.reactEl.dataset.testid).toEqual('ba-Layer--deselect');
31+
});
32+
});
33+
34+
describe('render()', () => {
35+
test('should format the props and pass them to the underlying components', () => {
36+
const wrapper = getWrapper();
37+
38+
wrapper.render({ store: createStore() });
39+
40+
expect(ReactDOM.render).toHaveBeenCalled();
41+
});
42+
});
43+
});

‎src/document/DocumentAnnotator.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,8 @@ export default class DocumentAnnotator extends BaseAnnotator {
142142
this.getPages()
143143
.filter(({ dataset }) => dataset.loaded && dataset.pageNumber)
144144
.forEach(pageEl => this.renderPage(pageEl));
145+
146+
this.postRender();
145147
}
146148

147149
renderPage(pageEl: HTMLElement): void {

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import DeselectManager from '../../common/DeselectManager';
12
import DocumentAnnotator from '../DocumentAnnotator';
23
import HighlightListener from '../../highlight/HighlightListener';
34
import PopupManager from '../../popup/PopupManager';
@@ -13,6 +14,7 @@ import { HighlightCreatorManager, HighlightManager } from '../../highlight';
1314
import { Manager } from '../../common/BaseManager';
1415
import { scrollToLocation } from '../../utils/scroll';
1516

17+
jest.mock('../../common/DeselectManager');
1618
jest.mock('../../highlight/HighlightManager');
1719
jest.mock('../../popup/PopupManager');
1820
jest.mock('../../region/RegionCreationManager');
@@ -237,6 +239,17 @@ describe('DocumentAnnotator', () => {
237239

238240
expect(annotator.renderPage).toHaveBeenCalledTimes(3);
239241
});
242+
243+
test('should instantiate the DeselectManager and call render', () => {
244+
annotator.annotatedEl = container.querySelector('.bp-doc') as HTMLElement;
245+
246+
expect(annotator.deselectManager).toBeNull();
247+
248+
annotator.render();
249+
250+
expect(annotator.deselectManager).toBeInstanceOf(DeselectManager);
251+
expect(annotator.deselectManager!.render).toHaveBeenCalled();
252+
});
240253
});
241254

242255
describe('renderPage()', () => {

‎src/image/ImageAnnotator.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ export default class ImageAnnotator extends BaseAnnotator {
105105
store: this.store,
106106
});
107107
});
108+
109+
this.postRender();
108110
}
109111

110112
scrollToAnnotation(annotationId: string | null): void {

‎src/image/__tests__/ImageAnnotator-test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import DeselectManager from '../../common/DeselectManager';
12
import DrawingManager from '../../drawing/DrawingManager';
23
import ImageAnnotator, { CSS_IS_DRAWING_CLASS } from '../ImageAnnotator';
34
import PopupManager from '../../popup/PopupManager';
@@ -9,6 +10,7 @@ import { annotations as drawings } from '../../drawing/__mocks__/drawingData';
910
import { annotations as regions } from '../../region/__mocks__/data';
1011
import { scrollToLocation } from '../../utils/scroll';
1112

13+
jest.mock('../../common/DeselectManager');
1214
jest.mock('../../popup/PopupManager');
1315
jest.mock('../../region/RegionCreationManager');
1416
jest.mock('../../region/RegionManager');
@@ -221,6 +223,17 @@ describe('ImageAnnotator', () => {
221223
expect(mockManager.render).not.toHaveBeenCalled();
222224
expect(mockManager.style).not.toHaveBeenCalled();
223225
});
226+
227+
test('should instantiate the DeselectManager and call render', () => {
228+
annotator.annotatedEl = getParent();
229+
230+
expect(annotator.deselectManager).toBeNull();
231+
232+
annotator.render();
233+
234+
expect(annotator.deselectManager).toBeInstanceOf(DeselectManager);
235+
expect(annotator.deselectManager!.render).toHaveBeenCalled();
236+
});
224237
});
225238

226239
describe('scrollToAnnotation()', () => {

0 commit comments

Comments
 (0)