MudOverlay: Remove OnClick callback

[danielchalmers]

Feature request type

Enhance component

Component name

MudOverlay

Is your feature request related to a problem?

MudOverlay.OnClick might be an antipattern.

It's commonly used to handle close events but you should use VisibleChanged instead as seen in #8914. This change resolved #3825 and #3824.

This will have to be fixed in several other components too where it is impacting accessibility: https://github.com/search?q=repo%3AMudBlazor%2FMudBlazor+%3CMudOverlay+OnClick&type=code

Describe the solution you'd like

The callback should have some kind of warning, or be removed altogether, so people don't rely on it for close-callbacks.

Users will still be able to subscribe to @onclick themselves.

Have you seen this feature anywhere else?

No response

Describe alternatives you've considered

No response

Pull Request

• I would like to do a Pull Request

Code of Conduct

• I agree to follow this project's Code of Conduct

[danielchalmers]

I'm planning to replace all other usages of OnClick with the VisibleChanged event like I did in #8914 in order to avoid subtle bugs. How do we stop people from falling into this same anti-pattern without removing OnClick entirely? @henon @ScarletKuro

[henon]

We just make OnClick obsolete with a message to use VisibleChanged instead. After it has been obsoleted for a time we can remove it in a future major version.

[danielchalmers]

@henon

/// <summary>
/// Triggers an event callback when the overlay is clicked.
/// </summary>
[Parameter]
[Obsolete("Use VisibleChanged with AutoClose to handle the overlay disappearing in a universal way.")]
public EventCallback<MouseEventArgs> OnClick { get; set; }

Can we make AutoClose true by default? It seems like that's the only way it's actually used in the whole library so it would make sense if we made that switch while moving away from OnClick. The pattern is always "OnClick = make visible false".

[henon]

OK, we can change AutoClose to true by default. Maybe we should think if obsoleting OnClick is even necessary. What if someone wants to use AutoClose="false" and doesn't want to close on click but still get the click event to do something, i.e. show a message or whatever? Once we remove it that wouldn't be possible any longer.

As for prevending people to use OnClick instead of AutoClose we can also just show a warning box in the docs.

[henon]

Or maybe we add an OnClosing eventcallback with class CloseEventArgs which contain a writable property bool Cancel which the user can use to prevent or allow the overlay from closing based on their business logic? In the overlay we await the OnClosing eventcallback and based on the value of Cancel update Visible accordingly?

WPF has such logic, would this be ok for Blazor as well @ScarletKuro ?

[danielchalmers]

ping @ScarletKuro

[danielchalmers]

I'm going to put this on hold due to the potential for issues to pop up and some components (MudDrawer) really need further refactoring which will take more time.

[ScarletKuro]

WPF has such logic, would this be ok for Blazor as well @ScarletKuro ?

Yeah