macOS: Don't clam to support action messages from non-Qt menu items

As described in 3bedeb837ef68e0062668406e7662ed9ffc5268a, the way
menu items on macOS are typically set up they have an action, e.g.
copy:, but no target, and the system then takes care of finding the
right target at runtime, starting with the first responder, walking
the responder chain, and then moving on to other NSWindows, before
ending up in the NSApplication and its delegate.

As we (still) don't have a mechanism in Qt to forward generic
actions, such as the cut/copy/paste, or selectAll, so we rely on
mapping the actions back to QCocoaNSMenuItem that we can trace
back to a QPlatformMenuItem that we in turn emit activated() for.

Normally this works fine, but in the case where the Qt app is embedded
in a native UI, which has its own menu items with cut/copy/paste,
we'll get callbacks into QNSView for actions triggered by a generic
NSMenuItem.

In that case, we need to bail out, but we want to do so in a way
that lets AppKit continue to walk the responder chain. This is
possible by implementing supplementalTargetForAction:sender:,
where we have access to the sender. If sender doesn't match
the expected QCocoaNSMenuItem we let AppKit find a better match
up the chain.

Since the target we return needs to ultimately respond to the
selectors and/or forward them, we can't point the target back
to ourselves, nor can we point it to the application delegate
directly, as the menu items need to be validated in the context
of the view, so a new per-view QNSViewMenuHelper class has been
added to take the role of forwarding the menu actions.

The logic for forwarding the resulting actions to the application
delegate has been simplified and hardened a bit as well.

A possible scenario with this new approach is that the Qt app
has a line edit focused, and the user tries to activate the
menu item for Paste, but the item is grayed out because we
can not support the action. This is of course confusing for
the user, but less so than having an active menu item that
then doesn't do anything when activated.

Another scenario is that a responder later in the chain does
respond to the paste action, and the menu item will end up
pasting into something that is not the first responder.
This might also be confusing for the user, but it's generally
recommended that implementers of actions like paste only
allow the action if the view is the first responder, and
this is something views have to deal with anyways, so it
doesn't change anything that we're now bailing out earlier
in not accepting the paste.

One benefit of allowing AppKit to find a better target for the
action is that if no target is found, and the user presses the
key equivalent of the disabled menu item, the key event will
be delivered as a normal keyDown to our QNSView, which we do
forward, allowing the Qt app to respond to the action, even
though the action came from a generic menu item. With our old
approach this would not happen, as we would claim to support
the action for our QNSView, but then drop it on the floor when
AppKit tried to deliver it to us.

Change-Id: I609db42df6a107a49e287f435e8808812c83d43e
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

1 files changed, 67 insertions, 47 deletions

diff --git a/src/plugins/platforms/cocoa/qnsview_menus.mm b/src/plugins/platforms/cocoa/qnsview_menus.mm
index 644f2139de..2840936975 100644
--- a/src/plugins/platforms/cocoa/qnsview_menus.mm
+++ b/src/plugins/platforms/cocoa/qnsview_menus.mm
@@ -9,23 +9,55 @@
 #include "qcocoamenu.h"
 #include "qcocoamenubar.h"
 
-static bool selectorIsCutCopyPaste(SEL selector)
+@implementation QNSView (Menus)
+
+// Qt does not (yet) have a mechanism for propagating generic actions,
+// so we can only support actions that originate from a QCocoaNSMenuItem,
+// where we can forward the action by emitting QPlatformMenuItem::activated().
+// But waiting for forwardInvocation to check that the sender is a
+// QCocoaNSMenuItem is too late, as AppKit has at that point chosen
+// our view as the target for the action, and if we can't handle it
+// the action will not propagate up the responder chain as it should.
+// Instead, we hook in early in the process of determining the target
+// via the supplementalTargetForAction API, and if we can support the
+// action we forward it to a helper. The helper must be tied to the
+// view, as the menu validation logic depends on the view's state.
+
+- (id)supplementalTargetForAction:(SEL)action sender:(id)sender
 {
-    return (selector == @selector(cut:)
-            || selector == @selector(copy:)
-            || selector == @selector(paste:)
-            || selector == @selector(selectAll:));
+    qCDebug(lcQpaMenus) << "Resolving action target for" << action << "from" << sender << "via" << self;
+
+    if (qt_objc_cast<QCocoaNSMenuItem *>(sender)) {
+        // The supplemental target must support the selector, but we
+        // determine so dynamically, so check here before continuing.
+        if ([self.menuHelper respondsToSelector:action])
+            return self.menuHelper;
+    } else {
+        qCDebug(lcQpaMenus) << "Ignoring action for menu item we didn't create";
+    }
+
+    return [super supplementalTargetForAction:action sender:sender];
 }
 
-@interface QNSView (Menus)
-- (void)qt_itemFired:(QCocoaNSMenuItem *)item;
 @end
 
-@implementation QNSView (Menus)
+@interface QNSViewMenuHelper ()
+@property (assign) QNSView* view;
+@end
+
+@implementation QNSViewMenuHelper
+
+- (instancetype)initWithView:(QNSView *)theView
+{
+    if ((self = [super init]))
+        self.view = theView;
+
+    return self;
+}
 
 - (BOOL)validateMenuItem:(NSMenuItem*)item
 {
-    qCDebug(lcQpaMenus) << "Validating" << item << "for" << self;
+    qCDebug(lcQpaMenus) << "Validating" << item << "for" << self.view;
 
     auto *nativeItem = qt_objc_cast<QCocoaNSMenuItem *>(item);
     if (!nativeItem)
@@ -53,7 +85,7 @@ static bool selectorIsCutCopyPaste(SEL selector)
         }
 
         if ((!menuWindow || menuWindow->window() != QGuiApplication::modalWindow())
-            && (!menubar || menubar->cocoaWindow() != self.platformWindow))
+            && (!menubar || menubar->cocoaWindow() != self.view.platformWindow))
             return NO;
     }
 
@@ -62,54 +94,42 @@ static bool selectorIsCutCopyPaste(SEL selector)
 
 - (BOOL)respondsToSelector:(SEL)selector
 {
-    // Not exactly true. Both copy: and selectAll: can work on non key views.
-    if (selectorIsCutCopyPaste(selector))
-        return ([NSApp keyWindow] == self.window) && (self.window.firstResponder == self);
+    // See QCocoaMenuItem::resolveTargetAction()
+
+    if (selector == @selector(cut:)
+     || selector == @selector(copy:)
+     || selector == @selector(paste:)
+     || selector == @selector(selectAll:)) {
+        // Not exactly true. Both copy: and selectAll: can work on non key views.
+        return NSApp.keyWindow == self.view.window
+            && self.view.window.firstResponder == self.view;
+    }
 
-    return [super respondsToSelector:selector];
-}
+    if (selector == @selector(qt_itemFired:))
+        return YES;
 
-- (void)qt_itemFired:(QCocoaNSMenuItem *)item
-{
-    auto *appDelegate = [QCocoaApplicationDelegate sharedDelegate];
-    [appDelegate qt_itemFired:item];
+    return [super respondsToSelector:selector];
 }
 
 - (NSMethodSignature *)methodSignatureForSelector:(SEL)selector
 {
-    if (selectorIsCutCopyPaste(selector)) {
-        NSMethodSignature *itemFiredSign = [super methodSignatureForSelector:@selector(qt_itemFired:)];
-        return itemFiredSign;
-    }
+    // Double check, in case something has cached that we respond
+    // to the selector, but the result has changed since then.
+    if (![self respondsToSelector:selector])
+        return nil;
 
-    return [super methodSignatureForSelector:selector];
+    auto *appDelegate = [QCocoaApplicationDelegate sharedDelegate];
+    return [appDelegate methodSignatureForSelector:@selector(qt_itemFired:)];
 }
 
 - (void)forwardInvocation:(NSInvocation *)invocation
 {
-    if (selectorIsCutCopyPaste(invocation.selector)) {
-        NSObject *sender;
-        [invocation getArgument:&sender atIndex:2];
-        qCDebug(lcQpaMenus) << "Forwarding" << invocation.selector << "from" << sender;
-
-        // We claim to respond to standard edit actions such as cut/copy/paste,
-        // but these might not be exclusively coming from menu items that we
-        // control. For example, when embedded into a native UI (as a plugin),
-        // the menu items might be part of the host application, and if we're
-        // the first responder, we'll be the target of these actions. As we
-        // don't have a mechanism in Qt to trigger generic actions, we have
-        // to bail out if we don't have a QCocoaNSMenuItem we can activate().
-        // Note that we skip the call to super as well, as that would just
-        // try to invoke the current action on ourselves again.
-        if (auto *nativeItem = qt_objc_cast<QCocoaNSMenuItem *>(sender))
-            [self qt_itemFired:nativeItem];
-        else
-            qCDebug(lcQpaMenus) << "Ignoring action for menu item we didn't create";
-
-        return;
-    }
-
-    [super forwardInvocation:invocation];
+    NSObject *sender;
+    [invocation getArgument:&sender atIndex:2];
+    qCDebug(lcQpaMenus) << "Forwarding" << invocation.selector << "from" << sender;
+    Q_ASSERT(qt_objc_cast<QCocoaNSMenuItem *>(sender));
+    invocation.selector = @selector(qt_itemFired:);
+    [invocation invokeWithTarget:[QCocoaApplicationDelegate sharedDelegate]];
 }
 
 @end