From 36ff93ac46ddb446ff79a34ed2bf735d5cd0f0aa Mon Sep 17 00:00:00 2001 From: Jared Stoffan Date: Wed, 20 Nov 2019 13:02:26 -0800 Subject: [PATCH] fix(find): Find bar container should only be created when needed --- src/lib/viewers/doc/DocBaseViewer.js | 7 +- src/lib/viewers/doc/DocFindBar.js | 62 ++++++++-------- .../doc/__tests__/DocBaseViewer-test.js | 10 +-- .../doc/__tests__/DocFindBar-test.html | 4 +- .../viewers/doc/__tests__/DocFindBar-test.js | 70 +++++++++++-------- 5 files changed, 78 insertions(+), 75 deletions(-) diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js index 125a6eda6..88e9f52f8 100644 --- a/src/lib/viewers/doc/DocBaseViewer.js +++ b/src/lib/viewers/doc/DocBaseViewer.js @@ -11,7 +11,6 @@ import PreviewError from '../../PreviewError'; import ThumbnailsSidebar from '../../ThumbnailsSidebar'; import { ANNOTATOR_EVENT, - CLASS_BOX_PREVIEW_FIND_BAR, CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE_ACTIVE, CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE, CLASS_BOX_PREVIEW_THUMBNAILS_CONTAINER, @@ -768,10 +767,6 @@ class DocBaseViewer extends BaseViewer { * @return {void} */ initFind() { - this.findBarEl = this.containerEl.appendChild(document.createElement('div')); - this.findBarEl.classList.add(CLASS_BOX_PREVIEW_FIND_BAR); - this.findBarEl.setAttribute('data-testid', 'document-findbar'); - // Only initialize the find bar if the user has download permissions on // the file. Users without download permissions shouldn't be able to // interact with the text layer @@ -779,7 +774,7 @@ class DocBaseViewer extends BaseViewer { return; } - this.findBar = new DocFindBar(this.findBarEl, this.pdfFindController, this.pdfEventBus); + this.findBar = new DocFindBar(this.containerEl, this.pdfFindController, this.pdfEventBus); this.findBar.addListener(VIEWER_EVENT.metric, this.emitMetric); this.findBar.addListener('close', this.handleFindBarClose); } diff --git a/src/lib/viewers/doc/DocFindBar.js b/src/lib/viewers/doc/DocFindBar.js index 350b2f3ec..456e87451 100644 --- a/src/lib/viewers/doc/DocFindBar.js +++ b/src/lib/viewers/doc/DocFindBar.js @@ -1,7 +1,7 @@ import EventEmitter from 'events'; import { decodeKeydown } from '../../util'; import { USER_DOCUMENT_FIND_EVENTS, VIEWER_EVENT } from '../../events'; -import { CLASS_HIDDEN } from '../../constants'; +import { CLASS_BOX_PREVIEW_FIND_BAR, CLASS_HIDDEN } from '../../constants'; import { ICON_FIND_DROP_DOWN, ICON_FIND_DROP_UP, ICON_CLOSE, ICON_SEARCH } from '../../icons/icons'; const MATCH_SEPARATOR = ' of '; @@ -17,48 +17,52 @@ class DocFindBar extends EventEmitter { /** * [constructor] * - * @param {string|HTMLElement} findBar - Find bar selector or element + * @param {Object} containerEl - Container element to which we append the find bar * @param {Object} findController - Document find controller to use * @param {Object} eventBus - Document event bus to use * @return {DocFindBar} DocFindBar instance */ - constructor(findBar, findController, eventBus) { + constructor(containerEl, findController, eventBus) { super(); - this.opened = false; - this.bar = findBar; - this.eventBus = eventBus; - this.findController = findController; + if (!containerEl) { + throw new Error('DocFindBar cannot be used without a container element.'); + } - if (this.eventBus === null) { + if (!eventBus) { throw new Error('DocFindBar cannot be used without an EventBus instance.'); } - if (this.findController === null) { + if (!findController) { throw new Error('DocFindBar cannot be used without a PDFFindController instance.'); } + this.eventBus = eventBus; + this.findBarEl = containerEl.appendChild(document.createElement('div')); + this.findBarEl.classList.add(CLASS_BOX_PREVIEW_FIND_BAR); + this.findBarEl.classList.add(CLASS_HIDDEN); // Default hides find bar on load + this.findBarEl.setAttribute('data-testid', 'document-findbar'); + this.findController = findController; + this.opened = false; + // Bind context for callbacks - this.onKeydown = this.onKeydown.bind(this); + this.close = this.close.bind(this); + this.findBarKeyDownHandler = this.findBarKeyDownHandler.bind(this); this.findFieldHandler = this.findFieldHandler.bind(this); - this.barKeyDownHandler = this.barKeyDownHandler.bind(this); this.findNextHandler = this.findNextHandler.bind(this); this.findPreviousHandler = this.findPreviousHandler.bind(this); - this.close = this.close.bind(this); + this.onKeydown = this.onKeydown.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); - this.createFindField(); this.createFindButtons(); - this.findPreviousButtonEl = this.bar.querySelector('.bp-doc-find-prev'); - this.findNextButtonEl = this.bar.querySelector('.bp-doc-find-next'); - this.findCloseButtonEl = this.bar.querySelector('.bp-doc-find-close'); + this.findCloseButtonEl = this.findBarEl.querySelector('.bp-doc-find-close'); + this.findNextButtonEl = this.findBarEl.querySelector('.bp-doc-find-next'); + this.findPreviousButtonEl = this.findBarEl.querySelector('.bp-doc-find-prev'); this.bindDOMListeners(); } @@ -73,18 +77,18 @@ class DocFindBar extends EventEmitter { const findSearchButtonEL = document.createElement('span'); findSearchButtonEL.classList.add('bp-doc-find-search'); findSearchButtonEL.innerHTML = ICON_SEARCH.trim(); - this.bar.appendChild(findSearchButtonEL); + this.findBarEl.appendChild(findSearchButtonEL); // Find input field this.findFieldEl = document.createElement('input'); this.findFieldEl.classList.add('bp-doc-find-field'); - this.bar.appendChild(this.findFieldEl); + this.findBarEl.appendChild(this.findFieldEl); // Match Results Count this.findResultsCountEl = document.createElement('span'); this.findResultsCountEl.classList.add('bp-doc-find-results-count'); this.findResultsCountEl.classList.add(CLASS_HIDDEN); - this.bar.appendChild(this.findResultsCountEl); + this.findBarEl.appendChild(this.findResultsCountEl); } /** @@ -106,7 +110,7 @@ class DocFindBar extends EventEmitter { this.findButtonContainerEl.classList.add('bp-doc-find-controls'); this.findButtonContainerEl.innerHTML = findPreviousButton + findNextButton + findCloseButton; - this.bar.appendChild(this.findButtonContainerEl); + this.findBarEl.appendChild(this.findButtonContainerEl); } /** @@ -119,8 +123,8 @@ class DocFindBar extends EventEmitter { this.removeAllListeners(); this.unbindDOMListeners(); - if (this.bar && this.bar.parentNode) { - this.bar.parentNode.removeChild(this.bar); + if (this.findBarEl && this.findBarEl.parentNode) { + this.findBarEl.parentNode.removeChild(this.findBarEl); } } @@ -221,7 +225,7 @@ class DocFindBar extends EventEmitter { * @return {void} */ bindDOMListeners() { - this.bar.addEventListener('keydown', this.barKeyDownHandler); + this.findBarEl.addEventListener('keydown', this.findBarKeyDownHandler); this.findFieldEl.addEventListener('input', this.findFieldHandler); this.findPreviousButtonEl.addEventListener('click', this.findPreviousHandler); this.findNextButtonEl.addEventListener('click', this.findNextHandler); @@ -235,7 +239,7 @@ class DocFindBar extends EventEmitter { * @return {void} */ unbindDOMListeners() { - this.bar.removeEventListener('keydown', this.barKeyDownHandler); + this.findBarEl.removeEventListener('keydown', this.findBarKeyDownHandler); this.findFieldEl.removeEventListener('input', this.findFieldHandler); this.findPreviousButtonEl.removeEventListener('click', this.findPreviousHandler); this.findNextButtonEl.removeEventListener('click', this.findNextHandler); @@ -287,7 +291,7 @@ class DocFindBar extends EventEmitter { * @param {Event} event - Key event * @return {void} */ - barKeyDownHandler(event) { + findBarKeyDownHandler(event) { const key = decodeKeydown(event).toLowerCase(); switch (key) { case 'enter': @@ -377,7 +381,7 @@ class DocFindBar extends EventEmitter { if (!this.opened) { this.opened = true; - this.bar.classList.remove(CLASS_HIDDEN); + this.findBarEl.classList.remove(CLASS_HIDDEN); // Emit a metric that the user opened the find bar this.emit(VIEWER_EVENT.metric, { name: USER_DOCUMENT_FIND_EVENTS.OPEN, @@ -404,7 +408,7 @@ class DocFindBar extends EventEmitter { } this.emit('close'); this.opened = false; - this.bar.classList.add(CLASS_HIDDEN); + this.findBarEl.classList.add(CLASS_HIDDEN); } /** diff --git a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js index 55001047f..12397aba9 100644 --- a/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js +++ b/src/lib/viewers/doc/__tests__/DocBaseViewer-test.js @@ -13,7 +13,6 @@ import * as file from '../../../file'; import * as util from '../../../util'; import { - CLASS_BOX_PREVIEW_FIND_BAR, CLASS_HIDDEN, PERMISSION_DOWNLOAD, STATUS_ERROR, @@ -657,17 +656,14 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => { off: sandbox.stub(), on: sandbox.stub(), }; + docBase.pdfFindController = { + execute: sandbox.stub(), + }; docBase.pdfViewer = { setFindController: sandbox.stub(), }; }); - it('should correctly set the find bar', () => { - docBase.initFind(); - expect(docBase.findBarEl.classList.contains(CLASS_BOX_PREVIEW_FIND_BAR)).to.be.true; - expect(docBase.docEl.parentNode).to.deep.equal(docBase.containerEl); - }); - it('should not set find bar if viewer option disableFindBar is true', () => { sandbox .stub(docBase, 'getViewerOption') diff --git a/src/lib/viewers/doc/__tests__/DocFindBar-test.html b/src/lib/viewers/doc/__tests__/DocFindBar-test.html index ddb8dcf62..e9b96d85d 100644 --- a/src/lib/viewers/doc/__tests__/DocFindBar-test.html +++ b/src/lib/viewers/doc/__tests__/DocFindBar-test.html @@ -1,3 +1 @@ -
-
-
+
diff --git a/src/lib/viewers/doc/__tests__/DocFindBar-test.js b/src/lib/viewers/doc/__tests__/DocFindBar-test.js index d212ba4f8..6e8020440 100644 --- a/src/lib/viewers/doc/__tests__/DocFindBar-test.js +++ b/src/lib/viewers/doc/__tests__/DocFindBar-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-unused-expressions */ -import DocFindBar from '../DocFindBar'; -import { CLASS_HIDDEN } from '../../../constants'; import * as util from '../../../util'; +import DocFindBar from '../DocFindBar'; +import { CLASS_BOX_PREVIEW_FIND_BAR, CLASS_HIDDEN } from '../../../constants'; import { VIEWER_EVENT, USER_DOCUMENT_FIND_EVENTS } from '../../../events'; const CLASS_FIND_MATCH_NOT_FOUND = 'bp-find-match-not-found'; @@ -14,9 +14,9 @@ const FIND_MATCH_PENDING = 3; const MATCH_OFFSET = 13; const sandbox = sinon.sandbox.create(); +let containerEl; let docFindBar; let eventBus; -let findBarEl; let findController; let stubs = {}; @@ -28,13 +28,13 @@ describe('lib/viewers/doc/DocFindBar', () => { beforeEach(() => { fixture.load('viewers/doc/__tests__/DocFindBar-test.html'); + containerEl = document.querySelector('.test-container'); eventBus = { off: sandbox.stub(), on: sandbox.stub() }; - findBarEl = document.querySelector('.test-find-bar'); findController = { executeCommand: sandbox.stub(), linkService: {}, }; - docFindBar = new DocFindBar(findBarEl, findController, eventBus, true); + docFindBar = new DocFindBar(containerEl, findController, eventBus); }); afterEach(() => { @@ -52,17 +52,27 @@ describe('lib/viewers/doc/DocFindBar', () => { describe('constructor()', () => { it('should correctly set the object parameters', () => { - expect(docFindBar.bar).to.equal(findBarEl); + expect(containerEl.querySelector(`.${CLASS_BOX_PREVIEW_FIND_BAR}`)).to.exist; expect(docFindBar.eventBus).to.equal(eventBus); expect(docFindBar.findController).to.equal(findController); expect(docFindBar.opened).to.be.false; }); + it('should throw an error if there is no container element', () => { + docFindBar.destroy(); + containerEl = null; + try { + docFindBar = new DocFindBar(containerEl, findController, eventBus); + } catch (e) { + expect(e.message).to.equal('DocFindBar cannot be used without a container element.'); + } + }); + it('should throw an error if there is no eventBus', () => { docFindBar.destroy(); eventBus = null; try { - docFindBar = new DocFindBar(findBarEl, findController, eventBus); + docFindBar = new DocFindBar(containerEl, findController, eventBus); } catch (e) { expect(e.message).to.equal('DocFindBar cannot be used without an EventBus instance.'); } @@ -72,7 +82,7 @@ describe('lib/viewers/doc/DocFindBar', () => { docFindBar.destroy(); findController = null; try { - docFindBar = new DocFindBar(findBarEl, findController, eventBus); + docFindBar = new DocFindBar(containerEl, findController, eventBus); } catch (e) { expect(e.message).to.equal('DocFindBar cannot be used without a PDFFindController instance.'); } @@ -85,7 +95,7 @@ describe('lib/viewers/doc/DocFindBar', () => { const searchIconEl = document.querySelector('.bp-doc-find-search'); - expect(searchIconEl.parentNode).to.equal(docFindBar.bar); + expect(searchIconEl.parentNode).to.equal(docFindBar.findBarEl); expect(searchIconEl.className).to.equal('bp-doc-find-search'); }); @@ -94,7 +104,7 @@ describe('lib/viewers/doc/DocFindBar', () => { const inputFieldEl = document.querySelector('.bp-doc-find-field'); - expect(inputFieldEl.parentNode).to.equal(docFindBar.bar); + expect(inputFieldEl.parentNode).to.equal(docFindBar.findBarEl); expect(inputFieldEl.className).to.equal('bp-doc-find-field'); }); @@ -103,7 +113,7 @@ describe('lib/viewers/doc/DocFindBar', () => { const resultsCountEl = document.querySelector('.bp-doc-find-results-count'); - expect(resultsCountEl.parentNode).to.equal(docFindBar.bar); + expect(resultsCountEl.parentNode).to.equal(docFindBar.findBarEl); expect(resultsCountEl.classList.contains('bp-doc-find-results-count')).to.be.true; expect(resultsCountEl.classList.contains(CLASS_HIDDEN)).to.be.true; }); @@ -114,14 +124,14 @@ describe('lib/viewers/doc/DocFindBar', () => { docFindBar.createFindButtons(); expect(docFindBar.findButtonContainerEl.classList.contains('bp-doc-find-controls')).to.be.true; - expect(docFindBar.findButtonContainerEl.parentNode).to.equal(docFindBar.bar); + expect(docFindBar.findButtonContainerEl.parentNode).to.equal(docFindBar.findBarEl); }); }); describe('destroy()', () => { beforeEach(() => { stubs.unbindDOMListeners = sandbox.stub(docFindBar, 'unbindDOMListeners'); - stubs.removeChild = sandbox.stub(docFindBar.bar.parentNode, 'removeChild'); + stubs.removeChild = sandbox.stub(docFindBar.findBarEl.parentNode, 'removeChild'); }); it('should unbind DOM listeners', () => { @@ -137,7 +147,7 @@ describe('lib/viewers/doc/DocFindBar', () => { }); it('should not remove the find bar if it does not exist', () => { - docFindBar.bar = undefined; + docFindBar.findBarEl = undefined; docFindBar.destroy(); expect(stubs.removeChild).to.not.be.called; @@ -237,14 +247,14 @@ describe('lib/viewers/doc/DocFindBar', () => { describe('bindDOMListeners()', () => { it('should add the correct event listeners', () => { - const barStub = sandbox.stub(docFindBar.bar, 'addEventListener'); + const barStub = sandbox.stub(docFindBar.findBarEl, 'addEventListener'); const findFieldStub = sandbox.stub(docFindBar.findFieldEl, 'addEventListener'); const findPrevStub = sandbox.stub(docFindBar.findPreviousButtonEl, 'addEventListener'); const findNextStub = sandbox.stub(docFindBar.findNextButtonEl, 'addEventListener'); const findCloseStub = sandbox.stub(docFindBar.findCloseButtonEl, 'addEventListener'); docFindBar.bindDOMListeners(); - expect(barStub).to.be.calledWith('keydown', docFindBar.barKeyDownHandler); + expect(barStub).to.be.calledWith('keydown', docFindBar.findBarKeyDownHandler); expect(findFieldStub).to.be.calledWith('input', docFindBar.findFieldHandler); expect(findPrevStub).to.be.calledWith('click', docFindBar.findPreviousHandler); expect(findNextStub).to.be.calledWith('click', docFindBar.findNextHandler); @@ -254,14 +264,14 @@ describe('lib/viewers/doc/DocFindBar', () => { describe('unbindDOMListeners()', () => { it('should remove the correct event listeners', () => { - const barStub = sandbox.stub(docFindBar.bar, 'removeEventListener'); + const barStub = sandbox.stub(docFindBar.findBarEl, 'removeEventListener'); const findFieldStub = sandbox.stub(docFindBar.findFieldEl, 'removeEventListener'); const findPrevStub = sandbox.stub(docFindBar.findPreviousButtonEl, 'removeEventListener'); const findNextStub = sandbox.stub(docFindBar.findNextButtonEl, 'removeEventListener'); const findCloseStub = sandbox.stub(docFindBar.findCloseButtonEl, 'removeEventListener'); docFindBar.unbindDOMListeners(); - expect(barStub).to.be.calledWith('keydown', docFindBar.barKeyDownHandler); + expect(barStub).to.be.calledWith('keydown', docFindBar.findBarKeyDownHandler); expect(findFieldStub).to.be.calledWith('input', docFindBar.findFieldHandler); expect(findPrevStub).to.be.calledWith('click', docFindBar.findPreviousHandler); expect(findNextStub).to.be.calledWith('click', docFindBar.findNextHandler); @@ -340,7 +350,7 @@ describe('lib/viewers/doc/DocFindBar', () => { }); }); - describe('barKeyDownHandler()', () => { + describe('findBarKeyDownHandler()', () => { beforeEach(() => { stubs.decodeKeydown = sandbox.stub(util, 'decodeKeydown'); stubs.event = { @@ -355,14 +365,14 @@ describe('lib/viewers/doc/DocFindBar', () => { it('should find the next match if Enter is entered', () => { stubs.decodeKeydown.returns('Enter'); - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.findNextHandler).to.be.called; }); it('should find the previous match if Shift+Enter is entered', () => { stubs.decodeKeydown.returns('Shift+Enter'); - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.findNextHandler).to.not.be.called; expect(stubs.findPreviousHandler).to.be.called; }); @@ -371,7 +381,7 @@ describe('lib/viewers/doc/DocFindBar', () => { stubs.decodeKeydown.returns('Escape'); docFindBar.opened = false; - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.close).to.not.be.called; expect(stubs.event.stopPropagation).to.not.be.called; expect(stubs.event.preventDefault).to.not.be.called; @@ -381,7 +391,7 @@ describe('lib/viewers/doc/DocFindBar', () => { stubs.decodeKeydown.returns('Escape'); docFindBar.opened = true; - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.close).to.be.called; expect(stubs.event.stopPropagation).to.be.called; expect(stubs.event.preventDefault).to.be.called; @@ -391,7 +401,7 @@ describe('lib/viewers/doc/DocFindBar', () => { stubs.decodeKeydown.returns('Esc'); docFindBar.opened = true; - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.close).to.be.called; expect(stubs.event.stopPropagation).to.be.called; expect(stubs.event.preventDefault).to.be.called; @@ -400,28 +410,28 @@ describe('lib/viewers/doc/DocFindBar', () => { it('should stop propogation if Shift++ is entered', () => { stubs.decodeKeydown.returns('Shift++'); - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.event.stopPropagation).to.be.called; }); it('should stop propogation if Shift+_ is entered', () => { stubs.decodeKeydown.returns('Shift+_'); - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.event.stopPropagation).to.be.called; }); it('should stop propogation if [ is entered', () => { stubs.decodeKeydown.returns('['); - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.event.stopPropagation).to.be.called; }); it('should stop propogation if ] is entered', () => { stubs.decodeKeydown.returns(']'); - docFindBar.barKeyDownHandler(stubs.event); + docFindBar.findBarKeyDownHandler(stubs.event); expect(stubs.event.stopPropagation).to.be.called; }); }); @@ -507,7 +517,7 @@ describe('lib/viewers/doc/DocFindBar', () => { describe('open()', () => { beforeEach(() => { stubs.findFieldHandler = sandbox.stub(docFindBar, 'findFieldHandler'); - stubs.remove = sandbox.stub(docFindBar.bar.classList, 'remove'); + stubs.remove = sandbox.stub(docFindBar.findBarEl.classList, 'remove'); stubs.select = sandbox.stub(docFindBar.findFieldEl, 'select'); stubs.focus = sandbox.stub(docFindBar.findFieldEl, 'focus'); }); @@ -559,7 +569,7 @@ describe('lib/viewers/doc/DocFindBar', () => { describe('close()', () => { beforeEach(() => { stubs.findFieldHandler = sandbox.stub(docFindBar, 'findFieldHandler'); - stubs.add = sandbox.stub(docFindBar.bar.classList, 'add'); + stubs.add = sandbox.stub(docFindBar.findBarEl.classList, 'add'); }); it('should save and clear the current search', () => {