Menu button updates for table (#986)

# Pull Request

## 🤨 Rationale

Some changes need to be made to the menu button before it can be used
within the table for the action menu. Specifically, those changes are:
- adding a `beforetoggle` event
- having the menu button find the slotted menu when it is nested within
multiple slots

See #956 for more details.

## 👩‍💻 Implementation

- Add `beforetoggle` event to the menu button that is fired before the
menu opens or closes
- Add Angular & Blazor support for the `beforetoggle` event
- Change the logic for finding the slotted menu. We can no longer use a
filter in the template to find slotted elements with a role of `menu`
because this won't match `slot` elements. Instead, the template assigns
all slotted elements to the `slottedMenus` array, and the getter for
`menu` finds the menu, which may be nested within `slot` elements.

## 🧪 Testing

- Manually tested that the `beforetoggle` event handler works correctly
in Blazor & Angular
- Added auto tests for the `beforetoggle` event works correctly for the
web components
- Basic manual testing in Storybook that the component still behaves as
expected
- Added tests that the focus & menu opening behavior works correctly
when the menu is nested in an additional slot

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

angular-workspace/projects/ni/nimble-angular/src/directives/menu-button/nimble-menu-button.directive.ts

@@ -1,9 +1,10 @@
-import { Directive, ElementRef, EventEmitter, HostListener, Input, Output, Renderer2 } from '@angular/core';
+import { Directive, ElementRef, Input, Renderer2 } from '@angular/core';
 import type { MenuButton } from '@ni/nimble-components/dist/esm/menu-button';
-import type { ButtonAppearance } from '@ni/nimble-components/dist/esm/menu-button/types';
+import type { ButtonAppearance, MenuButtonToggleEventDetail } from '@ni/nimble-components/dist/esm/menu-button/types';
 import { BooleanValueOrAttribute, toBooleanProperty } from '../utilities/template-value-helpers';
 
 export type { MenuButton };
+export type { MenuButtonToggleEventDetail };
 
 /**
  * Directive to provide Angular integration for the menu button.
@@ -46,14 +47,5 @@ export class NimbleMenuButtonDirective {
         this.renderer.setProperty(this.elementRef.nativeElement, 'open', toBooleanProperty(value));
     }
 
-    @Output() public openChange = new EventEmitter<boolean>();
-
     public constructor(private readonly renderer: Renderer2, private readonly elementRef: ElementRef<MenuButton>) {}
-
-    @HostListener('open-change', ['$event'])
-    public onOpenChange($event: Event): void {
-        if ($event.target === this.elementRef.nativeElement) {
-            this.openChange.emit(this.open);
-        }
-    }
 }

change/@ni-nimble-angular-81d4a2bd-2ca0-47ba-a7bb-c47be917d8ae.json

@@ -0,0 +1,7 @@
+{
+  "type": "major",
+  "comment": "Add 'beforetoggle' event on menu button and rename 'open-change' event to 'toggle'",
+  "packageName": "@ni/nimble-angular",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@ni-nimble-blazor-51e63b35-2ce0-44af-9dda-744c7234e857.json

@@ -0,0 +1,7 @@
+{
+  "type": "major",
+  "comment": "Add 'beforetoggle' event on menu button and rename 'open-change' event to 'toggle'",
+  "packageName": "@ni/nimble-blazor",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@ni-nimble-components-0c3b5f38-1366-416c-8b35-20029a8d3eba.json

@@ -0,0 +1,7 @@
+{
+  "type": "major",
+  "comment": "Add 'beforetoggle' event on menu button and rename 'open-change' event to 'toggle'.\nUpdate menu button to work when the slotted menu is nested within additional slots.",
+  "packageName": "@ni/nimble-components",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

packages/nimble-blazor/NimbleBlazor/Components/NimbleMenuButton.razor

@@ -2,7 +2,8 @@
 @inherits ComponentBase
 <nimble-menu-button
     open="@BindConverter.FormatValue(Open)"
-    @onnimblemenubuttonopenchange="(__value) => UpdateOpen(__value.Open)"
+    @onnimblemenubuttontoggle="(__value) => HandleToggle(__value)"
+    @onnimblemenubuttonbeforetoggle="(__value) => HandleBeforeToggle(__value)"
     appearance="@Appearance.ToAttributeValue()"
     position="@Position.ToAttributeValue()"
     disabled="@Disabled"

packages/nimble-blazor/NimbleBlazor/Components/NimbleMenuButton.razor.cs

@@ -50,16 +50,31 @@ public partial class NimbleMenuButton : ComponentBase
     /// Gets or sets a callback that's invoked when 'open' changes
     /// </summary>
     [Parameter]
-    public EventCallback<bool?> OpenChanged { get; set; }
+    public EventCallback<MenuButtonToggleEventArgs> Toggle { get; set; }
+
+    /// <summary>
+    /// Gets or sets a callback that's invoked before the 'open' state of the menu button changes
+    /// </summary>
+    [Parameter]
+    public EventCallback<MenuButtonToggleEventArgs> BeforeToggle { get; set; }
 
     /// <summary>
     /// Called when 'open' changes on the web component.
     /// </summary>
-    /// <param name="value">New value of open</param>
-    protected async void UpdateOpen(bool? value)
+    /// <param name="eventArgs">The state of the menu button</param>
+    protected async void HandleToggle(MenuButtonToggleEventArgs eventArgs)
+    {
+        Open = eventArgs.NewState;
+        await Toggle.InvokeAsync(eventArgs);
+    }
+
+    /// <summary>
+    /// Called when the 'beforetoggle' event is fired on the web component
+    /// </summary>
+    /// <param name="eventArgs">The state of the menu button</param>
+    protected async void HandleBeforeToggle(MenuButtonToggleEventArgs eventArgs)
     {
-        Open = value;
-        await OpenChanged.InvokeAsync(value);
+        await BeforeToggle.InvokeAsync(eventArgs);
     }
 
     /// <summary>

packages/nimble-blazor/NimbleBlazor/EventHandlers.cs

@@ -13,14 +13,16 @@ public class CheckboxChangeEventArgs : EventArgs
     public bool Checked { get; set; }
 }
 
-public class MenuButtonOpenChangeEventArgs : EventArgs
+public class MenuButtonToggleEventArgs : EventArgs
 {
-    public bool Open { get; set; }
+    public bool NewState { get; set; }
+    public bool OldState { get; set; }
 }
 
 [EventHandler("onnimbletabsactiveidchange", typeof(TabsChangeEventArgs), enableStopPropagation: true, enablePreventDefault: true)]
 [EventHandler("onnimblecheckedchange", typeof(CheckboxChangeEventArgs), enableStopPropagation: true, enablePreventDefault: true)]
-[EventHandler("onnimblemenubuttonopenchange", typeof(MenuButtonOpenChangeEventArgs), enableStopPropagation: true, enablePreventDefault: true)]
+[EventHandler("onnimblemenubuttontoggle", typeof(MenuButtonToggleEventArgs), enableStopPropagation: true, enablePreventDefault: false)]
+[EventHandler("onnimblemenubuttonbeforetoggle", typeof(MenuButtonToggleEventArgs), enableStopPropagation: true, enablePreventDefault: false)]
 public static class EventHandlers
 {
 }

packages/nimble-blazor/NimbleBlazor/wwwroot/NimbleBlazor.lib.module.js

@@ -31,11 +31,22 @@ export function afterStarted(Blazor) {
         }
     });
     // Used by NimbleMenuButton.razor
-    Blazor.registerCustomEventType('nimblemenubuttonopenchange', {
-        browserEventName: 'open-change',
+    Blazor.registerCustomEventType('nimblemenubuttontoggle', {
+        browserEventName: 'toggle',
         createEventArgs: event => {
             return {
-                open: event.target.open
+                newState: event.detail.newState,
+                oldState: event.detail.oldState
+            };
+        }
+    });
+    // Used by NimbleMenuButton.razor
+    Blazor.registerCustomEventType('nimblemenubuttonbeforetoggle', {
+        browserEventName: 'beforetoggle',
+        createEventArgs: event => {
+            return {
+                newState: event.detail.newState,
+                oldState: event.detail.oldState
             };
         }
     });

packages/nimble-components/src/menu-button/index.ts

@@ -10,7 +10,7 @@ import { ButtonAppearance } from '../button/types';
 import type { ToggleButton } from '../toggle-button';
 import { styles } from './styles';
 import { template } from './template';
-import { MenuButtonPosition } from './types';
+import { MenuButtonToggleEventDetail, MenuButtonPosition } from './types';
 import type { ButtonPattern } from '../patterns/button/types';
 import type { AnchoredRegion } from '../anchored-region';
 
@@ -108,7 +108,11 @@ export class MenuButton extends FoundationElement implements ButtonPattern {
         if (!this.open) {
             // Only fire an event here if the menu is changing to being closed. Otherwise,
             // wait until the menu is actually opened before firing the event.
-            this.$emit('open-change');
+            const eventDetail: MenuButtonToggleEventDetail = {
+                oldState: true,
+                newState: false
+            };
+            this.$emit('toggle', eventDetail);
         }
     }
 
@@ -120,7 +124,11 @@ export class MenuButton extends FoundationElement implements ButtonPattern {
             this.focusMenu();
         }
 
-        this.$emit('open-change');
+        const eventDetail: MenuButtonToggleEventDetail = {
+            oldState: false,
+            newState: true
+        };
+        this.$emit('toggle', eventDetail);
     }
 
     public focusoutHandler(e: FocusEvent): boolean {
@@ -129,18 +137,21 @@ export class MenuButton extends FoundationElement implements ButtonPattern {
         }
 
         const focusTarget = e.relatedTarget as HTMLElement;
-        if (!this.contains(focusTarget)) {
-            this.open = false;
+        if (
+            !this.contains(focusTarget)
+            && !this.getMenu()?.contains(focusTarget)
+        ) {
+            this.setOpen(false);
             return false;
         }
 
         return true;
     }
 
     public toggleButtonCheckedChangeHandler(e: Event): boolean {
-        this.open = this.toggleButton!.checked;
+        this.setOpen(this.toggleButton!.checked);
         // Don't bubble the 'change' event from the toggle button because
-        // the menu button has its own 'open-change' event.
+        // the menu button has its own 'toggle' event.
         e.stopPropagation();
         return false;
     }
@@ -149,10 +160,10 @@ export class MenuButton extends FoundationElement implements ButtonPattern {
         switch (e.key) {
             case keyArrowUp:
                 this.focusLastItemWhenOpened = true;
-                this.open = true;
+                this.setOpen(true);
                 return false;
             case keyArrowDown:
-                this.open = true;
+                this.setOpen(true);
                 return false;
             default:
                 return true;
@@ -162,32 +173,77 @@ export class MenuButton extends FoundationElement implements ButtonPattern {
     public menuKeyDownHandler(e: KeyboardEvent): boolean {
         switch (e.key) {
             case keyEscape:
-                this.open = false;
+                this.setOpen(false);
                 this.toggleButton!.focus();
                 return false;
             default:
                 return true;
         }
     }
 
-    private get menu(): HTMLElement | undefined {
-        return this.slottedMenus?.length ? this.slottedMenus[0] : undefined;
+    private setOpen(newValue: boolean): void {
+        if (this.open === newValue) {
+            return;
+        }
+
+        const eventDetail: MenuButtonToggleEventDetail = {
+            oldState: this.open,
+            newState: newValue
+        };
+        this.$emit('beforetoggle', eventDetail);
+
+        this.open = newValue;
+    }
+
+    private getMenu(): HTMLElement | undefined {
+        // Get the menu that is slotted within the menu-button, taking into account
+        // that it may be nested within multiple 'slot' elements, such as when used
+        // within a table.
+        if (!this.slottedMenus?.length) {
+            return undefined;
+        }
+
+        let currentItem: HTMLElement | undefined = this.slottedMenus[0];
+        while (currentItem) {
+            if (currentItem.getAttribute('role') === 'menu') {
+                return currentItem;
+            }
+
+            if (this.isSlotElement(currentItem)) {
+                const firstNode = currentItem.assignedNodes()[0];
+                if (firstNode instanceof HTMLElement) {
+                    currentItem = firstNode;
+                } else {
+                    currentItem = undefined;
+                }
+            } else {
+                return undefined;
+            }
+        }
+
+        return undefined;
+    }
+
+    private isSlotElement(
+        element: HTMLElement | undefined
+    ): element is HTMLSlotElement {
+        return element?.nodeName === 'SLOT';
     }
 
     private focusMenu(): void {
-        this.menu?.focus();
+        this.getMenu()?.focus();
     }
 
     private focusLastMenuItem(): void {
-        const menuItems = this.menu?.querySelectorAll('[role=menuitem]');
+        const menuItems = this.getMenu()?.querySelectorAll('[role=menuitem]');
         if (menuItems?.length) {
             const lastMenuItem = menuItems[menuItems.length - 1] as HTMLElement;
             lastMenuItem.focus();
         }
     }
 
     private readonly menuChangeHandler = (): void => {
-        this.open = false;
+        this.setOpen(false);
         this.toggleButton!.focus();
     };
 }

packages/nimble-components/src/menu-button/specs/README.md

@@ -84,7 +84,12 @@ _Methods_
 
 _Events_
 
--   `open-change` (event) - event for when the opened state has changed
+-   `beforetoggle` (event) - event fired before the opened state has changed. The event detail contains:
+    -   `newState` - boolean - The value of `open` on the menu button that the element is transitioning in to.
+    -   `oldState` - boolean - The value of `open` on the menu button that the element is transitioning out of.
+-   `toggle` (event) - event for when the opened state has changed
+    -   `newState` - boolean - The value of `open` on the menu button that the element transitioned in to.
+    -   `oldState` - boolean - The value of `open` on the menu button that the element transitioned out of.
 
 _CSS Classes and CSS Custom Properties that affect the component_
 

packages/nimble-components/src/menu-button/template.ts

@@ -1,4 +1,4 @@
-import { elements, html, ref, slotted, when } from '@microsoft/fast-element';
+import { html, ref, slotted, when } from '@microsoft/fast-element';
 import { DesignSystem } from '@microsoft/fast-foundation';
 import type { MenuButton } from '.';
 import { ToggleButton } from '../toggle-button';
@@ -43,7 +43,7 @@ export const template = html<MenuButton>`
                 ${ref('region')}
             >
                 <span part="menu">
-                    <slot name="menu" ${slotted({ property: 'slottedMenus', filter: elements('[role=menu]') })}></slot>
+                    <slot name="menu" ${slotted({ property: 'slottedMenus' })}></slot>
                 </span>
             </${DesignSystem.tagFor(AnchoredRegion)}>
         `