[Attachment Support] Support moving attachment elements in editable areas using drag...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jan 2018 18:42:14 +0000 (18:42 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jan 2018 18:42:14 +0000 (18:42 +0000)
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):

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

Source/WebCore/ChangeLog
Source/WebCore/editing/cocoa/EditorCocoa.mm
Source/WebCore/html/HTMLAttachmentElement.cpp
Source/WebCore/page/DragController.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm

index 96ce914..16daba2 100644 (file)
@@ -1,3 +1,50 @@
+2018-01-11  Wenson Hsieh  <wenson_hsieh@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  <fpizlo@apple.com>
 
         CodeBlocks should be in IsoSubspaces
index fafe589..6e2a0d6 100644 (file)
@@ -159,10 +159,11 @@ void Editor::getPasteboardTypesAndDataForAttachment(HTMLAttachmentElement& attac
 {
     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
index a52bd14..66185f8 100644 (file)
@@ -283,6 +283,7 @@ void HTMLAttachmentElement::populateShadowRootIfNecessary()
         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);
         }
 
index 658e07f..269d549 100644 (file)
@@ -719,24 +719,39 @@ static bool imageElementIsDraggable(const HTMLImageElement& image, const Frame&
     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;
     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 @@ bool DragController::startDrag(Frame& src, const DragState& state, DragOperation
 #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 @@ bool DragController::startDrag(Frame& src, const DragState& state, DragOperation
     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,16 +1109,19 @@ bool DragController::startDrag(Frame& src, const DragState& state, DragOperation
         
         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);
             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;
     }
index 15fcff5..f0a7e7d 100644 (file)
@@ -1,3 +1,18 @@
+2018-01-11  Wenson Hsieh  <wenson_hsieh@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  <jbedard@apple.com>
 
         REGRESSION(r225856): Incorrectly managing 'future' baseline_search_paths. 
index c5decd2..a858993 100644 (file)
@@ -184,6 +184,22 @@ static _WKAttachmentDisplayOptions *displayOptionsWithMode(_WKAttachmentDisplayM
 
 @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 @@ TEST(WKAttachmentTestsIOS, DragAttachmentInsertedAsData)
     [draggingSimulator endDataTransfer];
 }
 
+TEST(WKAttachmentTestsIOS, DragInPlaceVideoAttachmentElement)
+{
+    auto webView = webViewForTestingAttachments();
+    auto data = retainPtr(testVideoData());
+    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(testPDFData());
+    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(testImageData());
+    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