-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Context menu support for Windows and MacCatalyst #9174
Conversation
I just started to experiment with the implementation of context menu for MAUI a few hours back. However, I started down a different path by using an attached property. I also considered a behaviour. Obviously, you decided to augment IView directly. Just wanted to point out alternatives. Also here are links to docs on context menu for ios/macos and Android (which also supports this via 'PopupMenu' class): https://developer.apple.com/documentation/uikit/uicontextmenuinteraction |
@profix898 thanks for sharing. I added directly to View because it seemed "most natural" to me, and roughly aligns with what WinUI and MacCatalyst do in their respective systems (which this MAUI implementation ultimately wraps). Do you see any pros/cons either way? |
1e80525
to
ac872cc
Compare
0d5bfc0
to
c11dd36
Compare
To @PureWeen and any other reviewers:
I'd love to get this PR merged ASAP so that it can make its way to customers, while at the same time I can iterate on the missing features to continuously improve it. So, if you have concerns of what's in here, please do let me know, including whether the known limitations are acceptable at this time. Thanks! |
19e5784
to
6f54023
Compare
protected override UIMenuElement CreatePlatformElement() | ||
{ | ||
|
||
// REVIEW: I don't like this code, but couldn't come up with anything better. It feels right to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was just an oversight and I was unaware that UIAction
can just be used in the place of UICommand
@@ -53,6 +53,9 @@ public void Insert(int index, IMenuElement view) | |||
|
|||
void Rebuild() | |||
{ | |||
// REVIEW: For context flyout support this likely also needs some logic like in MenuFlyoutItemHandler.iOS.cs where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Label Text="{Binding CounterValue}" FontSize="30" VerticalOptions="Center"></Label> | ||
</HorizontalStackLayout> | ||
|
||
<Label Text="Right-click to see beautiful menus" FontSize="30"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -109,6 +109,10 @@ public static IMauiHandlersCollection AddMauiControlsHandlers(this IMauiHandlers | |||
handlersCollection.AddHandler(typeof(Frame), typeof(Handlers.Compatibility.FrameRenderer)); | |||
#endif | |||
|
|||
#if WINDOWS || MACCATALYST | |||
handlersCollection.AddHandler(typeof(ContextFlyout), typeof(ContextFlyoutHandler)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code appears to work fine on iOS13+
. Not sure if there was a specific reason why we aren't letting this code just run on iOS13. I created a tracking issue here that we can address after we're API completely for NET7 since it shouldn't take any API changes to enable the handlers for iOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nitpick.
MenuTestBase<MenuFlyout, IMenuElement, MenuFlyoutItem, ContextFlyoutItemHandlerUpdate> | ||
{ | ||
[Fact] | ||
public void BindingContextPropagatesWhenContextFlyoutIsAlreadySetOnParent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Add context menu (right-click menu) support for Windows and MacCatalyst. Fixes #7777
7f8fcba
to
b872532
Compare
- wire up context menu for MKWebView
# Conflicts: # src/Controls/src/Core/HandlerImpl/Element/Element.Impl.cs # src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt # src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt # src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt # src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt # src/Core/src/Handlers/View/ViewHandler.cs # src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/net/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/netstandard/PublicAPI.Unshipped.txt # src/Core/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
okAction: x => completionHandler(x.TextFields[0].Text), | ||
cancelAction: _ => completionHandler(null) | ||
okAction: x => completionHandler(x.TextFields[0].Text!), | ||
cancelAction: _ => completionHandler(null!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???
Maybe the delegate should be a nullable string?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK because it's an override I can't change the delegate. The iOS SDK would have to change it or we'd do it by converting this whole thing to implement the interface ourselves
If I set it to Action<string?>
then I get a compile error Nullability of reference types in type doesn't match overidden member
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh dear :( This sounds like a wrong API. Let me open an issue for that.
Description of Change
Open Questions
Should ContextFlyout be an attached property? On WinUI the ContextFlyout is a property onMadeUIElement
which is the base class to everything so that would be the equivalent of us putting it onElement
vsView
. They also haveFlyoutBase
which is the left click version of a flyout which is an attached property?ContextFlyout
an attached property onFlyoutBase
IContextFlyoutContainer
vsIContextFlyoutElement
Context menu support for Windows and MacCatalyst #9174 (comment)IContextFlyoutElement
winsshould we renameI went with this approach becauseContextFlyout
toMenuFlyout
and thenContextFlyout
is just the container for the attached property?MenuFlyout
is just a type ofFlyout
and we are most likely going to want to expand on the types that we can specify here. So now View just takesFlyoutBase
andMenuFlyout
inherits fromFlyoutBase
. At a later point we'll probably introduceFlyoutView
API CHANGES
MAUI.CORE
Maui.Controls
XAML Sample
Known Issues
Please refer to this comment to see what is and isn't supported.
Issues Fixed
Fixes #7777