fix(button): make click event target predictable (always host)

.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

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 {
@@ -179,16 +175,24 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
         }
       });
     }
+
+    /** @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();
   }
 
   /**
@@ -211,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) {
+  __clickDelegationHandler(ev) {
-    if ((this.type === 'submit' || this.type === 'reset') && e.target === this && this._form) {
+    if ((this.type === 'submit' || this.type === 'reset') && ev.target === this && this._form) {
-      e.stopImmediatePropagation();
+      /**
-      this._nativeButtonNode.click();
+       * 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);
     }
   }
 
@@ -291,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);
+    }
+  }
 }

packages/button/test/lion-button.test.js

@@ -359,9 +359,8 @@ describe('lion-button', () => {
           </form>
         `);
 
-        /** @type {LionButton} */ form
+        const lionButton = /** @type {LionButton} */ (form.querySelector('lion-button'));
-          .querySelector('lion-button')
+        lionButton.dispatchEvent(new KeyboardEvent('keyup', { key: ' ' }));
-          .dispatchEvent(new KeyboardEvent('keyup', { key: ' ' }));
         await aTimeout(0);
         await aTimeout(0);
 
@@ -431,24 +430,82 @@ describe('lion-button', () => {
       expect(clickSpy).to.have.been.calledOnce;
     });
 
-    it('is fired one inside a form', async () => {
+    it('is fired once outside and inside the form', async () => {
-      const formClickSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault()));
+      const outsideSpy = /** @type {EventListener} */ (sinon.spy(e => e.preventDefault()));
-      const el = /** @type {HTMLFormElement} */ (await fixture(
+      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`
-          <form @click="${formClickSpy}">
+          <div @click="${outsideSpy}">
-            <lion-button>foo</lion-button>
+            <form @click="${formSpyEarly}">
-          </form>
+              <div @click="${insideSpy}">
+                <lion-button>foo</lion-button>
+              </div>
+            </form>
+          </div>
         `,
       ));
+      const lionButton = /** @type {LionButton} */ (el.querySelector('lion-button'));
+      const form = /** @type {HTMLFormElement} */ (el.querySelector('form'));
+      form.addEventListener('click', formSpyLater);
 
-      // @ts-ignore
+      lionButton.click();
-      el.querySelector('lion-button').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`
+          <form>
+            <lion-button>foo</lion-button>
+          </form>
+        `,
+      ));
+      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`
+          <div @click="${outsideSpy}">
+            <form @click="${formSpyEarly}">
+              <div @click="${insideSpy}">${lionButton}</div>
+            </form>
+          </div>
+        `,
+      ));
+      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 () => {
@@ -490,17 +547,17 @@ describe('lion-button', () => {
       });
 
       const useCases = [
-        { container: 'div', type: 'submit', targetHost: true },
+        { container: 'div', type: 'submit' },
-        { container: 'div', type: 'reset', targetHost: true },
+        { container: 'div', type: 'reset' },
-        { container: 'div', type: 'button', targetHost: true },
+        { container: 'div', type: 'button' },
-        { container: 'form', type: 'submit', targetHost: false },
+        { container: 'form', type: 'submit' },
-        { container: 'form', type: 'reset', targetHost: false },
+        { container: 'form', type: 'reset' },
-        { container: 'form', type: 'button', targetHost: true },
+        { container: 'form', type: 'button' },
       ];
 
       useCases.forEach(useCase => {
-        const { container, type, targetHost } = useCase;
+        const { container, type } = useCase;
-        const targetName = targetHost ? 'host' : 'native button';
+        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(
@@ -510,11 +567,7 @@ describe('lion-button', () => {
           await fixture(html`<${tag} @click="${clickSpy}">${el}</${tag}>`);
           const event = await prepareClickEvent(el);
 
-          if (targetHost) {
+          expect(event.target).to.equal(el);
-            expect(event.target).to.equal(el);
-          } else {
-            expect(event.target).to.equal(el._nativeButtonNode);
-          }
         });
       });
     });