Skip to content

Comments

MudMenu: Replace IActivatable with MenuContext#12145

Merged
danielchalmers merged 17 commits intodevfrom
copilot/refactor-mudmenu-with-menucontext
Dec 19, 2025
Merged

MudMenu: Replace IActivatable with MenuContext#12145
danielchalmers merged 17 commits intodevfrom
copilot/refactor-mudmenu-with-menucontext

Conversation

Copy link
Contributor

Copilot AI commented Nov 23, 2025

Replaces the generic IActivatable interface with a dedicated async MenuContext class for MudMenu activation, providing a clean, modern, component-agnostic API.

Changes:

  • New MenuContext class - Clean async API with OpenAsync, CloseAsync, ToggleAsync, and CloseAllAsync methods. Fully XML-documented with proper null validation.
  • ActivatorContent signature changed - From RenderFragment? to RenderFragment<MenuContext>? allowing context to be passed directly to custom activators
  • MudMenu refactored - Removed IActivatable interface and CascadingValue pattern. Context passed directly to RenderFragment and cached in constructor for performance.
  • IActivatable preserved for MudFileUpload - Kept in MudBaseButton for backward compatibility with file upload component
  • All docs and tests updated - Updated all component examples and test components to use explicit context.ToggleAsync() calls
  • Fixed event bubbling conflicts - Removed implicit click handlers from wrapper divs and disabled root div's contextmenu handler when ActivatorContent is present, preventing double-toggle issues with both left-click and right-click events

New MenuContext API:

public sealed class MenuContext
{
    public Task OpenAsync(EventArgs? args = null);
    public Task CloseAsync();
    public Task ToggleAsync(EventArgs? args = null);
    public Task CloseAllAsync();
}

New usage pattern:

<MudMenu>
    <ActivatorContent>
        <MudButton OnClick="@context.ToggleAsync">Open Menu</MudButton>
    </ActivatorContent>
    <ChildContent>
        <MudMenuItem>Item 1</MudMenuItem>
    </ChildContent>
</MudMenu>

<!-- Right-click example -->
<MudMenu ActivationEvent="@MouseEvent.RightClick">
    <ActivatorContent>
        <div @oncontextmenu="@context.ToggleAsync" @oncontextmenu:preventDefault>
            <MudChip>Right Click</MudChip>
        </div>
    </ActivatorContent>
    <ChildContent>
        <MudMenuItem>Item 1</MudMenuItem>
    </ChildContent>
</MudMenu>

Key improvements:

  • No async void or .CatchAndLog()
  • All methods return Task with proper async patterns
  • Internal constructor with null validation
  • Supports position-at-cursor with MouseEventArgs/TouchEventArgs
  • Shorter method names (ToggleAsync instead of ToggleMenuAsync)
  • MenuContext instance cached for better performance
  • Explicit context usage required - all event handlers must be wired in ActivatorContent
  • Root div event handlers only fire for default activators (Button, Icon), not custom ActivatorContent

Breaking change: ActivatorContent now requires explicit context usage (e.g., context.ToggleAsync()). Users must explicitly wire up click/contextmenu handlers in their ActivatorContent.

Checklist:

  • I've read the contribution guidelines
  • My code follows the style of this project
  • I've added or updated relevant unit tests
Original prompt

This section details on the original issue you should resolve

<issue_title>MudMenu: Replace IActivatable with MenuContext</issue_title>
<issue_description>This issue tracks the work proposed in PR #10480 (MudMenu: Remove IActivatable) and requests further details and implementation steps concerning MenuContext integration.

Background:
The pull request by @ScarletKuro removes the IActivatable interface from MudMenu. For effective tracking and planning, this issue will expand upon the changes and discuss implementation tasks related to MenuContext

Implementation Details:

  • Refactor MudMenu to remove all usages and dependencies on IActivatable.
  • Introduce or leverage MenuContext to manage activation and state for menu components, centralizing associated logic.
  • Ensure the new context-based approach supports all scenarios previously covered by IActivatable.
  • Update documentation and related component examples as necessary.

Please use this issue for implementation discussion, status tracking, and to ensure feature parity after the refactor.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits November 23, 2025 18:34
- Created MenuContext class to encapsulate menu activation logic
- Updated MudMenu to use MenuContext instead of IActivatable
- Added MenuContext as cascading parameter in MudBaseButton
- Updated test to use MenuContext
- All 3936 tests passing (61 menu tests, 0 failures)

Co-authored-by: danielchalmers <7112040+danielchalmers@users.noreply.github.com>
Co-authored-by: danielchalmers <7112040+danielchalmers@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor MudMenu to replace IActivatable with MenuContext MudMenu: Replace IActivatable with MenuContext Nov 23, 2025
Copilot AI requested a review from danielchalmers November 23, 2025 18:45
@danielchalmers danielchalmers requested review from ScarletKuro and versile2 and removed request for danielchalmers November 23, 2025 18:46
@danielchalmers danielchalmers removed their assignment Nov 23, 2025
@danielchalmers
Copy link
Member

Donating this. I don't know enough about it to do any more work

@mudbot mudbot bot added the refactor Reorganizes code and has no changes to the API or functionality in the main library label Nov 23, 2025
@danielchalmers danielchalmers added breaking change This change will require consumer code updates and removed refactor Reorganizes code and has no changes to the API or functionality in the main library labels Nov 23, 2025
@ScarletKuro
Copy link
Member

ScarletKuro commented Nov 25, 2025

@copilot

Never use async void. Never use .CatchAndLog(). Never create fire-and-forget flows.
All new APIs MUST return Task and MUST follow the Async-suffix convention if they use Task API.
Do NOT copy/paste old code or reuse the obsolete IActivatable pattern.

Goal

Replace the old non-async, tightly coupled, component-specific, cascading IActivatable API with a clean, modern, async, component-agnostic MenuContext API.

This new API must:

Provide an elegant MenuContext class

  • Must be XML-documented.
  • Must wrap all menu operations (Open, Close, Toggle, CloseAll) etc.
  • Must work correctly when PositionAtCursor is enabled (e.g., using MouseEventArgs, TouchEventArgs, etc.).
  • Must expose fully async Task-returning methods if neccessary.
  • Must NOT leak internal MudMenu APIs.

Replace the activator pattern with a strongly typed context API:

Replace:

public RenderFragment? ActivatorContent { get; set; }

with:

public RenderFragment<MenuContext>? ActivatorContent { get; set; }

Replace the old activator render code:

<CascadingValue Value="@((IActivatable)this)" IsFixed="true">
    <div @onclick="ToggleMenuAsync" disabled="@Disabled" class="@ActivatorClassname">
        @ActivatorContent
    </div>
</CascadingValue>

with:

<div disabled="@Disabled" class="@ActivatorClassname">
    @ActivatorContent(new MenuContext(this))
</div>

Enable all components, HTML, custom components, anything — to trigger menu actions:

Activators must now be written like this (just one of the example):

<MudMenu FullWidth="true" PopoverClass="menu-popover-class">
    <ActivatorContent>
        <MudButton Variant="Variant.Filled"
                   OnClick="@(() => context.OpenMenuAsync())"
                   Color="Color.Primary">
            Activator Content Standard
        </MudButton>
    </ActivatorContent>

    <ChildContent>
        <MudMenuItem Class="test-class">1</MudMenuItem>
        <MudMenuItem Href="https://www.test.com">2</MudMenuItem>
        <MudMenuItem Href="https://www.test.com" Target="_blank">3</MudMenuItem>
    </ChildContent>
</MudMenu>

Where the consumer always interacts with the menu using the passed context.

Remove the obsolete API entirely:

  • Delete IActivatable.
  • Remove the CascadingParameter from MudBaseButton.
  • Remove Activate(object activator, MouseEventArgs args).
  • Remove any code relying on non-async activations.

Update all documentation & tests:

  • Update all component examples in docs to use context.
  • Update every test that used the old activator flow.
  • Rewrite tests where needed so they operate through MenuContext.
  • Ensure async test flows use await instead of fire-and-forget.

Required MenuContext API — Guidelines for Copilot:

Copilot must NOT reuse old code.
Copilot must generate a clean API similar to the following style (NOT copy-paste):

/// <summary>
/// Exposes async operations for managing a <see cref="MudMenu"/> instance,
/// including opening, closing, toggling, and closing parent menus.
/// </summary>
public sealed class MenuContext
{
    private readonly MudMenu _menu;

    /// <summary>
    /// Creates a new <see cref="MenuContext"/> for the specified <see cref="MudMenu"/>.
    /// </summary>
    public MenuContext(MudMenu menu)
    {
        _menu = menu;
    }

    /// <summary>
    /// Opens the menu. If <see cref="MudMenu.PositionAtCursor"/> is enabled,
    /// the menu is positioned using the supplied event arguments.
    /// </summary>
    public Task OpenMenuAsync(EventArgs? args = null)
        => _menu.OpenMenuAsync(args);

    /// <summary>Closes the menu.</summary>
    public Task CloseMenuAsync()
        => _menu.CloseMenuAsync();

    /// <summary>Toggles the menu's visibility.</summary>
    public Task ToggleMenuAsync(EventArgs? args = null)
        => _menu.ToggleMenuAsync(args);

    /// <summary>Closes all parent menus in the hierarchy.</summary>
    public Task CloseAllMenusAsync()
        => _menu.CloseAllMenusAsync();
}

Copilot may improve naming, add more XML doc details, or expand operations if needed.

Copilot MUST update all relevant tests and docs.

Things Copilot Must NOT Do:

  • No async void
  • No .CatchAndLog()
  • No copying existing outdated code
  • No reintroducing the IActivatable pattern
  • No hidden synchronous wrappers
  • No fallback to object activator parameters
  • No CascadingParameters for activators

@mudbot mudbot bot added the needs: changes A maintainer has asked for further modifications to be made to this pull request label Nov 25, 2025
Copilot AI and others added 2 commits November 25, 2025 17:57
- Create new MenuContext with async methods (OpenMenuAsync, CloseMenuAsync, ToggleMenuAsync, CloseAllMenusAsync)
- Change ActivatorContent from RenderFragment? to RenderFragment<MenuContext>?
- Remove CascadingValue pattern - context is passed directly to RenderFragment
- Remove IActivatable dependency from MudMenu (kept for MudFileUpload)
- Revert MudBaseButton changes - IActivatable remains for FileUpload
- Update all doc examples and test components to use context.ToggleMenuAsync
- Update tests to work with new context-based pattern

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
- Restore @OnClick handler on activator div for backward compatibility
- Add ArgumentNullException.ThrowIfNull validation in MenuContext constructor

Co-authored-by: ScarletKuro <19953225+ScarletKuro@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 25, 2025

@copilot

Never use async void. Never use .CatchAndLog(). Never create fire-and-forget flows.
...

Implemented the clean async MenuContext API as requested in commit b0c407b:

Changes made:

  • Created new MenuContext with async methods (OpenMenuAsync, CloseMenuAsync, ToggleMenuAsync, CloseAllMenusAsync)
  • Changed ActivatorContent from RenderFragment? to RenderFragment<MenuContext>?
  • Removed CascadingValue pattern - context passed directly to RenderFragment
  • Removed IActivatable from MudMenu (kept for MudFileUpload per your clarification)
  • Kept IActivatable CascadingParameter in MudBaseButton for FileUpload compatibility
  • Updated all doc examples and test components to use context.ToggleMenuAsync()
  • All 3926 tests pass

No async void, no .CatchAndLog(), all methods return Task with proper XML documentation.

@ScarletKuro
Copy link
Member

Well that looks much better than the previous version... At least it looks more realistic and workable

@mudbot mudbot bot added refactor Reorganizes code and has no changes to the API or functionality in the main library and removed needs: changes A maintainer has asked for further modifications to be made to this pull request labels Nov 25, 2025
@meenzen
Copy link
Contributor

meenzen commented Nov 25, 2025

Doesn't look too bad, but api wise personally I prefer shorter names like OpenAsync instead of OpenMenuAsync, especially considering it's already part of the type MenuContext. Having Menu in the name feels redundant to me.

@ScarletKuro
Copy link
Member

Doesn't look too bad, but api wise personally I prefer shorter names like OpenAsync instead of OpenMenuAsync, especially considering it's already part of the type MenuContext. Having Menu in the name feels redundant to me.

Fair

@ScarletKuro
Copy link
Member

ScarletKuro commented Dec 11, 2025

Also tag me when it's ready for review if you want @ScarletKuro

It's ready. If someone wishing to do a MudTarget etc that should be a separate PR.

adding a shared class name to the divs might be more intuitive and somewhat self-documenting (ex: <div class="mud-target-boundary")

Don't think I understood.

@danielchalmers
Copy link
Member

adding a shared class name to the divs might be more intuitive and somewhat self-documenting (ex: <div class="mud-target-boundary")

Don't think I understood.

-<div @onclick="@context.ToggleAsync" style="cursor: pointer">
+<div @onclick="@context.ToggleAsync" style="cursor: pointer" class="mud-target-boundary">

just for the sake of identifying it

@mudbot mudbot bot removed the needs: changes A maintainer has asked for further modifications to be made to this pull request label Dec 16, 2025
@ScarletKuro
Copy link
Member

ScarletKuro commented Dec 19, 2025

adding a shared class name to the divs might be more intuitive and somewhat self-documenting (ex: <div class="mud-target-boundary")

Don't think I understood.

-<div @onclick="@context.ToggleAsync" style="cursor: pointer">
+<div @onclick="@context.ToggleAsync" style="cursor: pointer" class="mud-target-boundary">

just for the sake of identifying it

I mean, I can ask copilot to add it, but I slightly don't see the point if this is basically examples & user area, why we need to "identify" them?
I'd understand if you mean something like this?

<div class="mud-target-boundary">
   @ActivatorContent(_menuContext)
</div>

@versile2
Copy link
Contributor

adding a shared class name to the divs might be more intuitive and somewhat self-documenting (ex: <div class="mud-target-boundary")

Don't think I understood.

-<div @onclick="@context.ToggleAsync" style="cursor: pointer">
+<div @onclick="@context.ToggleAsync" style="cursor: pointer" class="mud-target-boundary">

just for the sake of identifying it

I mean, I can ask copilot to add it, but I slightly don't see the point if this is basically examples & user area, why we need to "identify" them? I'd understand if you mean something like this?

<div class="mud-target-boundary">
   @ActivatorContent(_menuContext)
</div>

We've used that pattern before but I do not think it applies here, and applying MudTarget or anything similar would just be another IActivatable type reference though more abstract. I think it's good like it is and we can add or change it down the line if we feel the need. Especially once IActivatable is gone gone.

@ScarletKuro
Copy link
Member

Ok, I will @danielchalmers let hit the merge button, if he thinks something is missing/needs to be updated, feel free to directly change it in the PR or poke copilot

@danielchalmers danielchalmers merged commit c964ae1 into dev Dec 19, 2025
8 of 10 checks passed
@ScarletKuro ScarletKuro deleted the copilot/refactor-mudmenu-with-menucontext branch December 19, 2025 20:16
@ScarletKuro ScarletKuro mentioned this pull request Feb 11, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change This change will require consumer code updates refactor Reorganizes code and has no changes to the API or functionality in the main library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudMenu: Replace IActivatable with MenuContext

5 participants