From d19ca7cc5c7e41c9b4ac6170540ad43a9e7face2 Mon Sep 17 00:00:00 2001
From: qa46hx <Gerjan.van.Geest@ing.com>
Date: Tue, 15 Oct 2019 13:24:08 +0200
Subject: [PATCH 1/2] fix(button): fix form integration, fires only once,
 submit preventable

Co-authored-by: Joren Broekema <Joren.Broekema@ing.com>
---
 packages/button/README.md                |  33 ++++-
 packages/button/src/LionButton.js        |  53 ++++---
 packages/button/stories/index.stories.js |  58 ++++++--
 packages/button/test/lion-button.test.js | 170 +++++++++++++++--------
 4 files changed, 226 insertions(+), 88 deletions(-)

diff --git a/packages/button/README.md b/packages/button/README.md
index a19ca1bf9..81bac11c2 100644
--- a/packages/button/README.md
+++ b/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.
diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js
index fe23a4ffc..7138a74ce 100644
--- a/packages/button/src/LionButton.js
+++ b/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,22 @@ 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() {
-    if (this.type === 'submit' && this._nativeButtonNode && this._nativeButtonNode.form) {
-      this._nativeButtonNode.form.dispatchEvent(new Event('submit'));
-      this._nativeButtonNode.form.submit();
+  __clickDelegationHandler(e) {
+    if (this.constructor.__isIE11()) {
+      e.stopPropagation();
+    }
+
+    if (this.type === 'submit' && e.target === this) {
+      if (this._form) {
+        const nativeButton = document.createElement('button');
+        this._form.appendChild(nativeButton);
+        nativeButton.click();
+        this._form.removeChild(nativeButton);
+      }
     }
   }
 
@@ -209,12 +226,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 +241,16 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
   }
 
   __keyupHandler(e) {
-    if (this.__isKeyboardClickEvent(e)) {
-      // redispatch click
+    if (isKeyboardClickEvent(e)) {
+      // 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);
diff --git a/packages/button/stories/index.stories.js b/packages/button/stories/index.stories.js
index bc9f02d12..264065655 100644
--- a/packages/button/stories/index.stories.js
+++ b/packages/button/stories/index.stories.js
@@ -33,16 +33,56 @@ storiesOf('Buttons|Button', module)
     `,
   )
   .add(
-    'Within a form',
+    'Within a native form',
     () => html`
-      <form @submit=${() => console.log('native form submitted')}>
-        <input name="foo" label="Foo" .modelValue=${'bar'} />
-        <input name="foo2" label="Foo2" .modelValue=${'bar'} />
-        <lion-button
-          type="submit"
-          @click=${() => console.log(document.querySelector('#form').serializeGroup())}
-          >Submit</lion-button
-        >
+      <form
+        @submit=${ev => {
+          ev.preventDefault();
+          console.log('submit handler');
+        }}
+      >
+        <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>
+          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>
     `,
   );
diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js
index 71b021624..ca3001324 100644
--- a/packages/button/test/lion-button.test.js
+++ b/packages/button/test/lion-button.test.js
@@ -225,74 +225,132 @@ describe('lion-button', () => {
   });
 
   describe('form integration', () => {
-    it('behaves like native `button` when clicked', async () => {
-      const formSubmitSpy = sinon.spy(e => e.preventDefault());
-      const form = await fixture(html`
-        <form @submit="${formSubmitSpy}">
-          <lion-button type="submit">foo</lion-button>
-        </form>
-      `);
-      // Prevent page refresh
-      form.submit = () => {};
+    describe('with submit event', () => {
+      it('behaves like native `button` when clicked', async () => {
+        const formSubmitSpy = sinon.spy(e => e.preventDefault());
+        const form = await fixture(html`
+          <form @submit="${formSubmitSpy}">
+            <lion-button type="submit">foo</lion-button>
+          </form>
+        `);
 
-      const button = form.querySelector('lion-button');
-      getTopElement(button).click();
+        const button = form.querySelector('lion-button');
+        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);
+      });
+
+      // 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 () => {
-      const formSubmitSpy = sinon.spy(e => e.preventDefault());
-      const form = await fixture(html`
-        <form @submit="${formSubmitSpy}">
-          <lion-button type="submit">foo</lion-button>
-        </form>
-      `);
-      // Prevent page refresh
-      form.submit = () => {};
+    describe('with click event', () => {
+      it('behaves like native `button` when clicked', async () => {
+        const formButtonClickedSpy = sinon.spy();
+        const form = await fixture(html`
+          <form @submit=${ev => ev.preventDefault()}>
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
+          </form>
+        `);
 
-      pressSpace(form.querySelector('lion-button'));
-      await aTimeout();
-      await aTimeout();
+        const button = form.querySelector('lion-button');
+        getTopElement(button).click();
 
-      expect(formSubmitSpy.called).to.be.true;
-    });
+        expect(formButtonClickedSpy.callCount).to.equal(1);
+      });
 
-    it('behaves like native `button` when interected 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>
-      `);
-      // Prevent page refresh
-      form.submit = () => {};
+      it('behaves like native `button` when interacted with keyboard space', async () => {
+        const formButtonClickedSpy = sinon.spy();
+        const form = await fixture(html`
+          <form @submit=${ev => ev.preventDefault()}>
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
+          </form>
+        `);
 
-      pressEnter(form.querySelector('lion-button'));
-      await aTimeout();
-      await aTimeout();
+        pressSpace(form.querySelector('lion-button'));
+        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 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>
-      `);
-      // Prevent page refresh
-      form.submit = () => {};
+      it('behaves like native `button` when interacted with keyboard enter', async () => {
+        const formButtonClickedSpy = sinon.spy();
+        const form = await fixture(html`
+          <form @submit=${ev => ev.preventDefault()}>
+            <lion-button @click="${formButtonClickedSpy}" type="submit">foo</lion-button>
+          </form>
+        `);
 
-      pressEnter(form.querySelector('input[name="foo2"]'));
-      await aTimeout();
-      await aTimeout();
+        pressEnter(form.querySelector('lion-button'));
+        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);
+      });
     });
   });
 

From 7995dfced561f3b0f6e6e3bceb97245449815771 Mon Sep 17 00:00:00 2001
From: Joren Broekema <joren.broekema@ing.com>
Date: Mon, 21 Oct 2019 08:18:03 +0200
Subject: [PATCH 2/2] chore(button): re-add and test 'type=reset' feature with
 native form

---
 packages/button/src/LionButton.js        |  3 ++-
 packages/button/stories/index.stories.js |  3 +++
 packages/button/test/lion-button.test.js | 23 +++++++++++++++++++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/packages/button/src/LionButton.js b/packages/button/src/LionButton.js
index 7138a74ce..5f92bb082 100644
--- a/packages/button/src/LionButton.js
+++ b/packages/button/src/LionButton.js
@@ -188,9 +188,10 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
       e.stopPropagation();
     }
 
-    if (this.type === 'submit' && e.target === this) {
+    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);
diff --git a/packages/button/stories/index.stories.js b/packages/button/stories/index.stories.js
index 264065655..2899060c5 100644
--- a/packages/button/stories/index.stories.js
+++ b/packages/button/stories/index.stories.js
@@ -54,6 +54,9 @@ storiesOf('Buttons|Button', module)
         <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>
diff --git a/packages/button/test/lion-button.test.js b/packages/button/test/lion-button.test.js
index ca3001324..eae39df63 100644
--- a/packages/button/test/lion-button.test.js
+++ b/packages/button/test/lion-button.test.js
@@ -270,6 +270,29 @@ describe('lion-button', () => {
         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());