don't emit click when disabled; fixes #1113

docs/resources/changelog.md

@@ -14,6 +14,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 - Fixed a bug in `<sl-dropdown>` that prevented keyboard users from selecting menu items when using the keyboard [#1165](https://github.com/shoelace-style/shoelace/issues/1165)
 - Fixed a bug in the template for `<sl-select>` that caused the `form-control-help-text` part to not be in the same location as other form controls [#1178](https://github.com/shoelace-style/shoelace/issues/1178)
 - Fixed a bug in `<sl-checkbox>` and `<sl-switch>` that caused the browser to scroll incorrectly when focusing on a control in a container with overflow [#1169](https://github.com/shoelace-style/shoelace/issues/1169)
+- Fixed a bug in `<sl-menu-item>` that caused the `click` event to be emitted when the item was disabled [#1113](https://github.com/shoelace-style/shoelace/issues/1113)
 
 ## 2.0.0
 

src/components/button/button.ts

@@ -139,6 +139,17 @@ export default class SlButton extends ShoelaceElement implements ShoelaceFormCon
   /** Used to override the form owner's `target` attribute. */
   @property({ attribute: 'formtarget' }) formTarget: '_self' | '_blank' | '_parent' | '_top' | string;
 
+  connectedCallback() {
+    super.connectedCallback();
+    this.handleHostClick = this.handleHostClick.bind(this);
+    this.addEventListener('click', this.handleHostClick);
+  }
+
+  disconnectedCallback() {
+    super.disconnectedCallback();
+    this.removeEventListener('click', this.handleHostClick);
+  }
+
   firstUpdated() {
     if (this.isButton()) {
       this.formControlController.updateValidity();
@@ -155,13 +166,7 @@ export default class SlButton extends ShoelaceElement implements ShoelaceFormCon
     this.emit('sl-focus');
   }
 
-  private handleClick(event: MouseEvent) {
+  private handleClick() {
-    if (this.disabled || this.loading) {
-      event.preventDefault();
-      event.stopPropagation();
-      return;
-    }
-
     if (this.type === 'submit') {
       this.formControlController.submit(this);
     }
@@ -171,6 +176,14 @@ export default class SlButton extends ShoelaceElement implements ShoelaceFormCon
     }
   }
 
+  private handleHostClick(event: MouseEvent) {
+    // Prevent the click event from being emitted when the button is disabled or loading
+    if (this.disabled || this.loading) {
+      event.preventDefault();
+      event.stopImmediatePropagation();
+    }
+  }
+
   private isButton() {
     return this.href ? false : true;
   }

src/components/menu-item/menu-item.test.ts

@@ -1,4 +1,5 @@
-import { aTimeout, expect, fixture, html, waitUntil } from '@open-wc/testing';
+import { clickOnElement } from '../../internal/test';
+import { expect, fixture, html, waitUntil } from '@open-wc/testing';
 import sinon from 'sinon';
 import type SlMenuItem from './menu-item';
 
@@ -26,13 +27,20 @@ describe('<sl-menu-item>', () => {
   });
 
   it('should render the correct aria attributes when disabled', async () => {
-    const el = await fixture<SlMenuItem>(html` <sl-menu-item>Test</sl-menu-item> `);
+    const el = await fixture<SlMenuItem>(html` <sl-menu-item disabled>Test</sl-menu-item> `);
-
-    el.disabled = true;
-    await aTimeout(100);
     expect(el.getAttribute('aria-disabled')).to.equal('true');
   });
 
+  it('should not emit the click event when disabled', async () => {
+    const el = await fixture<SlMenuItem>(html` <sl-menu-item disabled>Test</sl-menu-item> `);
+    const clickHandler = sinon.spy();
+    el.addEventListener('click', clickHandler);
+    await clickOnElement(el);
+    await el.updateComplete;
+
+    expect(clickHandler).to.not.have.been.called;
+  });
+
   it('should return a text label when calling getTextLabel()', async () => {
     const el = await fixture<SlMenuItem>(html` <sl-menu-item>Test</sl-menu-item> `);
     expect(el.getTextLabel()).to.equal('Test');

src/components/menu-item/menu-item.ts

@@ -47,6 +47,17 @@ export default class SlMenuItem extends ShoelaceElement {
   /** Draws the menu item in a disabled state, preventing selection. */
   @property({ type: Boolean, reflect: true }) disabled = false;
 
+  connectedCallback() {
+    super.connectedCallback();
+    this.handleHostClick = this.handleHostClick.bind(this);
+    this.addEventListener('click', this.handleHostClick);
+  }
+
+  disconnectedCallback() {
+    super.disconnectedCallback();
+    this.removeEventListener('click', this.handleHostClick);
+  }
+
   private handleDefaultSlotChange() {
     const textLabel = this.getTextLabel();
 
@@ -63,6 +74,14 @@ export default class SlMenuItem extends ShoelaceElement {
     }
   }
 
+  private handleHostClick(event: MouseEvent) {
+    // Prevent the click event from being emitted when the button is disabled or loading
+    if (this.disabled) {
+      event.preventDefault();
+      event.stopImmediatePropagation();
+    }
+  }
+
   @watch('checked')
   handleCheckedChange() {
     // For proper accessibility, users have to use type="checkbox" to use the checked attribute