Fire `sl-select` when clicking an element inside a menu-item (#1599)

* Fire sl-select when clicking an element inside a menu-item

* changelog + remove unused code

* prettier

* prettier

---------

Co-authored-by: Cory LaViska <cory@abeautifulsite.net>

docs/pages/resources/changelog.md

@@ -18,6 +18,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti
 - Fixed a bug [in the localize dependency](https://github.com/shoelace-style/localize/issues/20) that caused underscores in language codes to throw a `RangeError`
 - Fixed a bug in the focus trapping utility used by modals that caused unexpected focus behavior. [#1583]
 - Fixed a bug in `<sl-copy-button>` that prevented exported tooltip parts from being styled [#1586]
+- Fixed a bug in `<sl-menu>` that caused it not to fire the `sl-select` event if you clicked an element inside of a `<sl-menu-item>` [#1599]
 - Improved submenu selection by implementing the [safe triangle](https://www.smashingmagazine.com/2023/08/better-context-menus-safe-triangles/) method [#1550]
 - Updated `@shoelace-style/localize` to 3.1.0
 

src/components/menu/menu.component.ts

@@ -1,9 +1,9 @@
 import { html } from 'lit';
 import { query } from 'lit/decorators.js';
 import ShoelaceElement from '../../internal/shoelace-element.js';
-import SlMenuItem from '../menu-item/menu-item.component.js';
 import styles from './menu.styles.js';
 import type { CSSResultGroup } from 'lit';
+import type SlMenuItem from '../menu-item/menu-item.component.js';
 export interface MenuSelectEventDetail {
   item: SlMenuItem;
 }
@@ -29,11 +29,14 @@ export default class SlMenu extends ShoelaceElement {
   }
 
   private handleClick(event: MouseEvent) {
-    if (!(event.target instanceof SlMenuItem)) {
-      return;
-    }
+    const menuItemTypes = ['menuitem', 'menuitemcheckbox'];
 
-    const item: SlMenuItem = event.target;
+    const target = event.composedPath().find((el: Element) => menuItemTypes.includes(el?.getAttribute?.('role') || ''));
+
+    if (!target) return;
+
+    // This isn't true. But we use it for TypeScript checks below.
+    const item = target as SlMenuItem;
 
     if (item.type === 'checkbox') {
       item.checked = !item.checked;

src/components/menu/menu.test.ts

@@ -101,3 +101,23 @@ describe('<sl-menu>', () => {
     expect(selectHandler).to.not.have.been.called;
   });
 });
+
+// @see https://github.com/shoelace-style/shoelace/issues/1596
+it('Should fire "sl-select" when clicking an element within a menu-item', async () => {
+  // eslint-disable-next-line
+  const selectHandler = sinon.spy(() => {});
+
+  const menu: SlMenu = await fixture(html`
+    <sl-menu>
+      <sl-menu-item>
+        <span>Menu item</span>
+      </sl-menu-item>
+    </sl-menu>
+  `);
+
+  menu.addEventListener('sl-select', selectHandler);
+  const span = menu.querySelector('span')!;
+  await clickOnElement(span);
+
+  expect(selectHandler).to.have.been.calledOnce;
+});