fix: fire click on Enter and Space, but not when disabled (#2438)

packages/button/src/vaadin-button.js

@@ -137,6 +137,31 @@ class Button extends ActiveMixin(TabindexMixin(FocusMixin(ElementMixin(ThemableM
       this.setAttribute('role', 'button');
     }
   }
+
+  /**
+   * Since the button component is designed on the base of the `[role=button]` attribute,
+   * and doesn't have a native <button> inside, in order to be fully accessible from the keyboard,
+   * it should manually fire the `click` event once an activation key is pressed,
+   * as it follows from the WAI-ARIA specifications:
+   * https://www.w3.org/TR/wai-aria-practices-1.1/#button
+   *
+   * According to the UI Events specifications,
+   * the `click` event should be fired exactly on `keydown`:
+   * https://www.w3.org/TR/uievents/#event-type-keydown
+   *
+   * @param {KeyboardEvent} event
+   * @protected
+   * @override
+   */
+  _onKeyDown(event) {
+    super._onKeyDown(event);
+
+    if (this._activeKeys.includes(event.key)) {
+      // `DisabledMixin` overrides the standard `click()` method
+      // so that it doesn't fire the `click` event when the element is disabled.
+      this.click();
+    }
+  }
 }
 
 customElements.define(Button.is, Button);

packages/button/test/button.test.js

@@ -1,3 +1,4 @@
+import sinon from 'sinon';
 import { expect } from '@esm-bundle/chai';
 import { sendKeys } from '@web/test-runner-commands';
 import {
@@ -72,6 +73,34 @@ describe('vaadin-button', () => {
     });
   });
 
+  describe('keyboard', () => {
+    beforeEach(() => {
+      element = fixtureSync('<vaadin-button>Press me</vaadin-button>');
+      element.focus();
+    });
+
+    ['Enter', 'Space'].forEach((key) => {
+      it(`should fire click event on ${key}`, async () => {
+        const spy = sinon.spy();
+        element.addEventListener('click', spy);
+
+        await sendKeys({ down: key });
+
+        expect(spy.calledOnce).to.be.true;
+      });
+
+      it(`should not fire click event on ${key} when disabled`, async () => {
+        const spy = sinon.spy();
+        element.addEventListener('click', spy);
+        element.disabled = true;
+
+        await sendKeys({ down: key });
+
+        expect(spy.called).to.be.false;
+      });
+    });
+  });
+
   describe('mixins', () => {
     beforeEach(() => {
       element = fixtureSync('<vaadin-button>Press me</vaadin-button>');

packages/field-base/src/disabled-mixin.js

@@ -40,6 +40,18 @@ const DisabledMixinImplementation = (superclass) =>
         this.removeAttribute('aria-disabled');
       }
     }
+
+    /**
+     * Overrides the default element `click` method in order to prevent
+     * firing the `click` event when the element is disabled.
+     *
+     * @override
+     */
+    click() {
+      if (!this.disabled) {
+        super.click();
+      }
+    }
   };
 
 /**

packages/field-base/test/disabled-mixin.test.js

@@ -1,3 +1,4 @@
+import sinon from 'sinon';
 import { expect } from '@esm-bundle/chai';
 import { fixtureSync } from '@vaadin/testing-helpers';
 import { PolymerElement, html } from '@polymer/polymer/polymer-element.js';
@@ -38,4 +39,12 @@ describe('disabled-mixin', () => {
     element.disabled = false;
     expect(element.hasAttribute('aria-disabled')).to.be.false;
   });
+
+  it('should prevent firing click event when disabled', () => {
+    const spy = sinon.spy();
+    element.addEventListener('click', spy);
+    element.disabled = true;
+    element.click();
+    expect(spy.called).to.be.false;
+  });
 });

packages/vaadin-app-layout/src/vaadin-drawer-toggle.js

@@ -80,22 +80,9 @@ class DrawerToggleElement extends Button {
     super();
 
     this.addEventListener('click', () => {
-      if (!this.disabled) {
-        this.__fireClick();
-      }
-    });
-
-    this.addEventListener('keyup', (event) => {
-      if (/^( |SpaceBar|Enter)$/.test(event.key) && !this.disabled) {
-        this.__fireClick();
-      }
+      this.dispatchEvent(new CustomEvent('drawer-toggle-click', { bubbles: true, composed: true }));
     });
   }
-
-  /** @private */
-  __fireClick() {
-    this.dispatchEvent(new CustomEvent('drawer-toggle-click', { bubbles: true, composed: true }));
-  }
 }
 
 customElements.define(DrawerToggleElement.is, DrawerToggleElement);