Merge pull request #328 from ing-bank/fix/button-submit

fix(button): remove submit() on click

packages/button/README.md

@@ -28,15 +28,38 @@ import '@lion/button/lion-button.js';
 <lion-button>Button Text</lion-button>
 ```
 
-- Don't use a button when you want a user to navigate. Use a link instead.
-- Not all color and font size combinations are available because some do not meet accessibility contrast requirements
-
 ## Considerations
 
-### Why a webcomponent?
+### Why a Web Component?
 
-There are multiple reasons why we used a web component as opposed to a CSS component.
+There are multiple reasons why we used a Web Component as opposed to a CSS component.
 
 - **Target size**: The minimum target size is 40 pixels, which makes even the small buttons easy to activate. A container element was needed to make this size possible.
 - **Accessibility**: Our button is accessible because it uses the native button element. Having this native button element available in the light dom, preserves all platform accessibility features, like having it recognized by a native form.
 - **Advanced styling**: There are advanced styling options regarding icons in buttons, where it is a lot more maintainable to handle icons in our button using slots. An example is that a sticky icon-only buttons may looks different from buttons which have both icons and text.
+
+### Event target
+
+We want to ensure that the event target returned to the user is `lion-button`, not `button`. Therefore, simply delegating the click to the native button immediately, is not desired. Instead, we catch the click event in the `lion-button`, and ensure delegation inside of there.
+
+### Flashing a native button click as a direct child of form
+
+By delegating the `click()` to the native button, it will bubble back up to `lion-button` which would cause duplicate actions. We have to simulate the full `.click()` however, otherwise form submission is not triggered. So this bubbling cannot be prevented.
+Therefore, on click, we flash a `<button>` to the form as a direct child and fire the click on that button. We then immediately remove that button. This is a fully synchronous process; users or developers will not notice this, it should not cause problems.
+
+### Native button & implicit form submission
+
+Flashing the button in the way we do solves almost all issues except for one.
+One of the specs of W3C is that when you have a form with multiple inputs, pressing enter while inside one of the inputs only triggers a form submit if that form has a button of type submit.
+
+To get this particular implicit form submission to work, having a native button in our `lion-button` is a hard requirement. Therefore, not only do we flash a native button on the form to delegate `lion-button` trigger to `button` and thereby trigger form submission, we **also** add a native `button` inside the `lion-button` which `type` property is synchronized with the type of the `lion-button`.
+
+### Preventing full page reloads
+
+To prevent form submission full page reloads, add a **submit handler on the form** like so:
+
+```html
+<form @submit=${ev => ev.preventDefault()} >
+```
+
+Putting this on the `@click` of the `lion-button` is not enough.

packages/button/src/LionButton.js

@@ -1,5 +1,8 @@
 import { css, html, SlotMixin, DisabledWithTabIndexMixin, LitElement } from '@lion/core';
 
+// eslint-disable-next-line class-methods-use-this
+const isKeyboardClickEvent = e => e.keyCode === 32 /* space */ || e.keyCode === 13; /* enter */
+
 export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement)) {
   static get properties() {
     return {
@@ -119,6 +122,14 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
     ];
   }
 
+  get _nativeButtonNode() {
+    return this.querySelector('[slot=_button]');
+  }
+
+  get _form() {
+    return this._nativeButtonNode.form;
+  }
+
   get slots() {
     return {
       ...super.slots,
@@ -132,10 +143,6 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
     };
   }
 
-  get _nativeButtonNode() {
-    return this.querySelector('[slot=_button]');
-  }
-
   constructor() {
     super();
     this.role = 'button';
@@ -172,12 +179,23 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
   }
 
   /**
-   * Dispatch submit event and invoke submit on the native form when clicked
+   * 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.
    */
-  __clickDelegationHandler() {
+  __clickDelegationHandler(e) {
-    if (this.type === 'submit' && this._nativeButtonNode && this._nativeButtonNode.form) {
+    if (this.constructor.__isIE11()) {
-      this._nativeButtonNode.form.dispatchEvent(new Event('submit'));
+      e.stopPropagation();
-      this._nativeButtonNode.form.submit();
+    }
+
+    if ((this.type === 'submit' || this.type === 'reset') && e.target === this) {
+      if (this._form) {
+        const nativeButton = document.createElement('button');
+        nativeButton.type = this.type;
+        this._form.appendChild(nativeButton);
+        nativeButton.click();
+        this._form.removeChild(nativeButton);
+      }
     }
   }
 
@@ -209,12 +227,13 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
   }
 
   __keydownHandler(e) {
-    if (this.active || !this.__isKeyboardClickEvent(e)) {
+    if (this.active || !isKeyboardClickEvent(e)) {
       return;
     }
+    // FIXME: In Edge & IE11, this toggling the active state to prevent bounce, does not work.
     this.active = true;
     const keyupHandler = keyupEvent => {
-      if (this.__isKeyboardClickEvent(keyupEvent)) {
+      if (isKeyboardClickEvent(keyupEvent)) {
         this.active = false;
         document.removeEventListener('keyup', keyupHandler, true);
       }
@@ -223,17 +242,16 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
   }
 
   __keyupHandler(e) {
-    if (this.__isKeyboardClickEvent(e)) {
+    if (isKeyboardClickEvent(e)) {
-      // redispatch click
+      // Fixes IE11 double submit/click. Enter keypress somehow triggers the __keyUpHandler on the native <button>
+      if (e.srcElement && e.srcElement !== this) {
+        return;
+      }
+      // dispatch click
       this.click();
     }
   }
 
-  // eslint-disable-next-line class-methods-use-this
-  __isKeyboardClickEvent(e) {
-    return e.keyCode === 32 /* space */ || e.keyCode === 13 /* enter */;
-  }
-
   static __isIE11() {
     const ua = window.navigator.userAgent;
     const result = /Trident/.test(ua);

packages/button/stories/index.stories.js

@@ -33,16 +33,59 @@ storiesOf('Buttons|Button', module)
     `,
   )
   .add(
-    'Within a form',
+    'Within a native form',
     () => html`
-      <form @submit=${() => console.log('native form submitted')}>
+      <form
-        <input name="foo" label="Foo" .modelValue=${'bar'} />
+        @submit=${ev => {
-        <input name="foo2" label="Foo2" .modelValue=${'bar'} />
+          ev.preventDefault();
-        <lion-button
+          console.log('submit handler');
-          type="submit"
+        }}
-          @click=${() => console.log(document.querySelector('#form').serializeGroup())}
+      >
-          >Submit</lion-button
+        <label>First name</label>
-        >
+        <input name="firstName" />
+        <label>Last name</label>
+        <input name="lastName" />
+        <lion-button @click=${() => console.log('click handler')}>Submit</lion-button>
       </form>
+      <p>
+        Supports the following use cases:
+      </p>
+      <ul>
+        <li>
+          Submit on button click
+        </li>
+        <li>
+          Reset native form fields when using type="reset"
+        </li>
+        <li>
+          Submit on button enter or space keypress
+        </li>
+        <li>
+          Submit on enter keypress inside an input
+        </li>
+      </ul>
+      <p>Important notes:</p>
+      <ul>
+        <li>
+          A (lion)-button of type submit is mandatory for the last use case, if you have multiple
+          inputs. This is native behavior.
+        </li>
+        <li>
+          <span style="background-color: azure">
+            <code>@click</code> on <code>lion-button</code>
+          </span>
+          and
+          <span style="background-color: seashell">
+            <code>@submit</code> on <code>form</code>
+          </span>
+          are triggered by these use cases. We strongly encourage you to listen to the submit
+          handler if your goal is to do something on form-submit
+        </li>
+        <li>
+          To prevent form submission full page reloads, add a <b>submit handler on the form</b>
+          <code>@submit</code> with <code>event.preventDefault()</code>. Adding it on the
+          <code>lion-button</code> is not enough.
+        </li>
+      </ul>
     `,
   );

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

@@ -225,74 +225,155 @@ describe('lion-button', () => {
   });
 
   describe('form integration', () => {
-    it('behaves like native `button` when clicked', async () => {
+    describe('with submit event', () => {
-      const formSubmitSpy = sinon.spy(e => e.preventDefault());
+      it('behaves like native `button` when clicked', async () => {
-      const form = await fixture(html`
+        const formSubmitSpy = sinon.spy(e => e.preventDefault());
-        <form @submit="${formSubmitSpy}">
+        const form = await fixture(html`
-          <lion-button type="submit">foo</lion-button>
+          <form @submit="${formSubmitSpy}">
-        </form>
+            <lion-button type="submit">foo</lion-button>
-      `);
+          </form>
-      // Prevent page refresh
+        `);
-      form.submit = () => {};
 
-      const button = form.querySelector('lion-button');
+        const button = form.querySelector('lion-button');
-      getTopElement(button).click();
+        getTopElement(button).click();
 
-      expect(formSubmitSpy.called).to.be.true;
+        expect(formSubmitSpy.callCount).to.equal(1);
+      });
+
+      it('behaves like native `button` when interacted with keyboard space', async () => {
+        const formSubmitSpy = sinon.spy(e => e.preventDefault());
+        const form = await fixture(html`
+          <form @submit="${formSubmitSpy}">
+            <lion-button type="submit">foo</lion-button>
+          </form>
+        `);
+
+        pressSpace(form.querySelector('lion-button'));
+        await aTimeout();
+        await aTimeout();
+
+        expect(formSubmitSpy.callCount).to.equal(1);
+      });
+
+      it('behaves like native `button` when interacted with keyboard enter', async () => {
+        const formSubmitSpy = sinon.spy(e => e.preventDefault());
+        const form = await fixture(html`
+          <form @submit="${formSubmitSpy}">
+            <lion-button type="submit">foo</lion-button>
+          </form>
+        `);
+
+        pressEnter(form.querySelector('lion-button'));
+        await aTimeout();
+        await aTimeout();
+
+        expect(formSubmitSpy.callCount).to.equal(1);
+      });
+
+      it('supports resetting form inputs in a native form', async () => {
+        const form = await fixture(html`
+          <form>
+            <input name="firstName" />
+            <input name="lastName" />
+            <lion-button type="reset">reset</lion-button>
+          </form>
+        `);
+        const btn = form.querySelector('lion-button');
+        const firstName = form.querySelector('input[name=firstName]');
+        const lastName = form.querySelector('input[name=lastName]');
+        firstName.value = 'Foo';
+        lastName.value = 'Bar';
+
+        expect(firstName.value).to.equal('Foo');
+        expect(lastName.value).to.equal('Bar');
+
+        btn.click();
+
+        expect(firstName.value).to.be.empty;
+        expect(lastName.value).to.be.empty;
+      });
+
+      // input "enter" keypress mock doesn't seem to work right now, but should be tested in the future (maybe with Selenium)
+      it.skip('works with implicit form submission on-enter inside an input', async () => {
+        const formSubmitSpy = sinon.spy(e => e.preventDefault());
+        const form = await fixture(html`
+          <form @submit="${formSubmitSpy}">
+            <input name="foo" />
+            <input name="foo2" />
+            <lion-button type="submit">foo</lion-button>
+          </form>
+        `);
+
+        pressEnter(form.querySelector('input[name="foo2"]'));
+        await aTimeout();
+        await aTimeout();
+
+        expect(formSubmitSpy.callCount).to.equal(1);
+      });
     });
 
-    it('behaves like native `button` when interected with keyboard space', async () => {
+    describe('with click event', () => {
-      const formSubmitSpy = sinon.spy(e => e.preventDefault());
+      it('behaves like native `button` when clicked', async () => {
-      const form = await fixture(html`
+        const formButtonClickedSpy = sinon.spy();
-        <form @submit="${formSubmitSpy}">
+        const form = await fixture(html`
-          <lion-button type="submit">foo</lion-button>
+          <form @submit=${ev => ev.preventDefault()}>
-        </form>
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
-      `);
+          </form>
-      // Prevent page refresh
+        `);
-      form.submit = () => {};
 
-      pressSpace(form.querySelector('lion-button'));
+        const button = form.querySelector('lion-button');
-      await aTimeout();
+        getTopElement(button).click();
-      await aTimeout();
 
-      expect(formSubmitSpy.called).to.be.true;
+        expect(formButtonClickedSpy.callCount).to.equal(1);
-    });
+      });
 
-    it('behaves like native `button` when interected with keyboard enter', async () => {
+      it('behaves like native `button` when interacted with keyboard space', async () => {
-      const formSubmitSpy = sinon.spy(e => e.preventDefault());
+        const formButtonClickedSpy = sinon.spy();
-      const form = await fixture(html`
+        const form = await fixture(html`
-        <form @submit="${formSubmitSpy}">
+          <form @submit=${ev => ev.preventDefault()}>
-          <lion-button type="submit">foo</lion-button>
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
-        </form>
+          </form>
-      `);
+        `);
-      // Prevent page refresh
-      form.submit = () => {};
 
-      pressEnter(form.querySelector('lion-button'));
+        pressSpace(form.querySelector('lion-button'));
-      await aTimeout();
+        await aTimeout();
-      await aTimeout();
+        await aTimeout();
 
-      expect(formSubmitSpy.called).to.be.true;
+        expect(formButtonClickedSpy.callCount).to.equal(1);
-    });
+      });
 
-    // input "enter" keypress mock doesn't seem to work right now, but should be tested in the future (maybe with Selenium)
+      it('behaves like native `button` when interacted with keyboard enter', async () => {
-    it.skip('works with implicit form submission on-enter inside an input', async () => {
+        const formButtonClickedSpy = sinon.spy();
-      const formSubmitSpy = sinon.spy(e => e.preventDefault());
+        const form = await fixture(html`
-      const form = await fixture(html`
+          <form @submit=${ev => ev.preventDefault()}>
-        <form @submit="${formSubmitSpy}">
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
-          <input name="foo" />
+          </form>
-          <input name="foo2" />
+        `);
-          <lion-button type="submit">foo</lion-button>
-        </form>
-      `);
-      // Prevent page refresh
-      form.submit = () => {};
 
-      pressEnter(form.querySelector('input[name="foo2"]'));
+        pressEnter(form.querySelector('lion-button'));
-      await aTimeout();
+        await aTimeout();
-      await aTimeout();
+        await aTimeout();
 
-      expect(formSubmitSpy.called).to.be.true;
+        expect(formButtonClickedSpy.callCount).to.equal(1);
+      });
+
+      // input "enter" keypress mock doesn't seem to work right now, but should be tested in the future (maybe with Selenium)
+      it.skip('works with implicit form submission on-enter inside an input', async () => {
+        const formButtonClickedSpy = sinon.spy();
+        const form = await fixture(html`
+          <form @submit=${ev => ev.preventDefault()}>
+            <input name="foo" />
+            <input name="foo2" />
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
+          </form>
+        `);
+
+        pressEnter(form.querySelector('input[name="foo2"]'));
+        await aTimeout();
+        await aTimeout();
+
+        expect(formButtonClickedSpy.callCount).to.equal(1);
+      });
     });
   });