Skip to content

Commit

Permalink
Fix: document findbar no longer ignores useHotkeys (#724)
Browse files Browse the repository at this point in the history
  • Loading branch information
DanDeMicco authored Mar 27, 2018
1 parent 312c300 commit 4e0bddc
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 29 deletions.
2 changes: 1 addition & 1 deletion src/lib/Preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -1747,7 +1747,7 @@ class Preview extends EventEmitter {
}

if (this.viewer && typeof this.viewer.onKeydown === 'function') {
consumed = !!this.viewer.onKeydown(key);
consumed = !!this.viewer.onKeydown(key, event);
}

if (!consumed) {
Expand Down
6 changes: 5 additions & 1 deletion src/lib/viewers/doc/DocBaseViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -500,9 +500,10 @@ class DocBaseViewer extends BaseViewer {
* Handles keyboard events for document viewer.
*
* @param {string} key - keydown key
* @param {Object} event - Key event
* @return {boolean} consumed or not
*/
onKeydown(key) {
onKeydown(key, event) {
switch (key) {
case 'ArrowLeft':
this.previousPage();
Expand All @@ -517,6 +518,9 @@ class DocBaseViewer extends BaseViewer {
this.nextPage();
break;
default:
if (this.findBar) {
return this.findBar.onKeydown(event);
}
return false;
}

Expand Down
14 changes: 5 additions & 9 deletions src/lib/viewers/doc/DocFindBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class DocFindBar extends EventEmitter {
}

// Bind context for callbacks
this.displayFindBarHandler = this.displayFindBarHandler.bind(this);
this.onKeydown = this.onKeydown.bind(this);
this.findFieldHandler = this.findFieldHandler.bind(this);
this.barKeyDownHandler = this.barKeyDownHandler.bind(this);
this.findNextHandler = this.findNextHandler.bind(this);
Expand Down Expand Up @@ -213,9 +213,6 @@ class DocFindBar extends EventEmitter {
this.findPreviousButtonEl.addEventListener('click', this.findPreviousHandler);
this.findNextButtonEl.addEventListener('click', this.findNextHandler);
this.findCloseButtonEl.addEventListener('click', this.close);

// KeyDown handler to show/hide find bar
document.addEventListener('keydown', this.displayFindBarHandler);
}

/**
Expand All @@ -230,9 +227,6 @@ class DocFindBar extends EventEmitter {
this.findPreviousButtonEl.removeEventListener('click', this.findPreviousHandler);
this.findNextButtonEl.removeEventListener('click', this.findNextHandler);
this.findCloseButtonEl.removeEventListener('click', this.close);

// Remove KeyDown handler to show/hide find bar
document.removeEventListener('keydown', this.displayFindBarHandler);
}

//--------------------------------------------------------------------------
Expand All @@ -245,7 +239,7 @@ class DocFindBar extends EventEmitter {
* @param {Event} event - Key event
* @return {void}
*/
displayFindBarHandler(event) {
onKeydown(event) {
// Lowercase keydown so we capture both lower and uppercase
const key = decodeKeydown(event).toLowerCase();
switch (key) {
Expand All @@ -260,8 +254,10 @@ class DocFindBar extends EventEmitter {
event.preventDefault();
break;
default:
break;
return false;
}

return true;
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/lib/viewers/doc/DocumentViewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@ class DocumentViewer extends DocBaseViewer {
*
* @override
* @param {string} key - Keydown key
* @param {Object} event - Key event
* @return {boolean} Consumed or not
*/
onKeydown(key) {
onKeydown(key, event) {
if (key === 'Shift++') {
this.zoomIn();
return true;
Expand All @@ -55,7 +56,7 @@ class DocumentViewer extends DocBaseViewer {
return true;
}

return super.onKeydown(key);
return super.onKeydown(key, event);
}

//--------------------------------------------------------------------------
Expand Down
14 changes: 13 additions & 1 deletion src/lib/viewers/doc/__tests__/DocBaseViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
});
});

describe('onKeyDown()', () => {
describe('onKeydown()', () => {
beforeEach(() => {
stubs.previousPage = sandbox.stub(docBase, 'previousPage');
stubs.nextPage = sandbox.stub(docBase, 'nextPage');
Expand All @@ -785,6 +785,18 @@ describe('src/lib/viewers/doc/DocBaseViewer', () => {
expect(rightBracket).to.equal(true);
});

it('should call the findBar onKeydown if present', () => {
const keys = 'ctrl+f';
const mockEvent = sandbox.stub();
const onKeydownStub = sandbox.stub().withArgs(mockEvent);
docBase.findBar = {
onKeydown: onKeydownStub,
destroy: sandbox.stub()
};
docBase.onKeydown(keys, mockEvent);
expect(onKeydownStub).to.have.been.calledOnce;
});

it('should return false if there is no match', () => {
const arrowLeft = docBase.onKeydown('ArrowUp');
expect(stubs.previousPage).to.not.be.called;
Expand Down
18 changes: 8 additions & 10 deletions src/lib/viewers/doc/__tests__/DocFindBar-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,19 +265,17 @@ describe('lib/viewers/doc/DocFindBar', () => {
const findPrevStub = sandbox.stub(docFindBar.findPreviousButtonEl, 'removeEventListener');
const findNextStub = sandbox.stub(docFindBar.findNextButtonEl, 'removeEventListener');
const findCloseStub = sandbox.stub(docFindBar.findCloseButtonEl, 'removeEventListener');
const documentStub = sandbox.stub(document, 'removeEventListener');

docFindBar.unbindDOMListeners();
expect(barStub).to.be.calledWith('keydown', docFindBar.barKeyDownHandler);
expect(findFieldStub).to.be.calledWith('input', docFindBar.findFieldHandler);
expect(findPrevStub).to.be.calledWith('click', docFindBar.findPreviousHandler);
expect(findNextStub).to.be.calledWith('click', docFindBar.findNextHandler);
expect(findCloseStub).to.be.calledWith('click', docFindBar.close);
expect(documentStub).to.be.calledWith('keydown', docFindBar.displayFindBarHandler);
});
});

describe('displayFindBarHandler()', () => {
describe('onKeydown()', () => {
beforeEach(() => {
stubs.decodeKeydown = sandbox.stub(util, 'decodeKeydown');
stubs.open = sandbox.stub(docFindBar, 'open');
Expand All @@ -292,47 +290,47 @@ describe('lib/viewers/doc/DocFindBar', () => {
docFindBar.canDownload = false;
stubs.decodeKeydown.returns('meta+f');

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.not.be.called;
expect(stubs.event.preventDefault).to.be.called;
});

it('should open and prevent default if meta+f is entered', () => {
stubs.decodeKeydown.returns('meta+f');

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.be.called;
expect(stubs.event.preventDefault).to.be.called;
});

it('should open and prevent default if control+f is entered', () => {
stubs.decodeKeydown.returns('control+f');

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.be.called;
expect(stubs.event.preventDefault).to.be.called;
});

it('should open and prevent default if meta+g is entered', () => {
stubs.decodeKeydown.returns('meta+g');

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.be.called;
expect(stubs.event.preventDefault).to.be.called;
});

it('should open and prevent default if control+g is entered', () => {
stubs.decodeKeydown.returns('control+g');

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.be.called;
expect(stubs.event.preventDefault).to.be.called;
});

it('should open and prevent default if f3 is entered', () => {
stubs.decodeKeydown.returns('f3');

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.be.called;
expect(stubs.event.preventDefault).to.be.called;
});
Expand All @@ -341,7 +339,7 @@ describe('lib/viewers/doc/DocFindBar', () => {
stubs.decodeKeydown.returns('escape');
docFindBar.opened = false;

docFindBar.displayFindBarHandler(stubs.event);
docFindBar.onKeydown(stubs.event);
expect(stubs.open).to.not.be.called;
expect(stubs.close).to.not.be.called;
expect(stubs.event.preventDefault).to.not.be.called;
Expand Down
16 changes: 11 additions & 5 deletions src/lib/viewers/doc/__tests__/DocumentViewer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ describe('lib/viewers/doc/DocumentViewer', () => {
});
});

describe('onKeyDown()', () => {
describe('onKeydown()', () => {
beforeEach(() => {
stubs.zoomIn = sandbox.stub(doc, 'zoomIn');
stubs.zoomOut = sandbox.stub(doc, 'zoomOut');
Expand Down Expand Up @@ -132,16 +132,22 @@ describe('lib/viewers/doc/DocumentViewer', () => {
});

it('should fallback to doc base\'s onKeydown if no entry matches', () => {
const docbaseStub = sandbox.spy(DocBaseViewer.prototype, 'onKeydown');
const eventStub = sandbox.stub();
stubs.fullscreen.returns(false);
const result = doc.onKeydown('ArrowDown');

const key = 'ArrowDown';
const result = doc.onKeydown(key, eventStub);
expect(docbaseStub).to.have.been.calledWithExactly(key, eventStub);
expect(result).to.be.false;
expect(stubs.nextPage).to.not.be.called;

stubs.fullscreen.returns(false);
const result2 = doc.onKeydown('ArrowRight');

const key2 = 'ArrowRight';
const result2 = doc.onKeydown(key2, eventStub);
expect(docbaseStub).to.have.been.calledWithExactly(key2, eventStub);
expect(result2).to.be.true;

expect(docbaseStub).to.have.been.calledTwice;
});
});

Expand Down

0 comments on commit 4e0bddc

Please sign in to comment.