Menu: Improve touch support & Merge OnTouch,OnAction into OnClick

[danielchalmers]

Description

Changes for v7:

• Remove OnTouch and OnAction in favor of OnClick like all other controls
• Fix touch events falling through
• Add Activate override for TouchEventArgs
• Properly handle the context menu as a right click on touch so it doesn't trigger the default action
• Remove obsolete members

Impact:

• Closes Menu: Fix touch events falling through popover #8331
• Resolves MudMenuItem cannot be used to open a dialog via touch input  #4527
• Resolves MudMenuItem trigger elements behind itself on touch devices  #4549
• Resolves Menu: press goes through to component below on mobile #5170
• Resolves Dialog does not stay open when opened from a menuitem touch event #5768
• Resolves MudMenu - on mobile when a dialog is opened from a MudMenuItem the dialog is closed automatically. #7784

Everything should work the same or better.
If there's any regressions in behavior they'll be fixed.

Migration Guide - MudMenu

OnTouch and OnAction have been removed to simplify the API. OnClick handles all use cases for both mouse and touch.

How Has This Been Tested?

unit

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)

Touch events don't fall through.

Before:

Video3.mp4

After:

Video2.mp4

Checklist:

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

[danielchalmers]

Took care of a few issues with the menu while simplifying the handlers to match the rest of the components.

@mckaragoz @henon @ScarletKuro

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.34%. Comparing base (27e54b3) to head (e5b2f2e).
Report is 19 commits behind head on dev.

Files	Patch %	Lines
src/MudBlazor/Components/Menu/MudMenu.razor.cs	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #8492      +/-   ##
==========================================
+ Coverage   89.01%   89.34%   +0.33%     
==========================================
  Files         411      410       -1     
  Lines       12175    11892     -283     
  Branches     2431     2355      -76     
==========================================
- Hits        10837    10625     -212     
+ Misses        803      748      -55     
+ Partials      535      519      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ScarletKuro]

Will this problem not come back since we removed semaphore? #7651
If I remember correctly the problem only existed on specific platforms.

[ScarletKuro]

I believe we missing spaces before and after ==

[danielchalmers]

That's a straight copy paste. I didn't want to do clean up and pollute the review

[danielchalmers]

If you tell me some components I could do them in bulk separately?

[danielchalmers]

Will this problem not come back since we removed semaphore? #7651 If I remember correctly the problem only existed on specific platforms.

Isn't that only relevant due to the OnClick/OnTouch split?

[ScarletKuro]

Isn't that only relevant due to the OnClick/OnTouch split?

I'm actually not sure, as I understand the problem was when you click multiple times?

Maybe @dennisrahmen knows wmore

[dennisrahmen]

@ScarletKuro the Problem was with OnAction.

If I remember correctly OnAction should be called if it is a Touch or a Click.

So that you don't have to add both OnTouch and OnClick when you want it to work on Desktop and Mobile.

But that at least for me never worked on Blazor WASM. I am still adding both to my menus.

[danielchalmers]

as I understand the problem was when you click multiple times?

Like this?

video2.mp4

[danielchalmers]

@ScarletKuro the Problem was with OnAction.

If I remember correctly OnAction should be called if it is a Touch or a Click.

So that you don't have to add both OnTouch and OnClick when you want it to work on Desktop and Mobile.

But that at least for me never worked on Blazor WASM. I am still adding both to my menus.

@dennisrahmen I think we can make OnClick functionally the same as OnAction and thus remove OnTouch and OnAction

Maybe you could try this PR out if you get some free time because it seems to work well for me

[ScarletKuro]

I found the original discussion #6954
I guess the problem was that on android when using OnClick and OnTouch together it would fire twice. I guess it should be fine now.
If this OnClick now works on Windows, Android and iOS in MAUI for MudMenuItem, we should be fine.
Sorry, just trying to be really cautious with this,

[danielchalmers]

Sorry, just trying to be really cautious with this,

Oh I think that's exactly the right approach

I couldn't personally find any problems but always happy to test new scenarios

[dennisrahmen]

@danielchalmers I will try it out this evening when I get home in my WASM App.

I can also try in MAUI for Android.

[mikes-gh]

@danielchalmers If this all works would you mind looking at the coverage. We probably need an additional couple of tests in this area

[danielchalmers]

@danielchalmers I will try it out this evening when I get home in my WASM App.

I can also try in MAUI for Android.

@dennisrahmen Thank you, that would be amazing

@danielchalmers If this all works would you mind looking at the coverage. We probably need an additional couple of tests in this area

@mikes-gh Sure, what kind of tests were you thinking?

[mikes-gh]

@mikes-gh Sure, what kind of tests were you thinking?

Use codecov to view the untested lines. Some of them may not be your changes but it would be good to get the patch coverage up

[danielchalmers]

should be looking good @mikes-gh

[mikes-gh]

should be looking good @mikes-gh

Thanks @danielchalmers I know it seems pedantic but there's a reason BL0005 exists. Component parameters should pass through all the normal Component lifecycles for a true test.

[danielchalmers]

@mikes-gh I don't think it's pedantic. I was just lazy and copy pasted from another test. Thanks for pointing it out 👌

[mikes-gh]

It seems we still have a coverage deficiency though in line 293 ToggleMenu?

[danielchalmers]

It seems we still have a coverage deficiency though in line 293 ToggleMenu?

Was the other method even covered? It's just a new overload and is covered by MenuTests.ToggleEventArgs() kind of.

How do you test IActivator? I couldn't see it in another tests but I probably just missed it.

[ScarletKuro]

How do you test IActivator? I couldn't see it in another tests but I probably just missed it.

Depends on what you want to do.
For example if you want to override the IActivatable then its how it can be done https://try.mudblazor.com/snippet/wYQSanFmRxwqjzLD

[danielchalmers]

@ScarletKuro just to satisfy code coverage

[mikes-gh]

We should probably wait for @dennisrahmen #8492 (comment) before merging.

[dennisrahmen]

@mikes-gh @danielchalmers for reference this was the problem I had when using 'OnAction', the first two menu items have 'OnClick' and 'OnTouch' the last one has just 'OnAction'.

current.mp4

I wanted to try it in the app you see above as it also has a MAUI version, but with the other breaking changes I would need more time to get it running.
In the meantime, if you think that is sufficient, I created a new Blazor WASM app and tested it there:

pullrequest.mp4

Now oddly enough, using the current MudBlazor Nuget on the app I created for the pull request the menu item does not work at all weather in desktop or mobile.

[dennisrahmen]

I could get my app updated by mid next week and test it again, If you guys are willing to wait.
Not sure about you timeline for v7.

[danielchalmers]

@dennisrahmen Thanks, this is all really helpful. Can you post the code for your second example?

[dennisrahmen]

@danielchalmers you can even see it in a tryMud:
Just switch from desktop to mobile in dev tools.
https://try.mudblazor.com/snippet/QEwxvlGVwWrSSpbr

That's the code I used, just removed the touch and action options since your version does not have them anymore.

[danielchalmers]

I could get my app updated by mid next week and test it again, If you guys are willing to wait. Not sure about you timeline for v7.

It could get merged before then to avoid conflicts, but development is so heavy right now that having you test it afterwards is still extremely helpful because there won't be a new release by then anyway

[danielchalmers]

@danielchalmers you can even see it in a tryMud: Just switch from desktop to mobile in dev tools. https://try.mudblazor.com/snippet/QEwxvlGVwWrSSpbr

That's the code I used, just removed the touch and action options since your version does not have them anymore.

Thanks, I believe that's #5768 and should be fixed by this PR 🤞

[henon]

What's the status?

[dennisrahmen]

@henon the issue I had with the current implementation should be fixed by this pr since another user reported a similar issue and that was already addressed by @danielchalmers

So as I understand @danielchalmers this pr is ready but I will still report my findings after I updated my app to the newest version in the v7.

[henon]

Thanks!

[dennisrahmen]

@danielchalmers I finally updated my app to the preview-3 of v7 and could now test the behavior.
Sadly, still does not work for mobile, although I am not really sure what the problem is.

Would think it has something to do with the recognition of the mobile touch in general, as you see in the video in mobile view the sub menu does not even open, I just go straight to the links in the submenus.

MS Edge on the dev site and my app has the same issue as described before, the OnClick is not fired:

Menus.-.MudBlazor.-.Personlich.Microsoft.Edge.2024-05-15.02-14-03.mp4

[dennisrahmen]

Here is my app, working in desktop view but not doing anything when in mobile:

iTS.Dashboard.und.3.weitere.Seiten.-.Personlich.Microsoft.Edge.2024-05-15.02-26-54.mp4

This is the menu code in my DataGrid:

<TemplateColumn Filterable="false" Sortable="false">
    <CellTemplate>
        <MudMenu Dense="true" Icon="@Icons.Material.Filled.MoreVert" Size="Size.Small" Color="Color.Primary">
            <MudMenuItem IconSize="Size.Small" Disabled="@(context.Item.DeviceType is null)" Icon="@Icons.Material.Filled.AppSettingsAlt" OnClick="() => UpdateDeviceConfig(context.Item)">Konfiguration</MudMenuItem>
            <MudMenuItem IconSize="Size.Small" Disabled="@(context.Item.DeviceType is null)" IconColor="Color.Warning" Icon="@Icons.Material.Filled.Edit" OnClick="() => UpdateDevice(context.Item)">Bearbeiten</MudMenuItem>
            <MudDivider Light DividerType="DividerType.Middle" Class="my-1"/>
            <MudMenuItem IconSize="Size.Small" IconColor="Color.Error" Icon="@Icons.Material.Filled.Delete" OnClick="() => RemoveDevice(context.Item)">Löschen</MudMenuItem>
        </MudMenu>
    </CellTemplate>
</TemplateColumn>

[danielchalmers]

@dennisrahmen I believe that's a missing PreventDefault or StopPropagation in the MudListItems that make up the MudMenu content. Could you transfer what you posed to a new issue so I can reference it? Basically "touches pass through menu item" I guess

[AntMaster7]

Thank you for fixing this. I was also affected by this issue.