MudMenu: Introduce OnClickHandlerAsync to fix #6645

[ScarletKuro]

Description

Resolved issue #6645 by introducing a new API, OnClickHandlerAsync, which properly awaits OnClick.InvokeAsync to show exceptions.
There are still some concerns with SelectedItem and SelectedValue as they call methods that should be awaited. We should avoid overriding the Parameters getter and setters and look for an alternative solution.
Additionally, to catch the MudMenuItem exception, it's necessary to have the MudPopoverProvider inside the ErrorBoundary since the MudPopoverProvider renders the list popover which is a significant disadvantage.

How Has This Been Tested?

new unit test + visual check on https://try.mudblazor.com/snippet/GOwnkebdRgyviedF

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• The PR is submitted to the correct branch (dev).
• My code follows the code style of this project.
• I've added relevant tests.

[codecov]

Codecov Report

Patch coverage: 68.96% and project coverage change: -0.11 ⚠️

Comparison is base (50da41d) 90.55% compared to head (85978f2) 90.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #6648      +/-   ##
==========================================
- Coverage   90.55%   90.44%   -0.11%     
==========================================
  Files         399      399              
  Lines       13610    13632      +22     
==========================================
+ Hits        12324    12329       +5     
- Misses       1286     1303      +17     

Impacted Files	Coverage Δ
src/MudBlazor/Components/List/MudListItem.razor	100.00% <ø> (ø)
src/MudBlazor/Components/Menu/MudMenu.razor.cs	83.67% <ø> (ø)
src/MudBlazor/Components/Menu/MudMenuItem.razor.cs	82.60% <ø> (ø)
src/MudBlazor/Components/List/MudListItem.razor.cs	64.93% <60.00%> (-16.74%)	⬇️
src/MudBlazor/Components/List/MudList.razor.cs	96.55% <88.88%> (+0.12%)	⬆️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[henon]

To be honest @ScarletKuro, I think we really need the global exception handler in MudBlazor I mentioned earlier. Because I think here a .AndForget() is a good solution. WPF has this also, a UnhandledDispatcherException event that you can subscribe to get all events that were handled by unawaitable async logic.

[ScarletKuro]

WPF has this also, a UnhandledDispatcherException event that you can subscribe to get all events that were handled by unawaitable async logic.

I don't agree.
Using UnhandledDispatcherException as the sole means of handling exceptions in a WPF application is not recommended. This approach only catches exceptions that were not handled on a background UI/worker thread (that returns back to UI) and is intended as a last resort to prevent the application from crashing and later investigate the problem via logs. In fact, you will not find a single built-in WPF component that relies on subscribing to UnhandledDispatcherException. Same goes to TaskScheduler.UnobservedTaskException, it should not be relied on as the primary means of handling unhandled exceptions in tasks. Nowadays, I haven't seen this practice in WPF much either. Most modern MVVM / Reactive frameworks have an async wrapper for ICommand that can be awaited and if you don't want to handle the error on UI thread there is options not to opt-in for it.
In the context of WPF, I find it peculiar why someone would choose not to handle an exception. Even if you don't want to display the error, you could still use an empty catch (Exception ex) block. But letting the exception bubble up to UnhandledDispatcherException leaves you with limited options since you are no longer on the UI thread in fact you are not in any Window / UserControl anymore.

Because I think here a .AndForget() is a good solution

Could you please provide more details on this? It seems that both the community and our team have already identified and fixed several instances where an awaited task was missing. Whether a task is long-running or not does not really matter. In any case, the menu is closed before the task is executed, so even if the task takes hours to complete, it does not impact the user experience. By properly awaiting the task, the UI thread can continue processing, which prevents it from freezing. However, if we were to use .GetAwaiter().GetResult() (which is not possible in Blazor), it would cause the UI thread to freeze until the task is completed.

[ScarletKuro]

In the aspnetcore repository, there are similar issues where Microsoft advises against using the fire-and-forget pattern. If it is used, there should be a mechanism to bubble up exceptions from such methods (e.g. .AndForget()). Unfortunately, we don't have any of these mechanisms, and it would take considerable effort to implement them properly. Therefore, I suggest that we begin by making the errors observable, as this PR does. Afterward, we can consider how to provide both options.

[henon]

Of course you would handle exceptions if you can in WPF, the handler is for observing those you forgot to handle. We may be talking about different things entirely. I was talking about this line:

_ = SetSelectedValueAsync(value, force: true); ... here the task is discarded.

[henon]

In the aspnetcore repository, there are similar issues where Microsoft advises against using the fire-and-forget pattern. If it is used, there should be a mechanism to bubble up exceptions from such methods (e.g. .AndForget()). Unfortunately, we don't have any of these mechanisms, and it would take considerable effort to implement them properly. Therefore, I suggest that we begin by making the errors observable, as this PR does. Afterward, we can consider how to provide both options.

Sure, you are of course right. I think I misread the intention of this PR.

[henon]

_ = SetSelectedValueAsync(value, force: true); ... here the task is discarded.

OK, for everyone's information we established in our discussion on the team that we should never discard tasks as this swallows exceptions silently. I am working on a PR that will introduce a global exception handler for all exceptions caught in .AndForget().

[ScarletKuro]

Updated PR

[henon]

Thanks a lot!