- Revision
- 261875
- Author
- [email protected]
- Date
- 2020-05-19 11:40:23 -0700 (Tue, 19 May 2020)
Log Message
[macOS] Drag and drop within a contenteditable area duplicates attachment-backed images
https://bugs.webkit.org/show_bug.cgi?id=212075
<rdar://problem/62434146>
Reviewed by Andy Estes.
Source/WebKit:
When the attachment API is enabled, starting a drag on an attachment-element-backed image within a
contenteditable area sometimes causes the ranged selection to collapse. This is because `WebViewImpl::startDrag`
has a separate codepath for starting a drag with attachment metadata that doesn't invoke `NSView`'s deprecated
`-dragImage:at:offset:event:pasteboar:source:slideBack:` method, but instead uses
`-beginDraggingSessionWithItems:event:source:`. While the former spins the runloop until the end of the drag
session, the latter does not and returns immediately after beginning the drag. As a result,
`WebPageProxy::didStartDrag()` is actually invoked after the drag ends when dragging non-attachment content,
whereas `WebPageProxy::didStartDrag()` is invoked immediately after beginning the drag when starting a drag in
an attachment or attachment-backed image element.
This call to `didStartDrag()` is used to inform the web process that dragging has begun, and sets the
`m_isStartingDrag` flag on `WebPage` to false. As a result, `m_isStartingDrag` is true over the course of the
drag session when dragging non-attachment elements, but it's only true while starting the drag when dragging
attachments. This flag is consulted in `WebPage::mouseEvent()` to determine whether we should avoid handling the
incoming mouse event when beginning a drag. This prevents these incoming mouse events from triggering selection
changes while dragging (for more details, refer to <http://trac.webkit.org/r176687>).
Now, if we combine this with the fact that mouse events can be queued up in the UI process (refer to
<https://trac.webkit.org/r230817>), it's possible to end up handling these queued mouse events in the web
process after `m_isStartingDrag` has been set back to false when dragging an attachment. This doesn't happen
when dragging non-attachment content, since we don't call `didStartDrag()` until the drag has already ended, so
any queued mouse events are sent to the web process and immediately dropped on the floor since
`m_isStartingDrag` is true.
To address this, we make a couple of minor adjustments:
1. Invoke `didStartDrag()` right before calling into `-dragImage:at:offset:event:pasteboar:source:slideBack:`,
so that we don't wait until the drag ends to set `m_isStartingDrag` to false. This makes the behavior of
both codepaths for starting a drag (attachment and non-attachment) consistent, and also makes it consistent
with iOS.
2. In `didStartDrag()`, discard any queued mouse events to prevent them from being handled by the page (e.g.
causing the selection to change immediately after starting the drag). We already have identical logic to do
this when showing a context menu, so we can factor this out into a private `WebPageProxy` helper and call it
from both places.
Test: WKAttachmentTestsMac.DraggingAttachmentBackedImagePreservesRangedSelection
* UIProcess/Cocoa/WebViewImpl.mm:
(WebKit::WebViewImpl::startDrag):
Invoke `didStartDrag()` earlier when starting a drag using the deprecated method, such that we don't wait until
the drag ends to set `m_isStartingDrag` to false in the web process.
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::discardQueuedMouseEvents):
Refactor logic to discard all queued mouse events into a separate private helper.
(WebKit::WebPageProxy::didStartDrag):
Use the above helper to empty queued mouse events when starting a drag.
(WebKit::WebPageProxy::showContextMenu):
* UIProcess/WebPageProxy.h:
Tools:
Add a new API test to verify that starting a drag on an attachment-backed image in an editable container with
queued mouse move events does not cause the selection to collapse. See Source/WebKit/ChangeLog for more details.
* TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
* TestWebKitAPI/cocoa/TestWKWebView.h:
* TestWebKitAPI/cocoa/TestWKWebView.mm:
(-[TestWKWebView waitForPendingMouseEvents]):
Move this method from DragAndDropSimulator into TestWKWebView.
* TestWebKitAPI/mac/DragAndDropSimulatorMac.mm:
(-[DragAndDropTestWKWebView waitForPendingMouseEvents]): Deleted.
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (261874 => 261875)
--- trunk/Source/WebKit/ChangeLog 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Source/WebKit/ChangeLog 2020-05-19 18:40:23 UTC (rev 261875)
@@ -1,3 +1,67 @@
+2020-05-19 Wenson Hsieh <[email protected]>
+
+ [macOS] Drag and drop within a contenteditable area duplicates attachment-backed images
+ https://bugs.webkit.org/show_bug.cgi?id=212075
+ <rdar://problem/62434146>
+
+ Reviewed by Andy Estes.
+
+ When the attachment API is enabled, starting a drag on an attachment-element-backed image within a
+ contenteditable area sometimes causes the ranged selection to collapse. This is because `WebViewImpl::startDrag`
+ has a separate codepath for starting a drag with attachment metadata that doesn't invoke `NSView`'s deprecated
+ `-dragImage:at:offset:event:pasteboar:source:slideBack:` method, but instead uses
+ `-beginDraggingSessionWithItems:event:source:`. While the former spins the runloop until the end of the drag
+ session, the latter does not and returns immediately after beginning the drag. As a result,
+ `WebPageProxy::didStartDrag()` is actually invoked after the drag ends when dragging non-attachment content,
+ whereas `WebPageProxy::didStartDrag()` is invoked immediately after beginning the drag when starting a drag in
+ an attachment or attachment-backed image element.
+
+ This call to `didStartDrag()` is used to inform the web process that dragging has begun, and sets the
+ `m_isStartingDrag` flag on `WebPage` to false. As a result, `m_isStartingDrag` is true over the course of the
+ drag session when dragging non-attachment elements, but it's only true while starting the drag when dragging
+ attachments. This flag is consulted in `WebPage::mouseEvent()` to determine whether we should avoid handling the
+ incoming mouse event when beginning a drag. This prevents these incoming mouse events from triggering selection
+ changes while dragging (for more details, refer to <http://trac.webkit.org/r176687>).
+
+ Now, if we combine this with the fact that mouse events can be queued up in the UI process (refer to
+ <https://trac.webkit.org/r230817>), it's possible to end up handling these queued mouse events in the web
+ process after `m_isStartingDrag` has been set back to false when dragging an attachment. This doesn't happen
+ when dragging non-attachment content, since we don't call `didStartDrag()` until the drag has already ended, so
+ any queued mouse events are sent to the web process and immediately dropped on the floor since
+ `m_isStartingDrag` is true.
+
+ To address this, we make a couple of minor adjustments:
+
+ 1. Invoke `didStartDrag()` right before calling into `-dragImage:at:offset:event:pasteboar:source:slideBack:`,
+ so that we don't wait until the drag ends to set `m_isStartingDrag` to false. This makes the behavior of
+ both codepaths for starting a drag (attachment and non-attachment) consistent, and also makes it consistent
+ with iOS.
+
+ 2. In `didStartDrag()`, discard any queued mouse events to prevent them from being handled by the page (e.g.
+ causing the selection to change immediately after starting the drag). We already have identical logic to do
+ this when showing a context menu, so we can factor this out into a private `WebPageProxy` helper and call it
+ from both places.
+
+ Test: WKAttachmentTestsMac.DraggingAttachmentBackedImagePreservesRangedSelection
+
+ * UIProcess/Cocoa/WebViewImpl.mm:
+ (WebKit::WebViewImpl::startDrag):
+
+ Invoke `didStartDrag()` earlier when starting a drag using the deprecated method, such that we don't wait until
+ the drag ends to set `m_isStartingDrag` to false in the web process.
+
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::discardQueuedMouseEvents):
+
+ Refactor logic to discard all queued mouse events into a separate private helper.
+
+ (WebKit::WebPageProxy::didStartDrag):
+
+ Use the above helper to empty queued mouse events when starting a drag.
+
+ (WebKit::WebPageProxy::showContextMenu):
+ * UIProcess/WebPageProxy.h:
+
2020-05-19 Andy Estes <[email protected]>
[Apple Pay] Add testing and logging for ApplePaySetup
Modified: trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm (261874 => 261875)
--- trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm 2020-05-19 18:40:23 UTC (rev 261875)
@@ -4206,11 +4206,12 @@
return;
}
+ m_page->didStartDrag();
+
[pasteboard setString:@"" forType:PasteboardTypes::WebDummyPboardType];
ALLOW_DEPRECATED_DECLARATIONS_BEGIN
[m_view dragImage:dragNSImage.get() at:NSPointFromCGPoint(clientDragLocation) offset:NSZeroSize event:m_lastMouseDownEvent.get() pasteboard:pasteboard source:m_view.getAutoreleased() slideBack:YES];
ALLOW_DEPRECATED_DECLARATIONS_END
- m_page->didStartDrag();
}
static bool matchesExtensionOrEquivalent(const String& filename, const String& extension)
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (261874 => 261875)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2020-05-19 18:40:23 UTC (rev 261875)
@@ -2368,6 +2368,12 @@
}
#endif
+void WebPageProxy::discardQueuedMouseEvents()
+{
+ while (m_mouseEventQueue.size() > 1)
+ m_mouseEventQueue.removeLast();
+}
+
#if ENABLE(DRAG_SUPPORT)
void WebPageProxy::dragEntered(DragData& dragData, const String& dragStorageName)
{
@@ -2454,8 +2460,11 @@
void WebPageProxy::didStartDrag()
{
- if (hasRunningProcess())
- send(Messages::WebPage::DidStartDrag());
+ if (!hasRunningProcess())
+ return;
+
+ discardQueuedMouseEvents();
+ send(Messages::WebPage::DidStartDrag());
}
void WebPageProxy::dragCancelled()
@@ -6537,8 +6546,7 @@
// Discard any enqueued mouse events that have been delivered to the UIProcess whilst the WebProcess is still processing the
// MouseDown event that triggered this ShowContextMenu message. This can happen if we take too long to enter the nested runloop.
ASSERT(isProcessingMouseEvents());
- while (m_mouseEventQueue.size() > 1)
- m_mouseEventQueue.takeLast();
+ discardQueuedMouseEvents();
m_activeContextMenuContextData = contextMenuContextData;
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (261874 => 261875)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.h 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h 2020-05-19 18:40:23 UTC (rev 261875)
@@ -2071,6 +2071,8 @@
void setCursor(const WebCore::Cursor&);
void setCursorHiddenUntilMouseMoves(bool);
+ void discardQueuedMouseEvents();
+
void didReceiveEvent(uint32_t opaqueType, bool handled);
void voidCallback(CallbackID);
Modified: trunk/Tools/ChangeLog (261874 => 261875)
--- trunk/Tools/ChangeLog 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Tools/ChangeLog 2020-05-19 18:40:23 UTC (rev 261875)
@@ -1,3 +1,24 @@
+2020-05-19 Wenson Hsieh <[email protected]>
+
+ [macOS] Drag and drop within a contenteditable area duplicates attachment-backed images
+ https://bugs.webkit.org/show_bug.cgi?id=212075
+ <rdar://problem/62434146>
+
+ Reviewed by Andy Estes.
+
+ Add a new API test to verify that starting a drag on an attachment-backed image in an editable container with
+ queued mouse move events does not cause the selection to collapse. See Source/WebKit/ChangeLog for more details.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
+ * TestWebKitAPI/cocoa/TestWKWebView.h:
+ * TestWebKitAPI/cocoa/TestWKWebView.mm:
+ (-[TestWKWebView waitForPendingMouseEvents]):
+
+ Move this method from DragAndDropSimulator into TestWKWebView.
+
+ * TestWebKitAPI/mac/DragAndDropSimulatorMac.mm:
+ (-[DragAndDropTestWKWebView waitForPendingMouseEvents]): Deleted.
+
2020-05-19 Carlos Alberto Lopez Perez <[email protected]>
[buildbot][GTK][WPE] Pass --verbose flag when running _javascript_Core tests
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm (261874 => 261875)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm 2020-05-19 18:40:23 UTC (rev 261875)
@@ -1567,6 +1567,37 @@
#if PLATFORM(MAC)
+TEST(WKAttachmentTestsMac, DraggingAttachmentBackedImagePreservesRangedSelection)
+{
+ platformCopyPNG();
+
+ RetainPtr<_WKAttachment> attachment;
+ auto webView = webViewForTestingAttachments(NSMakeSize(250, 250));
+ {
+ ObserveAttachmentUpdatesForScope observer(webView.get());
+ [webView _synchronouslyExecuteEditCommand:@"Paste" argument:nil];
+ EXPECT_EQ(1U, observer.observer().inserted.count);
+ attachment = observer.observer().inserted[0];
+ }
+
+ [webView waitForImageElementSizeToBecome:CGSizeMake(215, 174)];
+
+ [webView mouseEnterAtPoint:CGPointMake(125, 125)];
+ [webView mouseMoveToPoint:CGPointMake(125, 125) withFlags:0];
+ [webView mouseDownAtPoint:CGPointMake(125, 125) simulatePressure:NO];
+ [webView waitForPendingMouseEvents];
+
+ [webView mouseDragToPoint:CGPointMake(150, 150)];
+ [webView mouseDragToPoint:CGPointMake(200, 150)];
+ [webView waitForPendingMouseEvents];
+
+ [webView mouseUpAtPoint:CGPointMake(200, 150)];
+ [webView waitForPendingMouseEvents];
+
+ EXPECT_WK_STREQ("Range", [webView stringByEvaluatingJavaScript:@"getSelection().type"]);
+ EXPECT_TRUE([[webView objectByEvaluatingJavaScript:@"getSelection().containsNode(document.querySelector('img'))"] boolValue]);
+}
+
TEST(WKAttachmentTestsMac, InsertPastedFileURLsAsAttachments)
{
NSPasteboard *pasteboard = [NSPasteboard generalPasteboard];
Modified: trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.h (261874 => 261875)
--- trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.h 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.h 2020-05-19 18:40:23 UTC (rev 261875)
@@ -129,6 +129,7 @@
- (void)sendClickAtPoint:(NSPoint)pointInWindow;
- (NSWindow *)hostWindow;
- (void)typeCharacter:(char)character;
+- (void)waitForPendingMouseEvents;
@end
#endif
Modified: trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.mm (261874 => 261875)
--- trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.mm 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.mm 2020-05-19 18:40:23 UTC (rev 261875)
@@ -753,6 +753,15 @@
[self keyUp:[NSEvent keyEventWithType:keyUpEventType location:NSZeroPoint modifierFlags:0 timestamp:GetCurrentEventTime() windowNumber:[_hostWindow windowNumber] context:nil characters:characterAsString charactersIgnoringModifiers:characterAsString isARepeat:NO keyCode:character]];
}
+- (void)waitForPendingMouseEvents
+{
+ __block bool doneProcessingMouseEvents = false;
+ [self _doAfterProcessingAllPendingMouseEvents:^{
+ doneProcessingMouseEvents = true;
+ }];
+ TestWebKitAPI::Util::run(&doneProcessingMouseEvents);
+}
+
@end
#endif // PLATFORM(MAC)
Modified: trunk/Tools/TestWebKitAPI/mac/DragAndDropSimulatorMac.mm (261874 => 261875)
--- trunk/Tools/TestWebKitAPI/mac/DragAndDropSimulatorMac.mm 2020-05-19 18:23:57 UTC (rev 261874)
+++ trunk/Tools/TestWebKitAPI/mac/DragAndDropSimulatorMac.mm 2020-05-19 18:40:23 UTC (rev 261875)
@@ -70,15 +70,6 @@
return [_dragAndDropSimulator draggingSession];
}
-- (void)waitForPendingMouseEvents
-{
- __block bool doneProcessMouseEvents = false;
- [self _doAfterProcessingAllPendingMouseEvents:^{
- doneProcessMouseEvents = true;
- }];
- TestWebKitAPI::Util::run(&doneProcessMouseEvents);
-}
-
@end
// This exceeds the default drag hysteresis of all potential drag types.
