diff --git a/.changeset/afraid-maps-fix.md b/.changeset/afraid-maps-fix.md new file mode 100644 index 000000000..fef2738fc --- /dev/null +++ b/.changeset/afraid-maps-fix.md @@ -0,0 +1,8 @@ +--- +'@lion/button': patch +--- + +Button fixes + +- make click event target predictable (always host) +- do not override aria-labelledby from user diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js index 8f9449163..ab5526669 100644 --- a/packages/button/src/LionButton.js +++ b/packages/button/src/LionButton.js @@ -146,10 +146,6 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement)) )); } - get _form() { - return this._nativeButtonNode.form; - } - // @ts-ignore get slots() { return { @@ -173,18 +169,30 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement)) this._buttonId = `button-${Math.random().toString(36).substr(2, 10)}`; if (browserDetection.isIE11) { - this.updateComplete.then(() => this.setAttribute('aria-labelledby', this._buttonId)); + this.updateComplete.then(() => { + if (!this.hasAttribute('aria-labelledby')) { + this.setAttribute('aria-labelledby', this._buttonId); + } + }); } + + /** @type {HTMLButtonElement} */ + this.__submitAndResetHelperButton = document.createElement('button'); + + /** @type {EventListener} */ + this.__preventEventLeakage = this.__preventEventLeakage.bind(this); } connectedCallback() { super.connectedCallback(); this.__setupEvents(); + this.__setupSubmitAndResetHelperOnConnected(); } disconnectedCallback() { super.disconnectedCallback(); this.__teardownEvents(); + this.__teardownSubmitAndResetHelperOnDisconnected(); } /** @@ -207,12 +215,24 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement)) * Delegate click, by flashing a native button as a direct child * of the form, and firing click on this button. This will fire the form submit * without side effects caused by the click bubbling back up to lion-button. - * @param {Event} e + * @param {Event} ev */ - __clickDelegationHandler(e) { - if ((this.type === 'submit' || this.type === 'reset') && e.target === this && this._form) { - e.stopImmediatePropagation(); - this._nativeButtonNode.click(); + __clickDelegationHandler(ev) { + if ((this.type === 'submit' || this.type === 'reset') && ev.target === this && this._form) { + /** + * Here, we make sure our button is compatible with a native form, by firing a click + * from a native button that our form responds to. The native button we spawn will be a direct + * child of the form, plus the click event that will be sent will be prevented from + * propagating outside of the form. This will keep the amount of 'noise' (click events + * from 'ghost elements' that can be intercepted by listeners in the bubble chain) to an + * absolute minimum. + */ + this.__submitAndResetHelperButton.type = this.type; + + this._form.appendChild(this.__submitAndResetHelperButton); + // Form submission or reset will happen + this.__submitAndResetHelperButton.click(); + this._form.removeChild(this.__submitAndResetHelperButton); } } @@ -287,4 +307,28 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement)) this.click(); } } + + /** + * Prevents that someone who listens outside or on form catches the click event + * @param {Event} e + */ + __preventEventLeakage(e) { + if (e.target === this.__submitAndResetHelperButton) { + e.stopImmediatePropagation(); + } + } + + __setupSubmitAndResetHelperOnConnected() { + this._form = this._nativeButtonNode.form; + + if (this._form) { + this._form.addEventListener('click', this.__preventEventLeakage); + } + } + + __teardownSubmitAndResetHelperOnDisconnected() { + if (this._form) { + this._form.removeEventListener('click', this.__preventEventLeakage); + } + } } diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js index bd340c2ca..f0fddca3a 100644 --- a/packages/button/test/lion-button.test.js +++ b/packages/button/test/lion-button.test.js @@ -211,6 +211,15 @@ describe('lion-button', () => { browserDetectionStub.restore(); }); + it('does not override aria-labelledby when provided by user', async () => { + const browserDetectionStub = sinon.stub(browserDetection, 'isIE11').value(true); + const el = /** @type {LionButton} */ (await fixture( + `foo`, + )); + expect(el.getAttribute('aria-labelledby')).to.equal('some-id another-id'); + browserDetectionStub.restore(); + }); + it('has a native button node with aria-hidden set to true', async () => { const el = /** @type {LionButton} */ (await fixture('')); @@ -239,9 +248,9 @@ describe('lion-button', () => { foo `); - const button = /** @type {LionButton} */ ( - /** @type {LionButton} */ (form.querySelector('lion-button')) - ); + const button /** @type {LionButton} */ = /** @type {LionButton} */ (form.querySelector( + 'lion-button', + )); button.click(); expect(formSubmitSpy).to.have.been.calledOnce; }); @@ -253,9 +262,9 @@ describe('lion-button', () => { foo `); - const button = /** @type {LionButton} */ ( - /** @type {LionButton} */ (form.querySelector('lion-button')) - ); + const button /** @type {LionButton} */ = /** @type {LionButton} */ (form.querySelector( + 'lion-button', + )); button.dispatchEvent(new KeyboardEvent('keyup', { key: ' ' })); await aTimeout(0); await aTimeout(0); @@ -286,9 +295,9 @@ describe('lion-button', () => { reset `); - const btn = /** @type {LionButton} */ ( - /** @type {LionButton} */ (form.querySelector('lion-button')) - ); + const btn /** @type {LionButton} */ = /** @type {LionButton} */ (form.querySelector( + 'lion-button', + )); const firstName = /** @type {HTMLInputElement} */ (form.querySelector( 'input[name=firstName]', )); @@ -350,9 +359,8 @@ describe('lion-button', () => { `); - /** @type {LionButton} */ (form.querySelector('lion-button')).dispatchEvent( - new KeyboardEvent('keyup', { key: ' ' }), - ); + const lionButton = /** @type {LionButton} */ (form.querySelector('lion-button')); + lionButton.dispatchEvent(new KeyboardEvent('keyup', { key: ' ' })); await aTimeout(0); await aTimeout(0); @@ -410,7 +418,7 @@ describe('lion-button', () => { it('is fired once', async () => { const clickSpy = /** @type {EventListener} */ (sinon.spy()); const el = /** @type {LionButton} */ (await fixture( - html`foo`, + html` foo `, )); el.click(); @@ -422,22 +430,82 @@ describe('lion-button', () => { expect(clickSpy).to.have.been.calledOnce; }); - it('is fired one inside a form', async () => { - const formClickSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); - const el = /** @type {HTMLFormElement} */ (await fixture( - html`
- foo -
`, + it('is fired once outside and inside the form', async () => { + const outsideSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const insideSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const formSpyEarly = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const formSpyLater = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + + const el = /** @type {HTMLDivElement} */ (await fixture( + html` +
+
+
+ foo +
+
+
+ `, )); + const lionButton = /** @type {LionButton} */ (el.querySelector('lion-button')); + const form = /** @type {HTMLFormElement} */ (el.querySelector('form')); + form.addEventListener('click', formSpyLater); - // @ts-ignore - el.querySelector('lion-button').click(); - + lionButton.click(); // trying to wait for other possible redispatched events await aTimeout(0); await aTimeout(0); - expect(formClickSpy).to.have.been.calledOnce; + expect(insideSpy).to.have.been.calledOnce; + expect(outsideSpy).to.have.been.calledOnce; + // A small sacrifice for event listeners registered early: we get the native button evt. + expect(formSpyEarly).to.have.been.calledTwice; + expect(formSpyLater).to.have.been.calledOnce; + }); + + it('works when connected to different form', async () => { + const form1El = /** @type {HTMLFormElement} */ (await fixture( + html` +
+ foo +
+ `, + )); + const lionButton = /** @type {LionButton} */ (form1El.querySelector('lion-button')); + + expect(lionButton._form).to.equal(form1El); + + // Now we add the lionButton to a different form. + // We disconnect and connect and check if everything still works as expected + const outsideSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const insideSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const formSpyEarly = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + const formSpyLater = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); + + const form2El = /** @type {HTMLFormElement} */ (await fixture( + html` +
+
+
${lionButton}
+
+
+ `, + )); + const form2Node = /** @type {HTMLFormElement} */ (form2El.querySelector('form')); + + expect(lionButton._form).to.equal(form2Node); + + form2Node.addEventListener('click', formSpyLater); + lionButton.click(); + // trying to wait for other possible redispatched events + await aTimeout(0); + await aTimeout(0); + + expect(insideSpy).to.have.been.calledOnce; + expect(outsideSpy).to.have.been.calledOnce; + // A small sacrifice for event listeners registered early: we get the native button evt. + expect(formSpyEarly).to.have.been.calledTwice; + expect(formSpyLater).to.have.been.calledOnce; }); describe('native button behavior', async () => { @@ -479,17 +547,17 @@ describe('lion-button', () => { }); const useCases = [ - { container: 'div', type: 'submit', targetHost: true }, - { container: 'div', type: 'reset', targetHost: true }, - { container: 'div', type: 'button', targetHost: true }, - { container: 'form', type: 'submit', targetHost: false }, - { container: 'form', type: 'reset', targetHost: false }, - { container: 'form', type: 'button', targetHost: true }, + { container: 'div', type: 'submit' }, + { container: 'div', type: 'reset' }, + { container: 'div', type: 'button' }, + { container: 'form', type: 'submit' }, + { container: 'form', type: 'reset' }, + { container: 'form', type: 'button' }, ]; useCases.forEach(useCase => { - const { container, type, targetHost } = useCase; - const targetName = targetHost ? 'host' : 'native button'; + const { container, type } = useCase; + const targetName = 'host'; it(`is ${targetName} with type ${type} and it is inside a ${container}`, async () => { const clickSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault())); const el = /** @type {LionButton} */ (await fixture( @@ -499,11 +567,7 @@ describe('lion-button', () => { await fixture(html`<${tag} @click="${clickSpy}">${el}`); const event = await prepareClickEvent(el); - if (targetHost) { - expect(event.target).to.equal(el); - } else { - expect(event.target).to.equal(el._nativeButtonNode); - } + expect(event.target).to.equal(el); }); }); });