refactor(button): several improvements (#1019)

- remove click-area --> move styles to host::before
- reduce css so that extending styles makes sense. Merge .btn with host
- reduce the template and remove the if else construction inside the template
- hide focus styles if they're not needed, for example, when an element receives focus via the mouse.
- improve __clickDelegationHandler. Use current slotted native button instead of create new one
- fix vertical align when 2 buttons are inline and one has icon. Example included.
- add min-with to ensure target size for mobile, not only height
- add test to check event.target in all situations (inside and outside form)

.changeset/two-rats-tease.md

@@ -0,0 +1,12 @@
+---
+"@lion/button": minor
+---
+
+Several button improvements
+
+- remove click-area --> move styles to host::before
+- reduce css so that extending styles makes sense. Merge .btn with host.
+- reduce the template and remove the if else construction inside the template.
+- hide focus styles if they're not needed, for example, when an element receives focus via the mouse.
+- improve __clickDelegationHandler. Use current slotted native button instead of create new one.
+- fix vertical alignment when 2 buttons are inline and one has icon. Example included.

packages/button/README.md

@@ -16,7 +16,7 @@ export default {
 ```
 
 ```js preview-story
-export const main = () => html` <lion-button>Default</lion-button> `;
+export const main = () => html`<lion-button>Default</lion-button>`;
 ```
 
 ## Features
@@ -60,20 +60,43 @@ export const handler = () => html`
 ### Icon button
 
 ```js preview-story
-export const iconButton = () => html` <lion-button>${iconSvg(html)} Bug</lion-button> `;
+export const iconButton = () => html`<lion-button>${iconSvg(html)}Bug</lion-button>`;
 ```
 
 ### Icon only button
 
 ```js preview-story
-export const iconOnly = () =>
+export const iconOnly = () => html`<lion-button aria-label="Bug">${iconSvg(html)}</lion-button>`;
-  html` <lion-button aria-label="Bug"> ${iconSvg(html)} </lion-button> `;
 ```
 
 ### Disabled button
 
 ```js preview-story
-export const disabled = () => html` <lion-button disabled>Disabled</lion-button> `;
+export const disabled = () => html`<lion-button disabled>Disabled</lion-button>`;
+```
+
+### Multiple buttons inline
+
+```js preview-story
+export const mainAndIconButton = () => html`
+  <lion-button>Default</lion-button>
+  <lion-button>${iconSvg(html)} Bug</lion-button>
+`;
+```
+
+### Small button (minimum click area showed)
+
+```js preview-story
+export const smallButton = () => html` <style>
+    .small {
+      padding: 4px;
+      line-height: 1em;
+    }
+    .small::before {
+      border: 1px dashed #000;
+    }
+  </style>
+  <lion-button class="small">xs</lion-button>`;
 ```
 
 ### Usage with native form
@@ -90,14 +113,14 @@ export const withinForm = () => html`
   <form
     @submit=${ev => {
       ev.preventDefault();
-      console.log('submit handler');
+      console.log('submit handler', ev.target);
     }}
   >
     <label for="firstNameId">First name</label>
     <input id="firstNameId" name="firstName" />
     <label for="lastNameId">Last name</label>
     <input id="lastNameId" name="lastName" />
-    <lion-button @click=${() => console.log('click handler')}>Submit</lion-button>
+    <lion-button @click=${ev => console.log('click handler', ev.target)}>Submit</lion-button>
   </form>
 `;
 ```

packages/button/src/LionButton.js

@@ -8,16 +8,6 @@ import {
 } from '@lion/core';
 import '@lion/core/src/differentKeyEventNamesShimIE.js';
 
-// TODO: several improvements:
-// [1] remove click-area
-// [2] remove the native _button slot. We can detect and submit parent form without the slot.
-// [3] reduce css so that extending styles makes sense. Merge .btn with host
-// [4] reduce the template and remove the if else construction inside the template (an extra
-// div by default to support IE is fine) => <div id="${this._buttonId}"><slot></slot></div>
-// should be all needed
-// [5] do we need the before and after templates? Could be added by subclasser
-// [6] extract all functionality (except for form submission) into LionButtonMixin
-
 const isKeyboardClickEvent = (/** @type {KeyboardEvent} */ e) => e.key === ' ' || e.key === 'Enter';
 const isSpaceKeyboardClickEvent = (/** @type {KeyboardEvent} */ e) => e.key === ' ';
 
@@ -41,15 +31,10 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
 
   render() {
     return html`
-      <div class="btn">
+      ${this._beforeTemplate()}
-        <div class="click-area"></div>
+      <div class="button-content" id="${this._buttonId}"><slot></slot></div>
-        ${this._beforeTemplate()}
+      ${this._afterTemplate()}
-        ${browserDetection.isIE11
+      <slot name="_button"></slot>
-          ? html`<div id="${this._buttonId}"><slot></slot></div>`
-          : html`<slot></slot>`}
-        ${this._afterTemplate()}
-        <slot name="_button"></slot>
-      </div>
     `;
   }
 
@@ -67,24 +52,39 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
     return [
       css`
         :host {
+          position: relative;
           display: inline-block;
-          min-height: 40px; /* src = https://www.smashingmagazine.com/2012/02/finger-friendly-design-ideal-mobile-touchscreen-target-sizes/ */
-          outline: 0;
-          background-color: transparent;
           box-sizing: border-box;
+          vertical-align: middle;
+          line-height: 24px;
+          background: #eee; /* minimal styling to make it recognizable as btn */
+          padding: 8px; /* padding to fix with min-height */
+          outline: none; /* focus style handled below */
+          cursor: default; /* /* we should always see the default arrow, never a caret */
         }
 
-        .btn {
+        :host::before {
-          min-height: 24px;
+          content: '';
+
+          /* center vertically and horizontally */
+          position: absolute;
+          top: 50%;
+          left: 50%;
+          transform: translate(-50%, -50%);
+
+          /* touch area (comes into play when button height goes below this one) */
+          /* src = https://www.smashingmagazine.com/2012/02/finger-friendly-design-ideal-mobile-touchscreen-target-sizes/ */
+          min-height: 40px;
+          min-width: 40px;
+        }
+
+        .button-content {
           display: flex;
           align-items: center;
-          position: relative;
+          justify-content: center;
-          background: #eee; /* minimal styling to make it recognizable as btn */
-          padding: 8px; /* vertical padding to fix with host min-height */
-          outline: none; /* focus style handled below, else it follows boundaries of click-area */
         }
 
-        :host .btn ::slotted(button) {
+        :host ::slotted(button) {
           position: absolute;
           top: 0;
           left: 0;
@@ -98,41 +98,36 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
           border: 0; /* reset default agent styles */
         }
 
-        .click-area {
+        /* Show focus styles on keyboard focus. */
-          position: absolute;
+        :host(:focus:not([disabled])),
-          top: 0;
+        :host(:focus-visible) {
-          right: 0;
-          bottom: 0;
-          left: 0;
-          margin: 0;
-          padding: 0;
-        }
-
-        :host(:focus:not([disabled])) .btn {
           /* if you extend, please overwrite */
           outline: 2px solid #bde4ff;
         }
 
-        :host(:hover) .btn {
+        /* Hide focus styles if they're not needed, for example,
+        when an element receives focus via the mouse. */
+        :host(:focus:not(:focus-visible)) {
+          outline: 0;
+        }
+
+        :host(:hover) {
           /* if you extend, please overwrite */
           background: #f4f6f7;
         }
 
-        :host(:active) .btn, /* keep native :active to render quickly where possible */
+        :host(:active), /* keep native :active to render quickly where possible */
-        :host([active]) .btn /* use custom [active] to fix IE11 */ {
+        :host([active]) /* use custom [active] to fix IE11 */ {
           /* if you extend, please overwrite */
           background: gray;
         }
 
-        :host([disabled]) {
-          pointer-events: none;
-        }
-
         :host([hidden]) {
           display: none;
         }
 
-        :host([disabled]) .btn {
+        :host([disabled]) {
+          pointer-events: none;
           /* if you extend, please overwrite */
           background: lightgray;
           color: #adadad;
@@ -213,14 +208,9 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
    * @param {Event} e
    */
   __clickDelegationHandler(e) {
-    if ((this.type === 'submit' || this.type === 'reset') && e.target === this) {
+    if ((this.type === 'submit' || this.type === 'reset') && e.target === this && this._form) {
-      if (this._form) {
+      e.stopImmediatePropagation();
-        const nativeButton = document.createElement('button');
+      this._nativeButtonNode.click();
-        nativeButton.type = this.type;
-        this._form.appendChild(nativeButton);
-        nativeButton.click();
-        this._form.removeChild(nativeButton);
-      }
     }
   }
 
@@ -240,6 +230,7 @@ export class LionButton extends DisabledWithTabIndexMixin(SlotMixin(LitElement))
     this.removeEventListener('mousedown', this.__mousedownHandler);
     this.removeEventListener('keydown', this.__keydownHandler);
     this.removeEventListener('keyup', this.__keyupHandler);
+    this.removeEventListener('click', this.__clickDelegationHandler);
   }
 
   __mousedownHandler() {

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

@@ -1,5 +1,5 @@
 import { browserDetection } from '@lion/core';
-import { aTimeout, expect, fixture, html, oneEvent } from '@open-wc/testing';
+import { aTimeout, expect, fixture, html, oneEvent, unsafeStatic } from '@open-wc/testing';
 import sinon from 'sinon';
 import '@lion/core/src/differentKeyEventNamesShimIE.js';
 import '../lion-button.js';
@@ -206,7 +206,7 @@ describe('lion-button', () => {
       const wrapperId = el.getAttribute('aria-labelledby');
       expect(/** @type {ShadowRoot} */ (el.shadowRoot).querySelector(`#${wrapperId}`)).to.exist;
       expect(/** @type {ShadowRoot} */ (el.shadowRoot).querySelector(`#${wrapperId}`)).dom.to.equal(
-        `<div id="${wrapperId}"><slot></slot></div>`,
+        `<div class="button-content" id="${wrapperId}"><slot></slot></div>`,
       );
       browserDetectionStub.restore();
     });
@@ -397,6 +397,16 @@ describe('lion-button', () => {
   });
 
   describe('click event', () => {
+    /**
+     * @param {HTMLButtonElement | LionButton} el
+     */
+    async function prepareClickEvent(el) {
+      setTimeout(() => {
+        el.click();
+      });
+      return oneEvent(el, 'click');
+    }
+
     it('is fired once', async () => {
       const clickSpy = /** @type {EventListener} */ (sinon.spy());
       const el = /** @type {LionButton} */ (await fixture(
@@ -412,16 +422,25 @@ 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`<form @click="${formClickSpy}">
+          <lion-button>foo</lion-button>
+        </form>`,
+      ));
+
+      // @ts-ignore
+      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;
+    });
+
     describe('native button behavior', async () => {
-      /**
-       * @param {HTMLButtonElement | LionButton} el
-       */
-      async function prepareClickEvent(el) {
-        setTimeout(() => {
-          el.click();
-        });
-        return oneEvent(el, 'click');
-      }
       /** @type {Event} */
       let nativeButtonEvent;
       /** @type {Event} */
@@ -450,12 +469,43 @@ describe('lion-button', () => {
           expect(lionButtonEvent[property]).to.equal(nativeButtonEvent[property]);
         });
       });
+    });
 
-      it('has host in the target property', async () => {
+    describe('event target', async () => {
+      it('is host by default', async () => {
         const el = /** @type {LionButton} */ (await fixture('<lion-button>foo</lion-button>'));
         const event = await prepareClickEvent(el);
         expect(event.target).to.equal(el);
       });
+
+      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 },
+      ];
+
+      useCases.forEach(useCase => {
+        const { container, type, targetHost } = useCase;
+        const targetName = targetHost ? 'host' : 'native button';
+        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(
+            `<lion-button type="${type}">foo</lion-button>`,
+          ));
+          const tag = unsafeStatic(container);
+          await fixture(html`<${tag} @click="${clickSpy}">${el}</${tag}>`);
+          const event = await prepareClickEvent(el);
+
+          if (targetHost) {
+            expect(event.target).to.equal(el);
+          } else {
+            expect(event.target).to.equal(el._nativeButtonNode);
+          }
+        });
+      });
     });
   });
 });