- Revision
- 226787
- Author
- wenson_hs...@apple.com
- Date
- 2018-01-11 10:42:14 -0800 (Thu, 11 Jan 2018)
Log Message
[Attachment Support] Support moving attachment elements in editable areas using drag and drop
https://bugs.webkit.org/show_bug.cgi?id=181337
<rdar://problem/36324813>
Reviewed by Tim Horton.
Source/WebCore:
Makes slight adjustments to attachment-specific drag and drop logic to ensure that moving attachments via drag
and drop behaves correctly. See per-change comments for more detail.
Tests: WKAttachmentTests.DragInPlaceVideoAttachmentElement
WKAttachmentTests.MoveAttachmentElementAsIconByDragging
WKAttachmentTests.MoveInPlaceAttachmentElementByDragging
* editing/cocoa/EditorCocoa.mm:
(WebCore::Editor::getPasteboardTypesAndDataForAttachment):
Stop vending the private web archive pasteboard type for attachments, for now. This works around issues where an
attachment element that is dragged and dropped within the same page may lose its blob backing data if we try to
remove and insert it as a fragment from the archive. Providing a web archive would allow us to avoid destroying
and recreating an attachment element when dragging within the same page, but this is a nice-to-have optimization
we can re-enable after investigation in a subsequent patch.
* html/HTMLAttachmentElement.cpp:
(WebCore::HTMLAttachmentElement::populateShadowRootIfNecessary):
Add `draggable=false` to the image element of an in-place attachment element.
* page/DragController.cpp:
(WebCore::enclosingAttachmentElement):
(WebCore::DragController::draggableElement const):
Tweak single-selected-attachment handling to account for in-place attachments. Since the hit-tested node is
inside the shadow subtree of the attachment element, the condition needs to check for the startElement as well
as the startElement's shadow host.
(WebCore::DragController::startDrag):
Make two tweaks here. First, don't require a RenderAttachment to drag an attachment element (this is required
for dragging in-place attachments). This was added in r217083 to address <rdar://problem/32282831>, but is no
longer correct, since attachments may now be displayed in-place.
Secondly, only restore the previous selection if the attachment is in a richly contenteditable area. This was
added to prevent the selection highlight from appearing in when dragging non-editable attachment elements in the
Mail viewer. However, to allow drag moves to occur, we need the selection to persist after drag start.
Tools:
Add 3 new API tests for attachment element dragging.
* TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
(-[TestWKWebView expectElementTag:toComeBefore:]):
(-[NSItemProvider expectType:withData:]):
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (226786 => 226787)
--- trunk/Source/WebCore/ChangeLog 2018-01-11 18:26:26 UTC (rev 226786)
+++ trunk/Source/WebCore/ChangeLog 2018-01-11 18:42:14 UTC (rev 226787)
@@ -1,3 +1,50 @@
+2018-01-11 Wenson Hsieh <wenson_hs...@apple.com>
+
+ [Attachment Support] Support moving attachment elements in editable areas using drag and drop
+ https://bugs.webkit.org/show_bug.cgi?id=181337
+ <rdar://problem/36324813>
+
+ Reviewed by Tim Horton.
+
+ Makes slight adjustments to attachment-specific drag and drop logic to ensure that moving attachments via drag
+ and drop behaves correctly. See per-change comments for more detail.
+
+ Tests: WKAttachmentTests.DragInPlaceVideoAttachmentElement
+ WKAttachmentTests.MoveAttachmentElementAsIconByDragging
+ WKAttachmentTests.MoveInPlaceAttachmentElementByDragging
+
+ * editing/cocoa/EditorCocoa.mm:
+ (WebCore::Editor::getPasteboardTypesAndDataForAttachment):
+
+ Stop vending the private web archive pasteboard type for attachments, for now. This works around issues where an
+ attachment element that is dragged and dropped within the same page may lose its blob backing data if we try to
+ remove and insert it as a fragment from the archive. Providing a web archive would allow us to avoid destroying
+ and recreating an attachment element when dragging within the same page, but this is a nice-to-have optimization
+ we can re-enable after investigation in a subsequent patch.
+
+ * html/HTMLAttachmentElement.cpp:
+ (WebCore::HTMLAttachmentElement::populateShadowRootIfNecessary):
+
+ Add `draggable=false` to the image element of an in-place attachment element.
+
+ * page/DragController.cpp:
+ (WebCore::enclosingAttachmentElement):
+ (WebCore::DragController::draggableElement const):
+
+ Tweak single-selected-attachment handling to account for in-place attachments. Since the hit-tested node is
+ inside the shadow subtree of the attachment element, the condition needs to check for the startElement as well
+ as the startElement's shadow host.
+
+ (WebCore::DragController::startDrag):
+
+ Make two tweaks here. First, don't require a RenderAttachment to drag an attachment element (this is required
+ for dragging in-place attachments). This was added in r217083 to address <rdar://problem/32282831>, but is no
+ longer correct, since attachments may now be displayed in-place.
+
+ Secondly, only restore the previous selection if the attachment is in a richly contenteditable area. This was
+ added to prevent the selection highlight from appearing in when dragging non-editable attachment elements in the
+ Mail viewer. However, to allow drag moves to occur, we need the selection to persist after drag start.
+
2018-01-04 Filip Pizlo <fpi...@apple.com>
CodeBlocks should be in IsoSubspaces
Modified: trunk/Source/WebCore/editing/cocoa/EditorCocoa.mm (226786 => 226787)
--- trunk/Source/WebCore/editing/cocoa/EditorCocoa.mm 2018-01-11 18:26:26 UTC (rev 226786)
+++ trunk/Source/WebCore/editing/cocoa/EditorCocoa.mm 2018-01-11 18:42:14 UTC (rev 226787)
@@ -159,10 +159,11 @@
{
auto attachmentRange = Range::create(attachment.document(), { &attachment, Position::PositionIsBeforeAnchor }, { &attachment, Position::PositionIsAfterAnchor });
client()->getClientPasteboardDataForRange(attachmentRange.ptr(), outTypes, outData);
- if (auto archive = LegacyWebArchive::create(attachmentRange.ptr())) {
- outTypes.append(WebArchivePboardType);
- outData.append(SharedBuffer::create(archive->rawDataRepresentation().get()));
- }
+ // FIXME: We should additionally write the attachment as a web archive here, such that drag and drop within the
+ // same page doesn't destroy and recreate attachments unnecessarily. This is also needed to preserve the attachment
+ // display mode when dragging and dropping or cutting and pasting. For the time being, this is disabled because
+ // inserting attachment elements from web archive data sometimes causes attachment data to be lost; this requires
+ // further investigation.
}
#endif
Modified: trunk/Source/WebCore/html/HTMLAttachmentElement.cpp (226786 => 226787)
--- trunk/Source/WebCore/html/HTMLAttachmentElement.cpp 2018-01-11 18:26:26 UTC (rev 226786)
+++ trunk/Source/WebCore/html/HTMLAttachmentElement.cpp 2018-01-11 18:42:14 UTC (rev 226787)
@@ -283,6 +283,7 @@
auto image = ensureInnerImage();
if (image->attributeWithoutSynchronization(srcAttr).isEmpty()) {
image->setAttributeWithoutSynchronization(srcAttr, DOMURL::createObjectURL(document(), *m_file));
+ image->setAttributeWithoutSynchronization(draggableAttr, AtomicString("false"));
image->setInlineStyleProperty(CSSPropertyDisplay, CSSValueInline, true);
}
Modified: trunk/Source/WebCore/page/DragController.cpp (226786 => 226787)
--- trunk/Source/WebCore/page/DragController.cpp 2018-01-11 18:26:26 UTC (rev 226786)
+++ trunk/Source/WebCore/page/DragController.cpp 2018-01-11 18:42:14 UTC (rev 226787)
@@ -719,6 +719,21 @@
return cachedImage && !cachedImage->errorOccurred() && cachedImage->imageForRenderer(renderer);
}
+#if ENABLE(ATTACHMENT_ELEMENT)
+
+static RefPtr<HTMLAttachmentElement> enclosingAttachmentElement(Element& element)
+{
+ if (is<HTMLAttachmentElement>(element))
+ return downcast<HTMLAttachmentElement>(&element);
+
+ if (is<HTMLAttachmentElement>(element.parentOrShadowHostElement()))
+ return downcast<HTMLAttachmentElement>(element.parentOrShadowHostElement());
+
+ return { };
+}
+
+#endif
+
Element* DragController::draggableElement(const Frame* sourceFrame, Element* startElement, const IntPoint& dragOrigin, DragState& state) const
{
state.type = (sourceFrame->selection().contains(dragOrigin)) ? DragSourceActionSelection : DragSourceActionNone;
@@ -725,18 +740,18 @@
if (!startElement)
return nullptr;
#if ENABLE(ATTACHMENT_ELEMENT)
- if (is<HTMLAttachmentElement>(startElement)) {
+ if (auto attachment = enclosingAttachmentElement(*startElement)) {
auto selection = sourceFrame->selection().selection();
- bool isSingleAttachmentSelection = selection.start() == Position(startElement, Position::PositionIsBeforeAnchor) && selection.end() == Position(startElement, Position::PositionIsAfterAnchor);
+ bool isSingleAttachmentSelection = selection.start() == Position(attachment.get(), Position::PositionIsBeforeAnchor) && selection.end() == Position(attachment.get(), Position::PositionIsAfterAnchor);
bool isAttachmentElementInCurrentSelection = false;
if (auto selectedRange = selection.toNormalizedRange()) {
- auto compareResult = selectedRange->compareNode(*startElement);
+ auto compareResult = selectedRange->compareNode(*attachment);
isAttachmentElementInCurrentSelection = !compareResult.hasException() && compareResult.releaseReturnValue() == Range::NODE_INSIDE;
}
if (!isAttachmentElementInCurrentSelection || isSingleAttachmentSelection) {
state.type = DragSourceActionAttachment;
- return startElement;
+ return attachment.get();
}
}
#endif
@@ -857,6 +872,9 @@
#if ENABLE(VIDEO)
includeShadowDOM = state.source->isMediaElement();
#endif
+#if ENABLE(ATTACHMENT_ELEMENT)
+ includeShadowDOM = includeShadowDOM || is<HTMLAttachmentElement>(state.source.get());
+#endif
bool sourceContainsHitNode;
if (!includeShadowDOM)
sourceContainsHitNode = state.source->contains(hitTestResult.innerNode());
@@ -1071,17 +1089,15 @@
if (is<HTMLAttachmentElement>(element) && m_dragSourceAction & DragSourceActionAttachment) {
auto& attachment = downcast<HTMLAttachmentElement>(element);
auto* attachmentRenderer = attachment.attachmentRenderer();
- if (!attachmentRenderer)
- return false;
src.editor().setIgnoreSelectionChanges(true);
auto previousSelection = src.selection().selection();
- if (hasData == HasNonDefaultPasteboardData::No) {
- selectElement(element);
- if (!dragAttachmentElement(src, attachment) && src.editor().client()) {
+ selectElement(element);
+ if (hasData == HasNonDefaultPasteboardData::No && !dragAttachmentElement(src, attachment)) {
+ auto& editor = src.editor();
+ if (editor.client()) {
#if PLATFORM(COCOA)
// Otherwise, if no file URL is specified, call out to the injected bundle to populate the pasteboard with data.
- auto& editor = src.editor();
editor.willWriteSelectionToPasteboard(src.selection().toNormalizedRange().get());
editor.writeSelectionToPasteboard(dataTransfer.pasteboard());
editor.didWriteSelectionToPasteboard();
@@ -1093,9 +1109,11 @@
if (!dragImage) {
TextIndicatorData textIndicator;
- attachmentRenderer->setShouldDrawBorder(false);
+ if (attachmentRenderer)
+ attachmentRenderer->setShouldDrawBorder(false);
dragImage = DragImage { dissolveDragImageToFraction(createDragImageForSelection(src, textIndicator), DragImageAlpha) };
- attachmentRenderer->setShouldDrawBorder(true);
+ if (attachmentRenderer)
+ attachmentRenderer->setShouldDrawBorder(true);
if (textIndicator.contentImage)
dragImage.setIndicatorData(textIndicator);
dragLoc = dragLocForSelectionDrag(src);
@@ -1102,7 +1120,8 @@
m_dragOffset = IntPoint(dragOrigin.x() - dragLoc.x(), dragOrigin.y() - dragLoc.y());
}
doSystemDrag(WTFMove(dragImage), dragLoc, dragOrigin, src, state);
- src.selection().setSelection(previousSelection);
+ if (!element.isContentRichlyEditable())
+ src.selection().setSelection(previousSelection);
src.editor().setIgnoreSelectionChanges(false);
return true;
}
Modified: trunk/Tools/ChangeLog (226786 => 226787)
--- trunk/Tools/ChangeLog 2018-01-11 18:26:26 UTC (rev 226786)
+++ trunk/Tools/ChangeLog 2018-01-11 18:42:14 UTC (rev 226787)
@@ -1,3 +1,18 @@
+2018-01-11 Wenson Hsieh <wenson_hs...@apple.com>
+
+ [Attachment Support] Support moving attachment elements in editable areas using drag and drop
+ https://bugs.webkit.org/show_bug.cgi?id=181337
+ <rdar://problem/36324813>
+
+ Reviewed by Tim Horton.
+
+ Add 3 new API tests for attachment element dragging.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
+ (-[TestWKWebView expectElementTag:toComeBefore:]):
+ (-[NSItemProvider expectType:withData:]):
+ (TestWebKitAPI::TEST):
+
2018-01-11 Jonathan Bedard <jbed...@apple.com>
REGRESSION(r225856): Incorrectly managing 'future' baseline_search_paths.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm (226786 => 226787)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm 2018-01-11 18:26:26 UTC (rev 226786)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm 2018-01-11 18:42:14 UTC (rev 226787)
@@ -184,6 +184,22 @@
@implementation TestWKWebView (AttachmentTesting)
+- (void)expectElementTag:(NSString *)tagName toComeBefore:(NSString *)otherTagName
+{
+ NSArray *tagsInBody = [self objectByEvaluatingJavaScript:@"Array.from(document.body.getElementsByTagName('*')).map(e => e.tagName)"];
+ BOOL success = [tagsInBody containsObject:tagName] && [tagsInBody containsObject:otherTagName];
+ if (success) {
+ NSUInteger index = [tagsInBody indexOfObject:tagName];
+ NSUInteger otherIndex = [tagsInBody indexOfObjectWithOptions:NSEnumerationReverse passingTest:[&] (NSString *tag, NSUInteger, BOOL *) {
+ return [tag isEqualToString:otherTagName];
+ }];
+ success = index < otherIndex;
+ }
+ EXPECT_TRUE(success);
+ if (!success)
+ NSLog(@"Expected %@ to come before %@ in tags: %@", tagName, otherTagName, tagsInBody);
+}
+
- (BOOL)_synchronouslyExecuteEditCommand:(NSString *)command argument:(NSString *)argument
{
__block bool done = false;
@@ -1311,6 +1327,91 @@
[draggingSimulator endDataTransfer];
}
+TEST(WKAttachmentTestsIOS, DragInPlaceVideoAttachmentElement)
+{
+ auto webView = webViewForTestingAttachments();
+ auto data = ""
+ RetainPtr<_WKAttachment> attachment;
+ {
+ ObserveAttachmentUpdatesForScope observer(webView.get());
+ attachment = [webView synchronouslyInsertAttachmentWithFilename:@"video.mp4" contentType:@"video/mp4" data:data.get() options:displayOptionsWithMode(_WKAttachmentDisplayModeInPlace)];
+ observer.expectAttachmentUpdates(@[], @[attachment.get()]);
+ }
+
+ [webView waitForAttachmentElementSizeToBecome:CGSizeMake(320, 240)];
+ [attachment expectRequestedDataToBe:data.get()];
+ EXPECT_WK_STREQ("video.mp4", [webView valueOfAttribute:@"title" forQuerySelector:@"attachment"]);
+ EXPECT_WK_STREQ("video/mp4", [webView valueOfAttribute:@"type" forQuerySelector:@"attachment"]);
+
+ [webView evaluateJavaScript:@"getSelection().removeAllRanges()" completionHandler:nil];
+ auto draggingSimulator = adoptNS([[DataInteractionSimulator alloc] initWithWebView:webView.get()]);
+ [draggingSimulator runFrom:CGPointMake(25, 25) to:CGPointMake(-100, -100)];
+
+ EXPECT_EQ(1U, [draggingSimulator sourceItemProviders].count);
+ NSItemProvider *itemProvider = [draggingSimulator sourceItemProviders].firstObject;
+ EXPECT_EQ(UIPreferredPresentationStyleAttachment, itemProvider.preferredPresentationStyle);
+ [itemProvider expectType:(NSString *)kUTTypeMPEG4 withData:data.get()];
+ EXPECT_WK_STREQ("video.mp4", [itemProvider suggestedName]);
+ [draggingSimulator endDataTransfer];
+}
+
+TEST(WKAttachmentTestsIOS, MoveAttachmentElementAsIconByDragging)
+{
+ auto webView = webViewForTestingAttachments();
+ auto data = ""
+ RetainPtr<_WKAttachment> attachment;
+ {
+ ObserveAttachmentUpdatesForScope observer(webView.get());
+ attachment = [webView synchronouslyInsertAttachmentWithFilename:@"document.pdf" contentType:@"application/pdf" data:data.get() options:displayOptionsWithMode(_WKAttachmentDisplayModeAsIcon)];
+ observer.expectAttachmentUpdates(@[], @[attachment.get()]);
+ }
+
+ [webView _executeEditCommand:@"InsertParagraph" argument:nil completion:nil];
+ [webView _executeEditCommand:@"InsertHTML" argument:@"<strong>text</strong>" completion:nil];
+ [webView _synchronouslyExecuteEditCommand:@"InsertParagraph" argument:nil];
+ [webView expectElementTag:@"ATTACHMENT" toComeBefore:@"STRONG"];
+
+ auto draggingSimulator = adoptNS([[DataInteractionSimulator alloc] initWithWebView:webView.get()]);
+ [draggingSimulator runFrom:CGPointMake(25, 25) to:CGPointMake(25, 125)];
+
+ attachment = [[draggingSimulator insertedAttachments] firstObject];
+ [attachment expectRequestedDataToBe:data.get()];
+ EXPECT_WK_STREQ("document.pdf", [webView valueOfAttribute:@"title" forQuerySelector:@"attachment"]);
+ EXPECT_WK_STREQ("application/pdf", [webView valueOfAttribute:@"type" forQuerySelector:@"attachment"]);
+
+ [webView expectElementTag:@"STRONG" toComeBefore:@"ATTACHMENT"];
+ [draggingSimulator endDataTransfer];
+}
+
+TEST(WKAttachmentTestsIOS, MoveInPlaceAttachmentElementByDragging)
+{
+ auto webView = webViewForTestingAttachments();
+ auto data = ""
+ RetainPtr<_WKAttachment> attachment;
+ {
+ ObserveAttachmentUpdatesForScope observer(webView.get());
+ attachment = [webView synchronouslyInsertAttachmentWithFilename:@"icon.png" contentType:@"image/png" data:data.get() options:displayOptionsWithMode(_WKAttachmentDisplayModeInPlace)];
+ observer.expectAttachmentUpdates(@[], @[attachment.get()]);
+ }
+
+ [webView waitForAttachmentElementSizeToBecome:CGSizeMake(215, 174)];
+ [webView _executeEditCommand:@"InsertParagraph" argument:nil completion:nil];
+ [webView _executeEditCommand:@"InsertHTML" argument:@"<strong>text</strong>" completion:nil];
+ [webView _synchronouslyExecuteEditCommand:@"InsertParagraph" argument:nil];
+ [webView expectElementTag:@"ATTACHMENT" toComeBefore:@"STRONG"];
+
+ auto draggingSimulator = adoptNS([[DataInteractionSimulator alloc] initWithWebView:webView.get()]);
+ [draggingSimulator runFrom:CGPointMake(25, 25) to:CGPointMake(25, 125)];
+
+ attachment = [[draggingSimulator insertedAttachments] firstObject];
+ [attachment expectRequestedDataToBe:data.get()];
+ EXPECT_WK_STREQ("icon.png", [webView valueOfAttribute:@"title" forQuerySelector:@"attachment"]);
+ EXPECT_WK_STREQ("image/png", [webView valueOfAttribute:@"type" forQuerySelector:@"attachment"]);
+
+ [webView expectElementTag:@"STRONG" toComeBefore:@"ATTACHMENT"];
+ [draggingSimulator endDataTransfer];
+}
+
#endif // PLATFORM(IOS)
} // namespace TestWebKitAPI
