From 25912788eb3503508f5c77a4fdfc31c2ecadfa45 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Fri, 26 Jul 2019 10:40:46 +0200 Subject: [PATCH 1/6] chore(popup): fix flaky test --- packages/popup/test/lion-popup.test.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/popup/test/lion-popup.test.js b/packages/popup/test/lion-popup.test.js index a8ead651d..9c6fd461a 100644 --- a/packages/popup/test/lion-popup.test.js +++ b/packages/popup/test/lion-popup.test.js @@ -22,12 +22,11 @@ describe('lion-popup', () => { `); const invoker = el.querySelector('[slot="invoker"]'); - const eventOnClick = new Event('click'); - invoker.dispatchEvent(eventOnClick); + invoker.click(); await el.updateComplete; expect(el.querySelector('[slot="content"]').style.display).to.be.equal('inline-block'); - invoker.dispatchEvent(eventOnClick); + invoker.click(); await el.updateComplete; expect(el.querySelector('[slot="content"]').style.display).to.be.equal('none'); }); From ee8dc3122a35809f47deb2b161af8a84c998df79 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 25 Jul 2019 17:04:08 +0200 Subject: [PATCH 2/6] chore(button): fix click event notation in demo --- packages/button/stories/index.stories.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/button/stories/index.stories.js b/packages/button/stories/index.stories.js index 5e8a9b7ff..0fa6c9d34 100644 --- a/packages/button/stories/index.stories.js +++ b/packages/button/stories/index.stories.js @@ -25,7 +25,9 @@ storiesOf('Buttons|Button', module) Debug Submit - click/space/enter me + + click/space/enter me and see log + Disabled `, From e269b5d040c7f6f7abe1ddbede059a90982fccc8 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Thu, 25 Jul 2019 17:03:25 +0200 Subject: [PATCH 3/6] fix(button): click event fired twice in IE11 (fix #179) --- packages/button/src/LionButton.js | 18 +++++++--- packages/button/test/lion-button.test.js | 43 ++++++++++++------------ 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js index c7be8621c..f55e12558 100644 --- a/packages/button/src/LionButton.js +++ b/packages/button/src/LionButton.js @@ -20,7 +20,7 @@ export class LionButton extends DisabledWithTabIndexMixin( ${this._renderAfter()} -
+
`; } @@ -128,6 +128,7 @@ export class LionButton extends DisabledWithTabIndexMixin( constructor() { super(); this.role = 'button'; + this.__setupDelegationInConstructor(); } connectedCallback() { @@ -143,16 +144,19 @@ export class LionButton extends DisabledWithTabIndexMixin( _redispatchClickEvent(oldEvent) { // replacing `MouseEvent` with `oldEvent.constructor` breaks IE const newEvent = new MouseEvent(oldEvent.type, oldEvent); + newEvent.__isRedispatchedOnNativeButton = true; this.__enforceHostEventTarget(newEvent); this.$$slot('_button').dispatchEvent(newEvent); } /** - * Prevent click on the fake element and cause click on the native button. + * Prevent normal click and redispatch click on the native button unless already redispatched. */ __clickDelegationHandler(e) { - e.stopPropagation(); - this._redispatchClickEvent(e); + if (!e.__isRedispatchedOnNativeButton) { + e.stopImmediatePropagation(); + this._redispatchClickEvent(e); + } } __enforceHostEventTarget(event) { @@ -165,6 +169,12 @@ export class LionButton extends DisabledWithTabIndexMixin( } } + __setupDelegationInConstructor() { + // do not move to connectedCallback, otherwise IE11 breaks + // more info: https://github.com/ing-bank/lion/issues/179#issuecomment-511763835 + this.addEventListener('click', this.__clickDelegationHandler, true); + } + __setupDelegation() { this.addEventListener('keydown', this.__keydownDelegationHandler); this.addEventListener('keyup', this.__keyupDelegationHandler); diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js index 1ad5334b1..1d94aaa46 100644 --- a/packages/button/test/lion-button.test.js +++ b/packages/button/test/lion-button.test.js @@ -9,10 +9,11 @@ import { import '../lion-button.js'; function getTopElement(el) { - const { left, top } = el.getBoundingClientRect(); + const { left, top, width, height } = el.getBoundingClientRect(); // to support elementFromPoint() in polyfilled browsers we have to use document - const crossBrowserRoot = el.shadowRoot.elementFromPoint ? el.shadowRoot : document; - return crossBrowserRoot.elementFromPoint(left, top); + const crossBrowserRoot = + el.shadowRoot && el.shadowRoot.elementFromPoint ? el.shadowRoot : document; + return crossBrowserRoot.elementFromPoint(left + width / 2, top + height / 2); } describe('lion-button', () => { @@ -151,7 +152,7 @@ describe('lion-button', () => { const clickSpy = sinon.spy(); const el = await fixture( html` - + foo `, ); @@ -164,27 +165,22 @@ describe('lion-button', () => { expect(clickSpy.callCount).to.equal(1); }); - describe('event after redispatching', async () => { - async function prepareClickEvent(el, host) { + describe('native button behavior', async () => { + async function prepareClickEvent(el) { setTimeout(() => { - if (host) { - // click on host like in native button - makeMouseEvent('click', { x: 11, y: 11 }, el); - } else { - // click on click-area which is then redispatched - makeMouseEvent('click', { x: 11, y: 11 }, getTopElement(el)); - } + makeMouseEvent('click', { x: 11, y: 11 }, getTopElement(el)); }); return oneEvent(el, 'click'); } - let hostEvent; - let redispatchedEvent; + let nativeButtonEvent; + let lionButtonEvent; before(async () => { - const el = await fixture(''); - hostEvent = await prepareClickEvent(el, true); - redispatchedEvent = await prepareClickEvent(el, false); + const nativeButtonEl = await fixture(''); + const lionButtonEl = await fixture('foo'); + nativeButtonEvent = await prepareClickEvent(nativeButtonEl); + lionButtonEvent = await prepareClickEvent(lionButtonEl); }); const sameProperties = [ @@ -194,14 +190,19 @@ describe('lion-button', () => { 'cancelable', 'clientX', 'clientY', - 'target', ]; sameProperties.forEach(property => { - it(`has same value of the property "${property}"`, async () => { - expect(redispatchedEvent[property]).to.equal(hostEvent[property]); + it(`has same value of the property "${property}" as in native button event`, () => { + expect(lionButtonEvent[property]).to.equal(nativeButtonEvent[property]); }); }); + + it('has host in the target property', async () => { + const el = await fixture('foo'); + const event = await prepareClickEvent(el); + expect(event.target).to.equal(el); + }); }); }); }); From 471d662cb288ef26b058b736cdd131ef33d93340 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Fri, 26 Jul 2019 15:40:12 +0200 Subject: [PATCH 4/6] feat(button): move active to host for cross-browser support (fix #188) --- packages/button/src/LionButton.js | 25 ++++++++++-- packages/button/test/lion-button.test.js | 51 ++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js index f55e12558..23f027799 100644 --- a/packages/button/src/LionButton.js +++ b/packages/button/src/LionButton.js @@ -10,6 +10,10 @@ export class LionButton extends DisabledWithTabIndexMixin( type: String, reflect: true, }, + active: { + type: Boolean, + reflect: true, + }, }; } @@ -83,8 +87,8 @@ export class LionButton extends DisabledWithTabIndexMixin( background: #f4f6f7; } - :host(:active) .btn, - .btn[active] { + :host(:active) .btn, /* keep native :active to render quickly where possible */ + :host([active]) .btn /* use custom [active] to fix IE11 */ { /* if you extend, please overwrite */ background: gray; } @@ -128,6 +132,7 @@ export class LionButton extends DisabledWithTabIndexMixin( constructor() { super(); this.role = 'button'; + this.active = false; this.__setupDelegationInConstructor(); } @@ -176,19 +181,31 @@ export class LionButton extends DisabledWithTabIndexMixin( } __setupDelegation() { + this.addEventListener('mousedown', this.__mousedownDelegationHandler); + this.addEventListener('mouseup', this.__mouseupDelegationHandler); this.addEventListener('keydown', this.__keydownDelegationHandler); this.addEventListener('keyup', this.__keyupDelegationHandler); } __teardownDelegation() { + this.removeEventListener('mousedown', this.__mousedownDelegationHandler); + this.removeEventListener('mouseup', this.__mouseupDelegationHandler); this.removeEventListener('keydown', this.__keydownDelegationHandler); this.removeEventListener('keyup', this.__keyupDelegationHandler); } + __mousedownDelegationHandler() { + this.active = true; + } + + __mouseupDelegationHandler() { + this.active = false; + } + __keydownDelegationHandler(e) { if (e.keyCode === 32 /* space */ || e.keyCode === 13 /* enter */) { e.preventDefault(); - this.shadowRoot.querySelector('.btn').setAttribute('active', ''); + this.active = true; } } @@ -197,7 +214,7 @@ export class LionButton extends DisabledWithTabIndexMixin( // and make click handlers on button work on space and enter if (e.keyCode === 32 /* space */ || e.keyCode === 13 /* enter */) { e.preventDefault(); - this.shadowRoot.querySelector('.btn').removeAttribute('active'); + this.active = false; this.shadowRoot.querySelector('.click-area').click(); } } diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js index 1d94aaa46..cb2c15580 100644 --- a/packages/button/test/lion-button.test.js +++ b/packages/button/test/lion-button.test.js @@ -4,6 +4,10 @@ import { makeMouseEvent, pressEnter, pressSpace, + down, + up, + keyDownOn, + keyUpOn, } from '@polymer/iron-test-helpers/mock-interactions.js'; import '../lion-button.js'; @@ -57,6 +61,53 @@ describe('lion-button', () => { expect(el.hasAttribute('disabled')).to.equal(true); }); + describe('active', () => { + it('updates "active" attribute on host when mousedown/mouseup on button', async () => { + const el = await fixture(`foo`); + const topEl = getTopElement(el); + + down(topEl); + expect(el.active).to.be.true; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.true; + + up(topEl); + expect(el.active).to.be.false; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.false; + }); + + it('updates "active" attribute on host when space keydown/keyup on button', async () => { + const el = await fixture(`foo`); + const topEl = getTopElement(el); + + keyDownOn(topEl, 32); + expect(el.active).to.be.true; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.true; + + keyUpOn(topEl, 32); + expect(el.active).to.be.false; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.false; + }); + + it('updates "active" attribute on host when enter keydown/keyup on button', async () => { + const el = await fixture(`foo`); + const topEl = getTopElement(el); + + keyDownOn(topEl, 13); + expect(el.active).to.be.true; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.true; + + keyUpOn(topEl, 13); + expect(el.active).to.be.false; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.false; + }); + }); + describe('a11y', () => { it('has a role="button" by default', async () => { const el = await fixture(`foo`); From f3303ae014254e6043e1d88cfc809b68cc698d91 Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Fri, 26 Jul 2019 16:19:01 +0200 Subject: [PATCH 5/6] fix(button): remove active when mouse/key up on other element (fix #210) --- packages/button/src/LionButton.js | 70 +++++++++++++----------- packages/button/test/lion-button.test.js | 45 +++++++++++++++ 2 files changed, 84 insertions(+), 31 deletions(-) diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js index 23f027799..4f8823c43 100644 --- a/packages/button/src/LionButton.js +++ b/packages/button/src/LionButton.js @@ -138,12 +138,12 @@ export class LionButton extends DisabledWithTabIndexMixin( connectedCallback() { super.connectedCallback(); - this.__setupDelegation(); + this.__setupEvents(); } disconnectedCallback() { super.disconnectedCallback(); - this.__teardownDelegation(); + this.__teardownEvents(); } _redispatchClickEvent(oldEvent) { @@ -180,42 +180,50 @@ export class LionButton extends DisabledWithTabIndexMixin( this.addEventListener('click', this.__clickDelegationHandler, true); } - __setupDelegation() { - this.addEventListener('mousedown', this.__mousedownDelegationHandler); - this.addEventListener('mouseup', this.__mouseupDelegationHandler); - this.addEventListener('keydown', this.__keydownDelegationHandler); - this.addEventListener('keyup', this.__keyupDelegationHandler); + __setupEvents() { + this.addEventListener('mousedown', this.__mousedownHandler); + this.addEventListener('keydown', this.__keydownHandler); + this.addEventListener('keyup', this.__keyupHandler); } - __teardownDelegation() { - this.removeEventListener('mousedown', this.__mousedownDelegationHandler); - this.removeEventListener('mouseup', this.__mouseupDelegationHandler); - this.removeEventListener('keydown', this.__keydownDelegationHandler); - this.removeEventListener('keyup', this.__keyupDelegationHandler); + __teardownEvents() { + this.removeEventListener('mousedown', this.__mousedownHandler); + this.removeEventListener('keydown', this.__keydownHandler); + this.removeEventListener('keyup', this.__keyupHandler); } - __mousedownDelegationHandler() { + __mousedownHandler() { this.active = true; - } - - __mouseupDelegationHandler() { - this.active = false; - } - - __keydownDelegationHandler(e) { - if (e.keyCode === 32 /* space */ || e.keyCode === 13 /* enter */) { - e.preventDefault(); - this.active = true; - } - } - - __keyupDelegationHandler(e) { - // Makes the real button the trigger in forms (will submit form, as opposed to paper-button) - // and make click handlers on button work on space and enter - if (e.keyCode === 32 /* space */ || e.keyCode === 13 /* enter */) { - e.preventDefault(); + const mouseupHandler = () => { this.active = false; + document.removeEventListener('mouseup', mouseupHandler); + }; + document.addEventListener('mouseup', mouseupHandler); + } + + __keydownHandler(e) { + if (!this.__isKeyboardClickEvent(e)) { + return; + } + this.active = true; + const keyupHandler = keyupEvent => { + if (this.__isKeyboardClickEvent(keyupEvent)) { + this.active = false; + document.removeEventListener('keyup', keyupHandler, true); + } + }; + document.addEventListener('keyup', keyupHandler, true); + } + + __keyupHandler(e) { + if (this.__isKeyboardClickEvent(e)) { + // redispatch click this.shadowRoot.querySelector('.click-area').click(); } } + + // eslint-disable-next-line class-methods-use-this + __isKeyboardClickEvent(e) { + return e.keyCode === 32 /* space */ || e.keyCode === 13 /* enter */; + } } diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js index cb2c15580..0b6792aa6 100644 --- a/packages/button/test/lion-button.test.js +++ b/packages/button/test/lion-button.test.js @@ -77,6 +77,21 @@ describe('lion-button', () => { expect(el.hasAttribute('active')).to.be.false; }); + it('updates "active" attribute on host when mousedown on button and mouseup anywhere else', async () => { + const el = await fixture(`foo`); + const topEl = getTopElement(el); + + down(topEl); + expect(el.active).to.be.true; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.true; + + up(document.body); + expect(el.active).to.be.false; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.false; + }); + it('updates "active" attribute on host when space keydown/keyup on button', async () => { const el = await fixture(`foo`); const topEl = getTopElement(el); @@ -92,6 +107,21 @@ describe('lion-button', () => { expect(el.hasAttribute('active')).to.be.false; }); + it('updates "active" attribute on host when space keydown on button and space keyup anywhere else', async () => { + const el = await fixture(`foo`); + const topEl = getTopElement(el); + + keyDownOn(topEl, 32); + expect(el.active).to.be.true; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.true; + + keyUpOn(document.body, 32); + expect(el.active).to.be.false; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.false; + }); + it('updates "active" attribute on host when enter keydown/keyup on button', async () => { const el = await fixture(`foo`); const topEl = getTopElement(el); @@ -106,6 +136,21 @@ describe('lion-button', () => { await el.updateComplete; expect(el.hasAttribute('active')).to.be.false; }); + + it('updates "active" attribute on host when enter keydown on button and space keyup anywhere else', async () => { + const el = await fixture(`foo`); + const topEl = getTopElement(el); + + keyDownOn(topEl, 13); + expect(el.active).to.be.true; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.true; + + keyUpOn(document.body, 13); + expect(el.active).to.be.false; + await el.updateComplete; + expect(el.hasAttribute('active')).to.be.false; + }); }); describe('a11y', () => { From 06124e06bf26881fcf429ce116765ba0d28ac87e Mon Sep 17 00:00:00 2001 From: Mikhail Bashkirov Date: Fri, 26 Jul 2019 16:45:49 +0200 Subject: [PATCH 6/6] fix(button): prevent unnecessary keydown/keyup handlers --- packages/button/src/LionButton.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js index 4f8823c43..46526e5f3 100644 --- a/packages/button/src/LionButton.js +++ b/packages/button/src/LionButton.js @@ -202,7 +202,7 @@ export class LionButton extends DisabledWithTabIndexMixin( } __keydownHandler(e) { - if (!this.__isKeyboardClickEvent(e)) { + if (this.active || !this.__isKeyboardClickEvent(e)) { return; } this.active = true;