From 91270b2dc18aa9124ed2dc456887fd7d1eb5a15e Mon Sep 17 00:00:00 2001 From: vvo Date: Sat, 20 Feb 2016 00:32:36 +0100 Subject: [PATCH] fix(searchBox): stop setting the query twice We were previously setting the query on every keystroke twice, because of the keyup + input events. Now fixed. This was not triggering any bad behavior but extra work and maybe bad performance because the way we set queries in the helper (to be worked on) + refactor + update tests --- .../search-box/__tests__/search-box-test.js | 29 ++++----- src/widgets/search-box/search-box.js | 64 +++++++++++++------ 2 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/widgets/search-box/__tests__/search-box-test.js b/src/widgets/search-box/__tests__/search-box-test.js index 7e0d8fc0d9..5540519dc1 100644 --- a/src/widgets/search-box/__tests__/search-box-test.js +++ b/src/widgets/search-box/__tests__/search-box-test.js @@ -209,7 +209,6 @@ describe('searchBox()', () => { beforeEach(() => { container = document.createElement('div'); input = createHTMLNodeFromString(''); - input.addEventListener = sinon.spy(); }); function simulateInputEvent() { @@ -218,12 +217,9 @@ describe('searchBox()', () => { // When widget.init({state, helper, onHistoryChange}); // Then - expect(input.addEventListener.called).toEqual(true); - expect(input.addEventListener.args[1].length).toEqual(3); - expect(input.addEventListener.args[1][0]).toEqual('input'); - let fn = input.addEventListener.args[1][1]; - - return fn({currentTarget: {value: 'test'}}); + input.value = 'test'; + let event = new Event('input'); + input.dispatchEvent(event); } context('instant search', () => { @@ -234,9 +230,13 @@ describe('searchBox()', () => { it('performs a search on any change', () => { simulateInputEvent(); - expect(helper.setQuery.calledOnce).toBe(true); expect(helper.search.called).toBe(true); }); + + it('sets the query on any change', () => { + simulateInputEvent(); + expect(helper.setQuery.calledOnce).toBe(true); + }); }); context('non-instant search', () => { @@ -258,7 +258,6 @@ describe('searchBox()', () => { beforeEach(() => { container = document.createElement('div'); input = createHTMLNodeFromString(''); - input.addEventListener = sinon.spy(); }); function simulateKeyUpEvent(args) { @@ -267,12 +266,9 @@ describe('searchBox()', () => { // When widget.init({state, helper, onHistoryChange}); // Then - expect(input.addEventListener.called).toEqual(true); - expect(input.addEventListener.args[0].length).toEqual(2); - expect(input.addEventListener.args[0][0]).toEqual('keyup'); - let fn = input.addEventListener.args[0][1]; - - return fn(args); + let event = new Event('keyup', args); + Object.defineProperty(event, 'keyCode', {get: () => args.keyCode}); + input.dispatchEvent(event); } context('instant search', () => { @@ -283,7 +279,6 @@ describe('searchBox()', () => { it('do not perform the search on keyup event (should be done by input event)', () => { simulateKeyUpEvent({}); - expect(helper.setQuery.calledOnce).toBe(true); expect(helper.search.called).toBe(false); }); }); @@ -296,13 +291,11 @@ describe('searchBox()', () => { it('performs the search on keyup if ', () => { simulateKeyUpEvent({keyCode: 13}); - expect(helper.setQuery.calledOnce).toBe(true); expect(helper.search.calledOnce).toBe(true); }); it('doesn\'t perform the search on keyup if not ', () => { simulateKeyUpEvent({}); - expect(helper.setQuery.calledOnce).toBe(true); expect(helper.search.calledOnce).toBe(false); }); }); diff --git a/src/widgets/search-box/search-box.js b/src/widgets/search-box/search-box.js index 06c777df12..bbb257b2dc 100644 --- a/src/widgets/search-box/search-box.js +++ b/src/widgets/search-box/search-box.js @@ -44,6 +44,14 @@ function searchBox({ autofocus = 'auto', searchOnEnterKeyPressOnly = false }) { + // the 'input' event is triggered when the input value changes + // in any case: typing, copy pasting with mouse.. + // 'onpropertychange' is the IE8 alternative until we support IE8 + // but it's flawed: http://help.dottoro.com/ljhxklln.php + const INPUT_EVENT = window.addEventListener ? + 'input' : + 'propertychange'; + if (!container) { throw new Error(usage); } @@ -124,31 +132,37 @@ function searchBox({ // Add all the needed attributes and listeners to the input this.addDefaultAttributesToInput(input, state.query); - // Keep keyup to handle searchOnEnterKeyPressOnly - input.addEventListener('keyup', (e) => { - helper.setQuery(input.value); - if (searchOnEnterKeyPressOnly && e.keyCode === KEY_ENTER) { - helper.search(); - } - - // IE8/9 compatibility - if (window.attachEvent && e.keyCode === KEY_SUPPRESS) { - helper.search(); - } - }); + // always set the query on every change + addListener(input, INPUT_EVENT, setQuery); - function inputCallback(e) { - let target = (e.currentTarget) ? e.currentTarget : e.srcElement; - helper.setQuery(target.value); - if (!searchOnEnterKeyPressOnly) { - helper.search(); - } + // handle IE8 weirdness where BACKSPACE key will not trigger an input change.. + // can be removed as soon as we remove support for it + if (INPUT_EVENT === 'propertychange' || window.attachEvent) { + addListener(input, 'keyup', ifKey(KEY_SUPPRESS, setQuery)); + addListener(input, 'keyup', ifKey(KEY_SUPPRESS, search)); } - if (window.attachEvent) { // IE8/9 compatibility - input.attachEvent('onpropertychange', inputCallback); + // only search on enter + if (searchOnEnterKeyPressOnly) { + addListener(input, 'keyup', ifKey(KEY_ENTER, search)); } else { - input.addEventListener('input', inputCallback, false); + addListener(input, INPUT_EVENT, search); + } + + function setQuery(e) { + helper.setQuery(getValue(e)); + } + + function search() { + helper.search(); + } + + function getValue(e) { + return ((e.currentTarget) ? e.currentTarget : e.srcElement).value; + } + + function ifKey(expectedKeyCode, fnToExecute) { + return actualEvent => actualEvent.keyCode === expectedKeyCode && fnToExecute(actualEvent); } if (isInputTargeted) { @@ -180,4 +194,12 @@ function searchBox({ }; } +function addListener(el, type, fn) { + if (el.addEventListener) { + el.addEventListener(type, fn); + } else { + el.attachEvent('on' + type, fn); + } +} + export default searchBox;