diff --git a/src/lib/annotations/doc/DocAnnotator.js b/src/lib/annotations/doc/DocAnnotator.js index 2f026adf9..36bef9468 100644 --- a/src/lib/annotations/doc/DocAnnotator.js +++ b/src/lib/annotations/doc/DocAnnotator.js @@ -5,7 +5,6 @@ import rangyClassApplier from 'rangy/lib/rangy-classapplier'; import rangyHighlight from 'rangy/lib/rangy-highlighter'; import rangySaveRestore from 'rangy/lib/rangy-selectionsaverestore'; /* eslint-enable no-unused-vars */ -import throttle from 'lodash.throttle'; import autobind from 'autobind-decorator'; import Annotator from '../Annotator'; import DocHighlightThread from './DocHighlightThread'; @@ -18,8 +17,62 @@ const MOUSEMOVE_THROTTLE_MS = 50; const PAGE_PADDING_BOTTOM = 15; const PAGE_PADDING_TOP = 15; const HOVER_TIMEOUT_MS = 75; +const MOUSE_MOVE_MIN_DISTANCE = 5; + +/** + * For filtering out and only showing the first thread in a list of threads. + * + * @param {Object} thread - The annotation thread to either hide or show + * @param {number} index - The index of the annotation thread + * @return {void} + */ +function showFirstDialogFilter(thread, index) { + if (index === 0) { + thread.show(); + } else { + thread.hideDialog(); + } +} + +/** + * Check if a thread is in a hover state. + * + * @param {Object} thread - The thread to check the state of + * @return {boolean} True if the thread is in a state of hover + */ +function isThreadInHoverState(thread) { + return constants.HOVER_STATES.indexOf(thread.state) > -1; +} @autobind class DocAnnotator extends Annotator { + /** + * For tracking the most recent event fired by mouse move event. + * + * @property {Event} + */ + mouseMoveEvent; + + /** + * Event callback for mouse move events with for highlight annotations. + * + * @property {Function} + */ + highlightMousemoveHandler; + + /** + * Handle to RAF used to throttle highlight collision checks. + * + * @property {Function} + */ + highlightThrottleHandle; + + /** + * Timer used to throttle highlight event process. + * + * @property {number} + */ + throttleTimer = 0; + //-------------------------------------------------------------------------- // Abstract Implementations //-------------------------------------------------------------------------- @@ -245,7 +298,7 @@ const HOVER_TIMEOUT_MS = 75; this.annotatedElement.addEventListener('dblclick', this.highlightMouseupHandler); this.annotatedElement.addEventListener('mousedown', this.highlightMousedownHandler); this.annotatedElement.addEventListener('contextmenu', this.highlightMousedownHandler); - this.annotatedElement.addEventListener('mousemove', this.highlightMousemoveHandler()); + this.annotatedElement.addEventListener('mousemove', this.getHighlightMouseMoveHandler()); } } @@ -263,7 +316,12 @@ const HOVER_TIMEOUT_MS = 75; this.annotatedElement.removeEventListener('dblclick', this.highlightMouseupHandler); this.annotatedElement.removeEventListener('mousedown', this.highlightMousedownHandler); this.annotatedElement.removeEventListener('contextmenu', this.highlightMousedownHandler); - this.annotatedElement.removeEventListener('mousemove', this.highlightMousemoveHandler()); + this.annotatedElement.removeEventListener('mousemove', this.getHighlightMouseMoveHandler()); + + if (this.highlightThrottleHandle) { + cancelAnimationFrame(this.highlightThrottleHandle); + this.highlightThrottleHandle = null; + } } } @@ -352,76 +410,110 @@ const HOVER_TIMEOUT_MS = 75; * @private * @return {Function} mousemove handler */ - highlightMousemoveHandler() { - if (this.throttledHighlightMousemoveHandler) { - return this.throttledHighlightMousemoveHandler; + getHighlightMouseMoveHandler() { + if (this.highlightMousemoveHandler) { + return this.highlightMousemoveHandler; } - this.throttledHighlightMousemoveHandler = throttle((event) => { - // Only filter through highlight threads on the current page - const page = annotatorUtil.getPageElAndPageNumber(event.target).page; - const pageThreads = this.getHighlightThreadsOnPage(page); - const delayThreads = []; - - pageThreads.forEach((thread) => { - // Determine if any highlight threads on page are pending or active - // and ignore hover events of any highlights below - if ( - thread.state === constants.ANNOTATION_STATE_PENDING || - thread.state === constants.ANNOTATION_STATE_ACTIVE - ) { - return; - } + this.highlightMousemoveHandler = this.onHighlightMouseMove.bind(this); - // Determine if the mouse is hovering over any highlight threads - const shouldDelay = thread.onMousemove(event); - if (shouldDelay) { - delayThreads.push(thread); - } - }); + const highlightLoop = () => { + this.highlightThrottleHandle = requestAnimationFrame(highlightLoop); + this.onHighlightCheck(); + }; - // Ignore small mouse movements when figuring out if a mousedown - // and mouseup was a click - if (Math.abs(event.clientX - this.mouseX) > 5 || Math.abs(event.clientY - this.mouseY) > 5) { - this.didMouseMove = true; - } + // Kickstart event process loop. + highlightLoop(); - // Determine if the user is creating a new overlapping highlight - // and ignore hover events of any highlights below - if (this.isCreatingHighlight) { - return; - } + return this.highlightMousemoveHandler; + } + + /** + * Throttled processing of the most recent mouse move event. + * + * @return {void} + */ + onHighlightCheck() { + const dt = performance.now() - this.throttleTimer; + // Bail if no mouse events have occurred OR the throttle delay has not been met. + if (!this.mouseMoveEvent || dt < MOUSEMOVE_THROTTLE_MS) { + return; + } + + const event = this.mouseMoveEvent; + this.mouseMoveEvent = null; + this.throttleTimer = performance.now(); + // Only filter through highlight threads on the current page + const { page } = annotatorUtil.getPageElAndPageNumber(event.target); + const pageThreads = this.getHighlightThreadsOnPage(page); + const delayThreads = []; + let hoverActive = false; - // If we are hovering over a highlight, we should use a hand cursor + const threadLength = pageThreads.length; + for (let i = 0; i < threadLength; ++i) { + const thread = pageThreads[i]; + // Determine if any highlight threads on page are pending or active + // and ignore hover events of any highlights below if ( - delayThreads.some((thread) => { - return constants.HOVER_STATES.indexOf(thread.state) > 1; - }) + thread.state === constants.ANNOTATION_STATE_PENDING || + thread.state === constants.ANNOTATION_STATE_ACTIVE ) { - this.useDefaultCursor(); - clearTimeout(this.cursorTimeout); - } else { - // Setting timeout on cursor change so cursor doesn't - // flicker when hovering on line spacing - this.cursorTimeout = setTimeout(() => { - this.removeDefaultCursor(); - }, HOVER_TIMEOUT_MS); + return; } - // Delayed threads (threads that should be in active or hover - // state) should be drawn last. If multiple highlights are - // hovered over at the same time, only the top-most highlight - // dialog will be displayed and the others will be hidden - // without delay - delayThreads.forEach((thread, index) => { - if (index === 0) { - thread.show(); - } else { - thread.hideDialog(); + // Determine if the mouse is hovering over any highlight threads + const shouldDelay = thread.onMousemove(event); + if (shouldDelay) { + delayThreads.push(thread); + + if (!hoverActive) { + hoverActive = isThreadInHoverState(thread); } - }); - }, MOUSEMOVE_THROTTLE_MS); - return this.throttledHighlightMousemoveHandler; + } + } + + // If we are hovering over a highlight, we should use a hand cursor + if (hoverActive) { + this.useDefaultCursor(); + clearTimeout(this.cursorTimeout); + } else { + // Setting timeout on cursor change so cursor doesn't + // flicker when hovering on line spacing + this.cursorTimeout = setTimeout(() => { + this.removeDefaultCursor(); + }, HOVER_TIMEOUT_MS); + } + + // Delayed threads (threads that should be in active or hover + // state) should be drawn last. If multiple highlights are + // hovered over at the same time, only the top-most highlight + // dialog will be displayed and the others will be hidden + // without delay + delayThreads.forEach(showFirstDialogFilter); + } + + /** + * Mouse move handler. Paired with throttle mouse move handler to check for annotation highlights. + * + * @param {Event} event - DDOM event fired by mouse move event + * @return {void} + */ + onHighlightMouseMove(event) { + if ( + !this.didMouseMove && + (Math.abs(event.clientX - this.mouseX) > MOUSE_MOVE_MIN_DISTANCE || + Math.abs(event.clientY - this.mouseY) > MOUSE_MOVE_MIN_DISTANCE) + ) { + this.didMouseMove = true; + } + + // Determine if the user is creating a new overlapping highlight + // and ignore hover events of any highlights below + if (this.isCreatingHighlight) { + return; + } + + this.mouseMoveEvent = event; } /** diff --git a/src/lib/annotations/doc/DocHighlightThread.js b/src/lib/annotations/doc/DocHighlightThread.js index 4298b146b..ba5a144a3 100644 --- a/src/lib/annotations/doc/DocHighlightThread.js +++ b/src/lib/annotations/doc/DocHighlightThread.js @@ -10,6 +10,13 @@ const PAGE_PADDING_TOP = 15; const HOVER_TIMEOUT_MS = 75; @autobind class DocHighlightThread extends AnnotationThread { + /** + * Cached page element for the document. + * + * @property {HTMLElement} + */ + pageEl; + //-------------------------------------------------------------------------- // Public //-------------------------------------------------------------------------- @@ -450,24 +457,45 @@ const HOVER_TIMEOUT_MS = 75; PAGE_PADDING_TOP + PAGE_PADDING_BOTTOM ); + const scaleVertices = (val, index) => { + return index % 2 ? val * dimensionScale.y : val * dimensionScale.x; + }; + // DOM coordinates with respect to the page const x = event.clientX - pageDimensions.left; const y = event.clientY - pageTop; - return this.location.quadPoints.some((quadPoint) => { + let eventOccurredInHighlight = false; + + const points = this.location.quadPoints; + const length = points.length; + + let index = 0; + while (index < length && !eventOccurredInHighlight) { + const quadPoint = points[index]; // If needed, scale quad points comparing current dimensions with saved dimensions - let scaledQuadPoint = quadPoint; + const scaledQuadPoint = [...quadPoint]; if (dimensionScale) { - scaledQuadPoint = quadPoint.map((val, index) => { - return index % 2 ? val * dimensionScale.y : val * dimensionScale.x; - }); + const qLength = quadPoint.length; + for (let i = 0; i < qLength; i++) { + scaledQuadPoint[i] = scaleVertices(quadPoint[i], i); + } } const browserQuadPoint = docAnnotatorUtil.convertPDFSpaceToDOMSpace(scaledQuadPoint, pageHeight, zoomScale); + const [x1, y1, x2, y2, x3, y3, x4, y4] = browserQuadPoint; - return docAnnotatorUtil.isPointInPolyOpt([[x1, y1], [x2, y2], [x3, y3], [x4, y4]], x, y); - }); + eventOccurredInHighlight = docAnnotatorUtil.isPointInPolyOpt( + [[x1, y1], [x2, y2], [x3, y3], [x4, y4]], + x, + y + ); + + index += 1; + } + + return eventOccurredInHighlight; } /** @@ -477,7 +505,11 @@ const HOVER_TIMEOUT_MS = 75; * @return {HTMLElement} Page element */ getPageEl() { - return this.annotatedElement.querySelector(`[data-page-number="${this.location.page}"]`); + if (!this.pageEl) { + this.pageEl = this.annotatedElement.querySelector(`[data-page-number="${this.location.page}"]`); + } + + return this.pageEl; } /** diff --git a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js index de020fe68..f9f5e25c3 100644 --- a/src/lib/annotations/doc/__tests__/DocAnnotator-test.js +++ b/src/lib/annotations/doc/__tests__/DocAnnotator-test.js @@ -356,6 +356,19 @@ describe('lib/annotations/doc/DocAnnotator', () => { stubs.elMock.expects('removeEventListener').withArgs('dblclick', sinon.match.func); annotator.unbindDOMListeners(); }); + + it('should stop and destroy the requestAnimationFrame handle created by getHighlightMousemoveHandler()', () => { + const rafHandle = 12; // RAF handles are integers + annotator.annotationService.canAnnotate = true; + annotator.highlightThrottleHandle = rafHandle; + sandbox.stub(annotator, 'getHighlightMouseMoveHandler').returns(sandbox.stub()); + + const cancelRAFStub = sandbox.stub(window, 'cancelAnimationFrame'); + annotator.unbindDOMListeners(); + + expect(cancelRAFStub).to.be.calledWith(rafHandle); + expect(annotator.highlightThrottleHandle).to.not.exist; + }); }); describe('bindCustomListenersOnThread()', () => { @@ -424,10 +437,66 @@ describe('lib/annotations/doc/DocAnnotator', () => { }); }); - describe('highlightMousemoveHandler()', () => { + describe('getHighlightMouseMoveHandler()', () => { beforeEach(() => { - annotator.throttledHighlightMousemoveHandler = false; + annotator.highlightMousemoveHandler = false; + + // Request animation frame stub + stubs.RAF = sandbox.stub(window, 'requestAnimationFrame'); + }); + + it('should do nothing if the highlightMousemoveHandler already exists', () => { + annotator.highlightMousemoveHandler = true; + const result = annotator.getHighlightMouseMoveHandler(); + + expect(stubs.RAF).to.not.be.called; + expect(result).to.be.true; + }); + }); + + describe('onHighlightMouseMove()', () => { + it('should set didMouseMove to true if the mouse was moved enough', () => { + annotator.mouseX = 0; + annotator.mouseY = 0; + + annotator.onHighlightMouseMove({ clientX: 10, clientY: 10 }); + + expect(annotator.didMouseMove).to.equal(true); + }); + + it('should not set didMouseMove to true if the mouse was not moved enough', () => { + annotator.mouseX = 0; + annotator.mouseY = 0; + + annotator.onHighlightMouseMove({ clientX: 3, clientY: 3 }); + expect(annotator.didMouseMove).to.equal(undefined); + }); + + it('should assign the mouseMoveEvent if the annotator is highlighting', () => { + const moveEvent = { clientX: 10, clientY: 10 }; + annotator.mouseX = 0; + annotator.mouseY = 0; + + annotator.onHighlightMouseMove(moveEvent); + + expect(annotator.mouseMoveEvent).to.deep.equal(moveEvent); + }); + + it('should not assign the mouseMoveEvent if the annotator is highlighting', () => { + const moveEvent = { clientX: 10, clientY: 10 }; + annotator.mouseX = 0; + annotator.mouseY = 0; + annotator.isCreatingHighlight = true; + + annotator.onHighlightMouseMove(moveEvent); + + expect(annotator.mouseMoveEvent).to.not.deep.equal(moveEvent); + }); + }); + + describe('onHighlightCheck()', () => { + beforeEach(() => { stubs.thread = { onMousemove: () => {}, show: () => {} @@ -444,40 +513,26 @@ describe('lib/annotations/doc/DocAnnotator', () => { stubs.getPage = stubs.getPage.returns({ pageEl: {}, page: 1 }); stubs.getThreads = sandbox.stub(annotator, 'getHighlightThreadsOnPage'); - stubs.clock = sinon.useFakeTimers(); + + let timer = 0; + window.performance = window.performance || { now: () => {} }; + sandbox.stub(window.performance, 'now', () => { + return (timer += 500); + }); }); afterEach(() => { stubs.clock.restore(); }); - it('should do nothing if the throttledHighlightMousemoveHandler already exists', () => { - annotator.throttledHighlightMousemoveHandler = true; - stubs.threadMock.expects('onMousemove').returns(false).never(); - stubs.delayMock.expects('onMousemove').returns(true).never(); - - const result = annotator.highlightMousemoveHandler(); - - expect(stubs.getThreads).to.not.be.called; - expect(result).to.be.true; - }); - - it('should do nothing if there are pending, pending-active, active, or active hover highlight threads', () => { - stubs.thread.state = constants.ANNOTATION_STATE_PENDING; - stubs.getThreads.returns([stubs.thread]); - - const result = annotator.highlightMousemoveHandler()({ x: 1, y: 2 }); - - expect(stubs.getThreads).to.be.called; - expect(result).to.equal(undefined); - }); - it('should not add any delayThreads if there are no threads on the current page', () => { stubs.threadMock.expects('onMousemove').returns(false).never(); stubs.delayMock.expects('onMousemove').returns(true).never(); stubs.getThreads.returns([]); - annotator.highlightMousemoveHandler()({ clientX: 3, clientY: 3 }); + + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); }); it('should add delayThreads and hide innactive threads if the page is found', () => { @@ -487,7 +542,8 @@ describe('lib/annotations/doc/DocAnnotator', () => { stubs.threadMock.expects('show').never(); stubs.delayMock.expects('show'); - annotator.highlightMousemoveHandler()({ clientX: 3, clientY: 3 }); + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); }); it('should not trigger other highlights if user is creating a new highlight', () => { @@ -495,27 +551,9 @@ describe('lib/annotations/doc/DocAnnotator', () => { annotator.isCreatingHighlight = true; stubs.delayMock.expects('show').never(); stubs.delayMock.expects('hideDialog').never(); - annotator.highlightMousemoveHandler()({ clientX: 3, clientY: 3 }); - }); - - it('should set _didMouseMove to true if the mouse was moved enough', () => { - stubs.getThreads.returns([]); - annotator.mouseX = 0; - annotator.mouseY = 0; - - annotator.highlightMousemoveHandler()({ clientX: 10, clientY: 10 }); - - expect(annotator.didMouseMove).to.equal(true); - }); - it('should not set _didMouseMove to true if the mouse was not moved enough', () => { - stubs.getThreads.returns([]); - annotator.mouseX = 0; - annotator.mouseY = 0; - - annotator.highlightMousemoveHandler()({ clientX: 3, clientY: 3 }); - - expect(annotator.didMouseMove).to.equal(undefined); + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); }); it('should switch to the text cursor if mouse is no longer hovering over a highlight', () => { @@ -523,20 +561,47 @@ describe('lib/annotations/doc/DocAnnotator', () => { stubs.getThreads.returns([stubs.delayThread]); sandbox.stub(annotator, 'removeDefaultCursor'); - annotator.highlightMousemoveHandler()({ clientX: 3, clientY: 3 }); + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); + expect(annotator.removeDefaultCursor).to.not.be.called; stubs.clock.tick(75); expect(annotator.removeDefaultCursor).to.be.called; }); + it('should switch to the hand cursor if mouse is hovering over a highlight', () => { + stubs.delayMock.expects('onMousemove').returns(true); + stubs.getThreads.returns([stubs.delayThread]); + sandbox.stub(annotator, 'useDefaultCursor'); + + stubs.delayThread.state = constants.ANNOTATION_STATE_ACTIVE_HOVER; + + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); + + expect(annotator.useDefaultCursor).to.be.called; + }); + it('should show the top-most delayed thread, and hide all others', () => { stubs.getThreads.returns([stubs.delayThread, stubs.delayThread]); stubs.delayMock.expects('onMousemove').returns(true).twice(); stubs.delayMock.expects('show'); stubs.delayMock.expects('hideDialog'); - annotator.highlightMousemoveHandler()({ clientX: 3, clientY: 3 }); + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); + }); + + it('should do nothing if there are pending, pending-active, active, or active hover highlight threads', () => { + stubs.thread.state = constants.ANNOTATION_STATE_PENDING; + stubs.threadMock.expects('onMousemove').returns(false).never(); + stubs.getThreads.returns([stubs.thread]); + + annotator.mouseMoveEvent = { clientX: 3, clientY: 3 }; + annotator.onHighlightCheck(); + + expect(stubs.getThreads).to.be.called; }); }); diff --git a/src/lib/annotations/doc/__tests__/DocHighlightThread-test.js b/src/lib/annotations/doc/__tests__/DocHighlightThread-test.js index 12bdec60e..bf1692a9a 100644 --- a/src/lib/annotations/doc/__tests__/DocHighlightThread-test.js +++ b/src/lib/annotations/doc/__tests__/DocHighlightThread-test.js @@ -480,9 +480,7 @@ describe('lib/annotations/doc/DocHighlightThread', () => { pageEl.getBoundingClientRect.returns({ height: 0, top: 10 }); const pageElStub = sandbox.stub(highlightThread, 'getPageEl').returns(pageEl); const dimensionScaleStub = sandbox.stub(annotatorUtil, 'getDimensionScale').returns(false); - const quadPoint = { - map: sandbox.stub() - }; + const quadPoint = {}; highlightThread.location.quadPoints = [quadPoint, quadPoint, quadPoint]; const convertStub = sandbox .stub(docAnnotatorUtil, 'convertPDFSpaceToDOMSpace') @@ -492,7 +490,6 @@ describe('lib/annotations/doc/DocHighlightThread', () => { expect(pageElStub).to.be.called; expect(pageEl.getBoundingClientRect).to.be.called; expect(dimensionScaleStub).to.be.called; - expect(quadPoint.map).to.not.be.called; expect(convertStub).to.be.called; }); @@ -503,9 +500,7 @@ describe('lib/annotations/doc/DocHighlightThread', () => { pageEl.getBoundingClientRect.returns({ height: 0, top: 10 }); const pageElStub = sandbox.stub(highlightThread, 'getPageEl').returns(pageEl); const dimensionScaleStub = sandbox.stub(annotatorUtil, 'getDimensionScale').returns(true); - const quadPoint = { - map: sandbox.stub() - }; + const quadPoint = {}; highlightThread.location.quadPoints = [quadPoint, quadPoint, quadPoint]; const convertStub = sandbox .stub(docAnnotatorUtil, 'convertPDFSpaceToDOMSpace') @@ -515,7 +510,6 @@ describe('lib/annotations/doc/DocHighlightThread', () => { expect(pageElStub).to.be.called; expect(pageEl.getBoundingClientRect).to.be.called; expect(dimensionScaleStub).to.be.called; - expect(quadPoint.map).to.be.called; expect(convertStub).to.be.called; }); @@ -526,9 +520,7 @@ describe('lib/annotations/doc/DocHighlightThread', () => { pageEl.getBoundingClientRect.returns({ height: 0, top: 10 }); const pageElStub = sandbox.stub(highlightThread, 'getPageEl').returns(pageEl); const dimensionScaleStub = sandbox.stub(annotatorUtil, 'getDimensionScale').returns(false); - const quadPoint = { - map: sandbox.stub() - }; + const quadPoint = {}; highlightThread.location.quadPoints = [quadPoint, quadPoint, quadPoint]; const convertStub = sandbox .stub(docAnnotatorUtil, 'convertPDFSpaceToDOMSpace') @@ -539,7 +531,6 @@ describe('lib/annotations/doc/DocHighlightThread', () => { expect(pageElStub).to.be.called; expect(pageEl.getBoundingClientRect).to.be.called; expect(dimensionScaleStub).to.be.called; - expect(quadPoint.map).to.not.be.called; expect(convertStub).to.be.called; expect(pointInPolyStub).to.be.called; }); diff --git a/src/lib/annotations/doc/docAnnotatorUtil.js b/src/lib/annotations/doc/docAnnotatorUtil.js index 7f9186171..d20bbe00d 100644 --- a/src/lib/annotations/doc/docAnnotatorUtil.js +++ b/src/lib/annotations/doc/docAnnotatorUtil.js @@ -272,7 +272,7 @@ export function getQuadPoints(element, pageEl, scale) { // Cleanup helper container element.removeChild(quadCornerContainerEl); - // Calculate coordinates of these 4 corners + // Calculate coordinates of these 4 corners. const quadPoints = [ corner1Dimensions.left - pageLeft, corner1Dimensions.top - pageTop,