Changeset 247326 in webkit

Timestamp:

Jul 10, 2019 1:40:05 PM (5 years ago)

Author:

dino@apple.com

Message:

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:]):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/API/Cocoa/_WKActivatedElementInfo.mm (modified) (3 diffs)
• UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h (modified) (2 diffs)
• UIProcess/API/Cocoa/_WKElementAction.h (modified) (1 diff)
• UIProcess/API/Cocoa/_WKElementAction.mm (modified) (2 diffs)
• UIProcess/API/Cocoa/_WKElementActionInternal.h (modified) (1 diff)
• UIProcess/ios/WKContentViewInteraction.mm (modified) (5 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm

@@ -42 +42 @@
     RetainPtr<NSURL> _imageURL;
     RetainPtr<NSString> _title;
-    CGPoint _interactionLocation;
+    WebCore::IntPoint _interactionLocation;
     RetainPtr<NSString> _ID;
     RefPtr<WebKit::ShareableBitmap> _image;
@@ -89 +89 @@
 #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]))
@@ -134 +134 @@
 }
 
-- (CGPoint)_interactionLocation
+- (WebCore::IntPoint)_interactionLocation
 {
     return _interactionLocation;

trunk/Source/WebKit/UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h

@@ -29 +29 @@
 #import "_WKActivatedElementInfo.h"
 
+#import <WebCore/IntPoint.h>
+
 namespace WebKit {
     class ShareableBitmap;
@@ -39 +41 @@
 - (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

trunk/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.h

@@ -68 +68 @@
 - (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;

trunk/Source/WebKit/UIProcess/API/Cocoa/_WKElementAction.mm

@@ -124 +124 @@
         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;
@@ -136 +140 @@
         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;

trunk/Source/WebKit/UIProcess/API/Cocoa/_WKElementActionInternal.h

@@ -37 +37 @@
 - (void)_runActionWithElementInfo:(_WKActivatedElementInfo *)info forActionSheetAssistant:(WKActionSheetAssistant *)assistant;
 
+- (UIAction *)uiActionForElementInfo:(_WKActivatedElementInfo *)elementInfo;
+
 @end
 

trunk/Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm

@@ -77 +77 @@
 #import "_WKActivatedElementInfoInternal.h"
 #import "_WKElementAction.h"
+#import "_WKElementActionInternal.h"
 #import "_WKFocusedElementInfo.h"
 #import "_WKInputDelegate.h"
@@ -7862 +7863 @@
         };
 
+        _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]);
@@ -7873 +7876 @@
 
         if (configurationFromWKUIDelegate) {
+            strongSelf->_page->startInteractionWithElementAtPosition(strongSelf->_positionInformation.request.point);
             strongSelf->_contextMenuActionProviderDelegateNeedsOverride = YES;
             continueWithContextMenuConfiguration(configurationFromWKUIDelegate);
@@ -7895 +7899 @@
             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);
@@ -8121 +8126 @@
         }
     }
+
+    _page->stopInteraction();
 
     _contextMenuLegacyPreviewController = nullptr;