From 40fe6c711b81e0db2bb861d9c4366ae2439d38b5 Mon Sep 17 00:00:00 2001 From: Jared Stoffan Date: Tue, 13 Aug 2019 14:57:59 -0700 Subject: [PATCH] feat(pdf): Refactor find controller to use event bus to update UI --- .travis.yml | 4 ++ src/lib/util.js | 23 ++++-- src/lib/viewers/doc/DocBaseViewer.js | 41 +++++++++-- src/lib/viewers/doc/DocFindBar.js | 46 ++++++------ src/lib/viewers/doc/SinglePageViewer.js | 2 +- .../doc/__tests__/DocBaseViewer-test.js | 30 ++++++-- .../viewers/doc/__tests__/DocFindBar-test.js | 72 ++++++++----------- 7 files changed, 128 insertions(+), 90 deletions(-) diff --git a/.travis.yml b/.travis.yml index cf52a3d958..2860c44aef 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,6 +1,10 @@ language: node_js node_js: - '10' +addons: + apt: + packages: + - libgconf-2-4 cache: yarn: true directories: diff --git a/src/lib/util.js b/src/lib/util.js index 8579992948..c9b8f6cc90 100644 --- a/src/lib/util.js +++ b/src/lib/util.js @@ -13,6 +13,18 @@ const HEADER_CLIENT_NAME = 'X-Box-Client-Name'; const HEADER_CLIENT_VERSION = 'X-Box-Client-Version'; const PROMISE_MAP = {}; +/** + * Clears the promise map of any active promises + * + * @private + * @return {void} + */ +export function clearPromises() { + Object.keys(PROMISE_MAP).forEach(promiseKey => { + delete PROMISE_MAP[promiseKey]; + }); +} + /** * Creates an empty iframe or uses an existing one * for the purposes of downloading or printing @@ -339,7 +351,7 @@ export function loadScripts(urls, disableAMD = false) { } urls.forEach(url => { - if (!PROMISE_MAP[url]) { + if (!head.querySelector(`script[src="${url}"]`) && !PROMISE_MAP[url]) { const script = createScript(url); PROMISE_MAP[url] = new Promise((resolve, reject) => { script.addEventListener('load', resolve); @@ -357,16 +369,15 @@ export function loadScripts(urls, disableAMD = false) { if (disableAMD && amdPresent) { define = defineRef; } + + clearPromises(); }) .catch(() => { if (disableAMD && amdPresent) { define = defineRef; } - }) - .finally(() => { - Object.keys(PROMISE_MAP).forEach(promiseKey => { - delete PROMISE_MAP[promiseKey]; - }); + + clearPromises(); }); } diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index 2a13bf0dce..f4d5589e40 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -149,6 +149,7 @@ class DocBaseViewer extends BaseViewer { */ destroy() { this.unbindDOMListeners(); + this.unbindEventBusListeners(); // Clean up print blob this.printBlob = null; @@ -207,6 +208,31 @@ class DocBaseViewer extends BaseViewer { super.destroy(); } + /** + * Unbinds all events on the internal PDFJS event bus + * + * @private + * @return {void} + */ + unbindEventBusListeners() { + if (!this.pdfEventBus) { + return; + } + + const eventBusListeners = this.pdfEventBus._listeners || {}; + + // EventBus does not have a destroy method, so iterate over and remove all subscribed event handlers + Object.keys(eventBusListeners).forEach(eventName => { + const eventListeners = eventBusListeners[eventName]; + + if (Array.isArray(eventListeners)) { + eventListeners.forEach(eventListener => { + this.pdfEventBus.off(eventName, eventListener); + }); + } + }); + } + /** * Converts a value and unit to page number * @@ -583,18 +609,19 @@ class DocBaseViewer extends BaseViewer { initViewer(pdfUrl) { this.bindDOMListeners(); - this.pdfJsEventBus = new this.pdfjsViewer.EventBus(); - this.pdfJsEventBus.on('pagechanging', this.pagechangingHandler); - this.pdfJsEventBus.on('pagerendered', this.pagerenderedHandler); - this.pdfJsEventBus.on('pagesinit', this.pagesinitHandler); + this.pdfEventBus = new this.pdfjsViewer.EventBus(); + this.pdfEventBus.on('pagechanging', this.pagechangingHandler); + this.pdfEventBus.on('pagerendered', this.pagerenderedHandler); + this.pdfEventBus.on('pagesinit', this.pagesinitHandler); this.pdfLinkService = new this.pdfjsViewer.PDFLinkService({ - eventBus: this.pdfJsEventBus, + eventBus: this.pdfEventBus, externalLinkRel: 'noopener noreferrer nofollow', // Prevent referrer hijacking externalLinkTarget: this.pdfjsLib.LinkTarget.BLANK, // Open links in new tab }); this.pdfFindController = new this.pdfjsViewer.PDFFindController({ + eventBus: this.pdfEventBus, linkService: this.pdfLinkService, }); @@ -686,7 +713,7 @@ class DocBaseViewer extends BaseViewer { return new this.pdfjsViewer.PDFViewer({ container: this.docEl, enhanceTextSelection: !this.isMobile, // Uses more memory, so disable on mobile - eventBus: this.pdfJsEventBus, + eventBus: this.pdfEventBus, findController: this.pdfFindController, linkService: this.pdfLinkService, }); @@ -710,7 +737,7 @@ class DocBaseViewer extends BaseViewer { return; } - this.findBar = new DocFindBar(this.findBarEl, this.pdfFindController, canDownload); + this.findBar = new DocFindBar(this.findBarEl, this.pdfFindController, this.pdfEventBus, canDownload); this.findBar.addListener(VIEWER_EVENT.metric, this.emitMetric); } diff --git a/src/lib/viewers/doc/DocFindBar.js b/src/lib/viewers/doc/DocFindBar.js index 5ed393af31..bd763f6769 100644 --- a/src/lib/viewers/doc/DocFindBar.js +++ b/src/lib/viewers/doc/DocFindBar.js @@ -19,18 +19,23 @@ class DocFindBar extends EventEmitter { * * @param {string|HTMLElement} findBar - Find bar selector or element * @param {Object} findController - Document find controller to use + * @param {Object} eventBus - Document event bus to use * @param {boolean} canDownload - Whether user can download document or not * @return {DocFindBar} DocFindBar instance */ - constructor(findBar, findController, canDownload) { + constructor(findBar, findController, eventBus, canDownload) { super(); this.opened = false; this.bar = findBar; + this.eventBus = eventBus; this.findController = findController; - this.currentMatch = 0; this.canDownload = canDownload; + if (this.eventBus === null) { + throw new Error('DocFindBar cannot be used without an EventBus instance.'); + } + if (this.findController === null) { throw new Error('DocFindBar cannot be used without a PDFFindController instance.'); } @@ -43,9 +48,9 @@ class DocFindBar extends EventEmitter { this.findPreviousHandler = this.findPreviousHandler.bind(this); this.close = this.close.bind(this); - // overriding some find controller methods to update match count - this.findController.updateUIState = this.updateUIState.bind(this); - this.findController.updateUIResultsCount = this.updateUIResultsCount.bind(this); + // attaching some listeners to update match count + this.eventBus.on('updatefindcontrolstate', this.updateUIState.bind(this)); + this.eventBus.on('updatefindmatchescount', this.updateUIResultsCount.bind(this)); // Default hides find bar on load this.bar.classList.add(CLASS_HIDDEN); @@ -108,7 +113,6 @@ class DocFindBar extends EventEmitter { * @return {void} */ destroy() { - this.currentMatch = 0; this.removeAllListeners(); this.unbindDOMListeners(); @@ -136,10 +140,11 @@ class DocFindBar extends EventEmitter { /** * Update Find Bar UI to current match state * - * @param {number} state FindState from PDFFindController + * @param {number} matchesCount - total number of matches from find controller + * @param {number} state - Find state from find controller * @return {void} */ - updateUIState(state) { + updateUIState({ matchesCount, state }) { this.status = ''; switch (state) { @@ -160,21 +165,24 @@ class DocFindBar extends EventEmitter { } this.findFieldEl.setAttribute('data-status', this.status); - this.updateUIResultsCount(); + this.updateUIResultsCount({ matchesCount }); } /** * Update results count to current match count * + * @param {Object} matchesCount - matches from find controller + * @param {number} matchesCount.current - current match index + * @param {number} matchesCount.total - current total number of matches * @return {void} */ - updateUIResultsCount() { + updateUIResultsCount({ matchesCount }) { if (!this.findResultsCountEl) { return; // no UI control is provided } // If there are no matches, hide the counter - if (!this.findController.matchCount) { + if (!matchesCount || !matchesCount.total) { this.findResultsCountEl.classList.add(CLASS_HIDDEN); return; } @@ -184,7 +192,7 @@ class DocFindBar extends EventEmitter { this.findFieldEl.style.paddingRight = `${paddingRight}px`; // Create the match counter - this.findResultsCountEl.textContent = this.currentMatch + MATCH_SEPARATOR + this.findController.matchCount; + this.findResultsCountEl.textContent = `${matchesCount.current} ${MATCH_SEPARATOR} ${matchesCount.total}`; // Show the counter this.findResultsCountEl.classList.remove(CLASS_HIDDEN); @@ -269,7 +277,6 @@ class DocFindBar extends EventEmitter { */ findFieldHandler() { this.dispatchFindEvent('find'); - this.currentMatch = 1; } /** @@ -323,12 +330,6 @@ class DocFindBar extends EventEmitter { this.findNextButtonEl.focus(); } else { this.dispatchFindEvent('findagain', false); - this.currentMatch = this.currentMatch + 1; - - // Loops search to first match in document - if (this.currentMatch > this.findController.matchCount) { - this.currentMatch = 1; - } } // Emit a metric that the user navigated forward in the find bar @@ -351,12 +352,6 @@ class DocFindBar extends EventEmitter { this.findPreviousButtonEl.focus(); } else { this.dispatchFindEvent('findagain', true); - this.currentMatch = this.currentMatch - 1; - - // Loops search to last match in document - if (this.currentMatch <= 0) { - this.currentMatch = this.findController.matchCount; - } } // Emit a metric that the user navigated back in the find bar @@ -408,7 +403,6 @@ class DocFindBar extends EventEmitter { } this.opened = false; this.bar.classList.add(CLASS_HIDDEN); - this.findController.active = false; } } diff --git a/src/lib/viewers/doc/SinglePageViewer.js b/src/lib/viewers/doc/SinglePageViewer.js index 0cb827059d..77dfef14e8 100644 --- a/src/lib/viewers/doc/SinglePageViewer.js +++ b/src/lib/viewers/doc/SinglePageViewer.js @@ -16,7 +16,7 @@ class SinglePageViewer extends DocumentViewer { return new this.pdfjsViewer.PDFSinglePageViewer({ container: this.docEl, enhanceTextSelection: !this.isMobile, // Uses more memory, so disable on mobile - eventBus: this.pdfJsEventBus, + eventBus: this.pdfEventBus, findController: this.pdfFindController, linkService: this.pdfLinkService, }); diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js index f2816ff958..f0e4b08044 100644 --- a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js @@ -255,11 +255,13 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { describe('destroy()', () => { it('should unbind listeners and clear the print blob', () => { const unbindDOMListenersStub = sandbox.stub(docBase, 'unbindDOMListeners'); + const unbindEventBusListenersStub = sandbox.stub(docBase, 'unbindEventBusListeners'); docBase.printURL = 'someblob'; sandbox.stub(URL, 'revokeObjectURL'); docBase.destroy(); expect(unbindDOMListenersStub).to.be.called; + expect(unbindEventBusListenersStub).to.be.called; expect(docBase.printBlob).to.equal(null); expect(URL.revokeObjectURL).to.be.calledWith(docBase.printURL); }); @@ -647,9 +649,12 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { }); }); - describe.skip('initFind()', () => { - // TODO: FIX TESTS + describe('initFind()', () => { beforeEach(() => { + docBase.pdfEventBus = { + off: sandbox.stub(), + on: sandbox.stub(), + }; docBase.pdfViewer = { setFindController: sandbox.stub(), }; @@ -661,11 +666,6 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { expect(docBase.docEl.parentNode).to.deep.equal(docBase.containerEl); }); - it('should create and set a new findController', () => { - docBase.initFind(); - expect(docBase.pdfViewer.setFindController).to.be.called; - }); - it('should not set find bar if viewer option disableFindBar is true', () => { sandbox .stub(docBase, 'getViewerOption') @@ -1752,6 +1752,22 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { }); }); + describe('unbindEventBusListeners', () => { + it('should remove all the event listeners on the internal PDFJS event bus', () => { + docBase.pdfEventBus = { + _listeners: { + event1: [() => {}], + event2: [() => {}, () => {}], + }, + off: sandbox.stub(), + }; + + docBase.unbindEventBusListeners(); + + expect(docBase.pdfEventBus.off).to.have.callCount(3); + }); + }); + describe('pagesinitHandler()', () => { beforeEach(() => { stubs.loadUI = sandbox.stub(docBase, 'loadUI'); diff --git a/src/lib/viewers/doc/__tests__/DocFindBar-test.js b/src/lib/viewers/doc/__tests__/DocFindBar-test.js index 665ae5b24e..d9d2a72272 100644 --- a/src/lib/viewers/doc/__tests__/DocFindBar-test.js +++ b/src/lib/viewers/doc/__tests__/DocFindBar-test.js @@ -15,6 +15,7 @@ const MATCH_OFFSET = 13; const sandbox = sinon.sandbox.create(); let docFindBar; +let eventBus; let findBarEl; let findController; let stubs = {}; @@ -27,12 +28,13 @@ describe('lib/viewers/doc/DocFindBar', () => { beforeEach(() => { fixture.load('viewers/doc/__tests__/DocFindBar-test.html'); + eventBus = { off: sandbox.stub, on: sandbox.stub() }; findBarEl = document.querySelector('.test-find-bar'); findController = { executeCommand: sandbox.stub(), linkService: {}, }; - docFindBar = new DocFindBar(findBarEl, findController, true); + docFindBar = new DocFindBar(findBarEl, findController, eventBus, true); }); afterEach(() => { @@ -52,16 +54,26 @@ describe('lib/viewers/doc/DocFindBar', () => { it('should correctly set the object parameters', () => { expect(docFindBar.opened).to.be.false; expect(docFindBar.bar).to.equal(findBarEl); + expect(docFindBar.eventBus).to.equal(eventBus); expect(docFindBar.findController).to.equal(findController); - expect(docFindBar.currentMatch).to.equal(0); expect(docFindBar.canDownload).to.be.true; }); + it('should throw an error if there is no eventBus', () => { + docFindBar.destroy(); + eventBus = null; + try { + docFindBar = new DocFindBar(findBarEl, findController, eventBus); + } catch (e) { + expect(e.message).to.equal('DocFindBar cannot be used without an EventBus instance.'); + } + }); + it('should throw an error if there is no findController', () => { docFindBar.destroy(); findController = null; try { - docFindBar = new DocFindBar(findBarEl, findController); + docFindBar = new DocFindBar(findBarEl, findController, eventBus); } catch (e) { expect(e.message).to.equal('DocFindBar cannot be used without a PDFFindController instance.'); } @@ -113,10 +125,9 @@ describe('lib/viewers/doc/DocFindBar', () => { stubs.removeChild = sandbox.stub(docFindBar.bar.parentNode, 'removeChild'); }); - it('should reset the current match, and unbind DOM listeners', () => { + it('should unbind DOM listeners', () => { docFindBar.destroy(); - expect(docFindBar.currentMatch).to.equal(0); expect(stubs.unbindDOMListeners).to.be.called; }); @@ -155,7 +166,7 @@ describe('lib/viewers/doc/DocFindBar', () => { }); it('should update the status and add the correct class if the match is not found', () => { - docFindBar.updateUIState(FIND_MATCH_NOT_FOUND); + docFindBar.updateUIState({ state: FIND_MATCH_NOT_FOUND }); expect(docFindBar.status).to.equal(''); expect(docFindBar.findFieldEl.classList.contains(CLASS_FIND_MATCH_NOT_FOUND)).to.be.true; @@ -164,7 +175,7 @@ describe('lib/viewers/doc/DocFindBar', () => { }); it('should update the status if the status is pending', () => { - docFindBar.updateUIState(FIND_MATCH_PENDING); + docFindBar.updateUIState({ state: FIND_MATCH_PENDING }); expect(docFindBar.status).to.equal('pending'); expect(docFindBar.findFieldEl.getAttribute('data-status')).to.equal('pending'); @@ -172,7 +183,7 @@ describe('lib/viewers/doc/DocFindBar', () => { }); it('should update the status and add the correct class if the status is found', () => { - docFindBar.updateUIState(FIND_MATCH_FOUND); + docFindBar.updateUIState({ state: FIND_MATCH_FOUND }); expect(docFindBar.status).to.equal(''); expect(docFindBar.findFieldEl.classList.contains(CLASS_FIND_MATCH_NOT_FOUND)).to.be.false; @@ -191,27 +202,25 @@ describe('lib/viewers/doc/DocFindBar', () => { it('should do nothing if there is no find results count element', () => { docFindBar.findResultsCountEl = undefined; - docFindBar.updateUIResultsCount(); + docFindBar.updateUIResultsCount({ matchesCount: { current: 1, total: 2 } }); expect(stubs.getBoundingClientRect).to.not.be.called; }); it('should hide the counter if there are no matches', () => { - docFindBar.findController.matchCount = undefined; + docFindBar.updateUIResultsCount({ matchesCount: { current: 0, total: 0 } }); - docFindBar.updateUIResultsCount(); expect(docFindBar.findResultsCountEl.classList.contains(CLASS_HIDDEN)).to.be.true; expect(stubs.getBoundingClientRect).to.not.be.called; }); it('should adjust padding, and create/show the counter', () => { - docFindBar.findController.matchCount = 1; - let paddingRight = 5 + MATCH_OFFSET; - paddingRight = `${paddingRight}px`; + const paddingRight = `${5 + MATCH_OFFSET}px`; + + docFindBar.updateUIResultsCount({ matchesCount: { current: 1, total: 2 } }); - docFindBar.updateUIResultsCount(); expect(docFindBar.findFieldEl.style.paddingRight).to.be.equal(paddingRight); - expect(stubs.getBoundingClientRect).to.be.called; expect(docFindBar.findResultsCountEl.classList.contains(CLASS_HIDDEN)).to.be.false; + expect(stubs.getBoundingClientRect).to.be.called; }); }); @@ -333,12 +342,11 @@ describe('lib/viewers/doc/DocFindBar', () => { }); describe('findFieldHandler()', () => { - it('should dispatch the find event, and set current match to 1', () => { + it('should dispatch the find event', () => { const dispatchFindEventStub = sandbox.stub(docFindBar, 'dispatchFindEvent'); docFindBar.findFieldHandler(); expect(dispatchFindEventStub).to.be.calledWith('find'); - expect(docFindBar.currentMatch).to.equal(1); }); }); @@ -434,7 +442,6 @@ describe('lib/viewers/doc/DocFindBar', () => { stubs.dispatchFindEvent = sandbox.stub(docFindBar, 'dispatchFindEvent'); docFindBar.findFieldEl.value = 'test'; docFindBar.findController.matchCount = 1; - docFindBar.currentMatch = 0; }); it('should do nothing if there is nothing to find', () => { @@ -457,22 +464,12 @@ describe('lib/viewers/doc/DocFindBar', () => { expect(stubs.dispatchFindEvent).to.be.called; }); - it('should go back to the first match if the next button is clicked when on the last match', () => { - docFindBar.findFieldEl.value = 'test'; - docFindBar.findController.matchCount = 1; - docFindBar.currentMatch = 2; - - docFindBar.findNextHandler(true); - expect(docFindBar.currentMatch).to.equal(1); - }); - it('should emit the find next event', () => { sandbox.stub(docFindBar, 'emit'); - docFindBar.findFieldEl.value = 'test'; - docFindBar.findController.matchCount = 1; - docFindBar.currentMatch = 2; + docFindBar.findFieldEl.value = 'test'; docFindBar.findNextHandler(true); + expect(docFindBar.emit).to.be.calledWith(VIEWER_EVENT.metric, { name: USER_DOCUMENT_FIND_EVENTS.NEXT, }); @@ -485,13 +482,12 @@ describe('lib/viewers/doc/DocFindBar', () => { stubs.dispatchFindEvent = sandbox.stub(docFindBar, 'dispatchFindEvent'); docFindBar.findFieldEl.value = 'test'; docFindBar.findController.matchCount = 5; - docFindBar.currentMatch = 0; }); it('should do nothing if there is nothing to find', () => { docFindBar.findFieldEl.value = ''; - docFindBar.findPreviousHandler(false); + expect(stubs.focus).to.not.be.called; expect(stubs.dispatchFindEvent).to.not.be.called; }); @@ -508,18 +504,9 @@ describe('lib/viewers/doc/DocFindBar', () => { expect(stubs.dispatchFindEvent).to.be.called; }); - it('should go back to the first match if the previous button is clicked when on the last match', () => { - docFindBar.findFieldEl.value = 'test'; - docFindBar.currentMatch = 0; - - docFindBar.findPreviousHandler(true); - expect(docFindBar.currentMatch).to.equal(5); - }); - it('should emit a find previous metric', () => { sandbox.stub(docFindBar, 'emit'); docFindBar.findFieldEl.value = 'test'; - docFindBar.currentMatch = 0; docFindBar.findPreviousHandler(true); expect(docFindBar.emit).to.be.calledWith(VIEWER_EVENT.metric, { @@ -602,7 +589,6 @@ describe('lib/viewers/doc/DocFindBar', () => { docFindBar.close(); expect(docFindBar.opened).to.equal(false); expect(stubs.add).to.be.calledWith(CLASS_HIDDEN); - expect(docFindBar.findController.active).to.equal(false); }); }); });