Share or Copy image from context menu does not share the correct data
authordino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jul 2019 20:40:05 +0000 (20:40 +0000)
committerdino@apple.com <dino@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jul 2019 20:40:05 +0000 (20:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199681
<rdar://problem/50538771>

Reviewed by Tim Horton.

The UIContextMenuInteraction calls didEndInteraction before executing the
actions of a selected menu item. This means we were assuming the interaction
had finished before performing the action triggered in the interaction, ending
up in the state where we had forgotten which element we were working with.

Rather than ask for UIKit to change, I'm just starting the interaction again
as the action is run. Thankfully we already had the location of the interaction.
There is a small risk that the page has changed in the meantime, but I'm not
sure what to do about that.

While here, I moved a method only used by us into _WKElementActionInternal,
and changed the location stored by _WKActivatedElementInfo from a CGPoint
to an WebCore::IntPoint (since it doesn't escape WebKit).

* UIProcess/API/Cocoa/_WKActivatedElementInfo.mm: Use a WebCore::IntPoint rather than a CGPoint.
(-[_WKActivatedElementInfo _initWithType:URL:imageURL:location:title:ID:rect:image:]):
(-[_WKActivatedElementInfo _initWithType:URL:imageURL:location:title:ID:rect:image:userInfo:]):
(-[_WKActivatedElementInfo _interactionLocation]):
* UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h:

* UIProcess/API/Cocoa/_WKElementAction.h: Move uiActionForElementInfo to Internal.
* UIProcess/API/Cocoa/_WKElementActionInternal.h:

* UIProcess/API/Cocoa/_WKElementAction.mm: When executing the handlers, restart the interaction
using the location in _WKActivatedElementInfo.
(+[_WKElementAction _elementActionWithType:customTitle:assistant:]):

* UIProcess/ios/WKContentViewInteraction.mm: Explicitly start and stop interactions at
the appropriate points in the UIContextMenu flow. This isn't really needed since we're
doing it in the handlers, but it will be correct if the UIKit delegate order changes.
(-[WKContentView continueContextMenuInteraction:]):
(-[WKContentView contextMenuInteractionDidEnd:]):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247326 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm
Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h
Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.h
Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm
Source/WebKit/UIProcess/API/Cocoa/_WKElementActionInternal.h
Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

index ce02fab..f19f86a 100644 (file)
@@ -1,3 +1,44 @@
+2019-07-10  Dean Jackson  <dino@apple.com>
+
+        Share or Copy image from context menu does not share the correct data
+        https://bugs.webkit.org/show_bug.cgi?id=199681
+        <rdar://problem/50538771>
+
+        Reviewed by Tim Horton.
+
+        The UIContextMenuInteraction calls didEndInteraction before executing the
+        actions of a selected menu item. This means we were assuming the interaction
+        had finished before performing the action triggered in the interaction, ending
+        up in the state where we had forgotten which element we were working with.
+
+        Rather than ask for UIKit to change, I'm just starting the interaction again
+        as the action is run. Thankfully we already had the location of the interaction.
+        There is a small risk that the page has changed in the meantime, but I'm not
+        sure what to do about that.
+
+        While here, I moved a method only used by us into _WKElementActionInternal,
+        and changed the location stored by _WKActivatedElementInfo from a CGPoint
+        to an WebCore::IntPoint (since it doesn't escape WebKit).
+
+        * UIProcess/API/Cocoa/_WKActivatedElementInfo.mm: Use a WebCore::IntPoint rather than a CGPoint.
+        (-[_WKActivatedElementInfo _initWithType:URL:imageURL:location:title:ID:rect:image:]):
+        (-[_WKActivatedElementInfo _initWithType:URL:imageURL:location:title:ID:rect:image:userInfo:]):
+        (-[_WKActivatedElementInfo _interactionLocation]):
+        * UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h:
+
+        * UIProcess/API/Cocoa/_WKElementAction.h: Move uiActionForElementInfo to Internal.
+        * UIProcess/API/Cocoa/_WKElementActionInternal.h:
+
+        * UIProcess/API/Cocoa/_WKElementAction.mm: When executing the handlers, restart the interaction
+        using the location in _WKActivatedElementInfo.
+        (+[_WKElementAction _elementActionWithType:customTitle:assistant:]):
+
+        * UIProcess/ios/WKContentViewInteraction.mm: Explicitly start and stop interactions at
+        the appropriate points in the UIContextMenu flow. This isn't really needed since we're
+        doing it in the handlers, but it will be correct if the UIKit delegate order changes.
+        (-[WKContentView continueContextMenuInteraction:]):
+        (-[WKContentView contextMenuInteractionDidEnd:]):
+
 2019-07-10  Chris Dumez  <cdumez@apple.com>
 
         Crash under IPC::Connection::waitForMessage()
index 24615bd..53321bb 100644 (file)
@@ -41,7 +41,7 @@
     RetainPtr<NSURL> _URL;
     RetainPtr<NSURL> _imageURL;
     RetainPtr<NSString> _title;
-    CGPoint _interactionLocation;
+    WebCore::IntPoint _interactionLocation;
     RetainPtr<NSString> _ID;
     RefPtr<WebKit::ShareableBitmap> _image;
 #if PLATFORM(IOS_FAMILY)
 }
 #endif
 
-- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(CGPoint)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image
+- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(const WebCore::IntPoint&)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image
 {
     return [self _initWithType:type URL:url imageURL:imageURL location:location title:title ID:ID rect:rect image:image userInfo:nil];
 }
 
-- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(CGPoint)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image userInfo:(NSDictionary *)userInfo
+- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(const WebCore::IntPoint&)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image userInfo:(NSDictionary *)userInfo
 {
     if (!(self = [super init]))
         return nil;
     return _ID.get();
 }
 
-- (CGPoint)_interactionLocation
+- (WebCore::IntPoint)_interactionLocation
 {
     return _interactionLocation;
 }
index 42ee26d..ae57bb3 100644 (file)
@@ -28,6 +28,8 @@
 #endif
 #import "_WKActivatedElementInfo.h"
 
+#import <WebCore/IntPoint.h>
+
 namespace WebKit {
     class ShareableBitmap;
 }
@@ -38,9 +40,9 @@ namespace WebKit {
 + (instancetype)activatedElementInfoWithInteractionInformationAtPosition:(const WebKit::InteractionInformationAtPosition&)information;
 - (instancetype)_initWithInteractionInformationAtPosition:(const WebKit::InteractionInformationAtPosition&)information;
 #endif
-- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(CGPoint)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image;
-- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(CGPoint)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image userInfo:(NSDictionary *)userInfo;
+- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(const WebCore::IntPoint&)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image;
+- (instancetype)_initWithType:(_WKActivatedElementType)type URL:(NSURL *)url imageURL:(NSURL *)imageURL location:(const WebCore::IntPoint&)location title:(NSString *)title ID:(NSString *)ID rect:(CGRect)rect image:(WebKit::ShareableBitmap*)image userInfo:(NSDictionary *)userInfo;
 
-@property (nonatomic, readonly) CGPoint _interactionLocation;
+@property (nonatomic, readonly) WebCore::IntPoint _interactionLocation;
 
 @end
index 9d0ab46..85ec135 100644 (file)
@@ -67,8 +67,6 @@ WK_CLASS_AVAILABLE(macos(10.10), ios(8.0))
 
 - (void)runActionWithElementInfo:(_WKActivatedElementInfo *)info WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(9.0));
 
-- (UIAction *)uiActionForElementInfo:(_WKActivatedElementInfo *)elementInfo WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(13.0));
-
 @property (nonatomic, readonly) _WKElementActionType type;
 @property (nonatomic, readonly) NSString* title;
 @property (nonatomic, copy) WKElementActionDismissalHandler dismissalHandler;
index 73de825..6aa8a61 100644 (file)
@@ -123,7 +123,11 @@ static void addToReadingList(NSURL *targetURL, NSString *title)
     case _WKElementActionTypeCopy:
         title = WEB_UI_STRING_KEY("Copy", "Copy (ActionSheet)", "Title for Copy Link or Image action button");
         handler = ^(WKActionSheetAssistant *assistant, _WKActivatedElementInfo *actionInfo) {
+            if ([assistant.delegate respondsToSelector:@selector(actionSheetAssistant:willStartInteractionWithElement:)])
+                [assistant.delegate actionSheetAssistant:assistant willStartInteractionWithElement:actionInfo];
             [assistant.delegate actionSheetAssistant:assistant performAction:WebKit::SheetAction::Copy];
+            if ([assistant.delegate respondsToSelector:@selector(actionSheetAssistantDidStopInteraction:)])
+                [assistant.delegate actionSheetAssistantDidStopInteraction:assistant];
         };
         break;
     case _WKElementActionTypeOpen:
@@ -135,7 +139,11 @@ static void addToReadingList(NSURL *targetURL, NSString *title)
     case _WKElementActionTypeSaveImage:
         title = WEB_UI_STRING("Add to Photos", "Title for Add to Photos action button");
         handler = ^(WKActionSheetAssistant *assistant, _WKActivatedElementInfo *actionInfo) {
+            if ([assistant.delegate respondsToSelector:@selector(actionSheetAssistant:willStartInteractionWithElement:)])
+                [assistant.delegate actionSheetAssistant:assistant willStartInteractionWithElement:actionInfo];
             [assistant.delegate actionSheetAssistant:assistant performAction:WebKit::SheetAction::SaveImage];
+            if ([assistant.delegate respondsToSelector:@selector(actionSheetAssistantDidStopInteraction:)])
+                [assistant.delegate actionSheetAssistantDidStopInteraction:assistant];
         };
         break;
 #if HAVE(SAFARI_SERVICES_FRAMEWORK)
index a08d423..6c48f10 100644 (file)
@@ -36,6 +36,8 @@
 + (instancetype)_elementActionWithType:(_WKElementActionType)type title:(NSString *)title actionHandler:(WKElementActionHandler)actionHandler;
 - (void)_runActionWithElementInfo:(_WKActivatedElementInfo *)info forActionSheetAssistant:(WKActionSheetAssistant *)assistant;
 
+- (UIAction *)uiActionForElementInfo:(_WKActivatedElementInfo *)elementInfo;
+
 @end
 
 #endif // PLATFORM(IOS_FAMILY)
index c57e940..656a192 100644 (file)
@@ -76,6 +76,7 @@
 #import "WebProcessProxy.h"
 #import "_WKActivatedElementInfoInternal.h"
 #import "_WKElementAction.h"
+#import "_WKElementActionInternal.h"
 #import "_WKFocusedElementInfo.h"
 #import "_WKInputDelegate.h"
 #import "_WKTextInputContextInternal.h"
@@ -7861,6 +7862,8 @@ static NSString *titleForMenu(bool isLink, bool showLinkPreviews, const URL& url
             return strongSelf->_contextMenuLegacyPreviewController.get();
         };
 
+        _page->startInteractionWithElementAtPosition(_positionInformation.request.point);
+
         // FIXME: Should we provide an identifier and ASSERT in delegates if we don't match?
         return continueWithContextMenuConfiguration([UIContextMenuConfiguration configurationWithIdentifier:nil previewProvider:contentPreviewProvider actionProvider:actionMenuProvider]);
     }
@@ -7872,6 +7875,7 @@ static NSString *titleForMenu(bool isLink, bool showLinkPreviews, const URL& url
             return continueWithContextMenuConfiguration(nil);
 
         if (configurationFromWKUIDelegate) {
+            strongSelf->_page->startInteractionWithElementAtPosition(strongSelf->_positionInformation.request.point);
             strongSelf->_contextMenuActionProviderDelegateNeedsOverride = YES;
             continueWithContextMenuConfiguration(configurationFromWKUIDelegate);
             return;
@@ -7894,6 +7898,7 @@ static NSString *titleForMenu(bool isLink, bool showLinkPreviews, const URL& url
             NSDictionary *context = [strongSelf dataDetectionContextForPositionInformation:strongSelf->_positionInformation];
             UIContextMenuConfiguration *configurationFromDD = [ddContextMenuActionClass contextMenuConfigurationForURL:linkURL identifier:strongSelf->_positionInformation.dataDetectorIdentifier selectedText:[strongSelf selectedText] results:strongSelf->_positionInformation.dataDetectorResults.get() inView:strongSelf.get() context:context menuIdentifier:nil];
             strongSelf->_contextMenuActionProviderDelegateNeedsOverride = YES;
+            strongSelf->_page->startInteractionWithElementAtPosition(strongSelf->_positionInformation.request.point);
             if (strongSelf->_showLinkPreviews)
                 return continueWithContextMenuConfiguration(configurationFromDD);
             return continueWithContextMenuConfiguration([UIContextMenuConfiguration configurationWithIdentifier:[configurationFromDD identifier] previewProvider:nil actionProvider:[configurationFromDD actionProvider]]);
@@ -8121,6 +8126,8 @@ static RetainPtr<UITargetedPreview> createFallbackTargetedPreview(UIView *rootVi
         }
     }
 
+    _page->stopInteraction();
+
     _contextMenuLegacyPreviewController = nullptr;
     _contextMenuLegacyMenu = nullptr;
     _contextMenuHasRequestedLegacyData = NO;