[Attachment Support] The 'webkitattachmentbloburl' attribute should not persist after markup serialization

https://bugs.webkit.org/show_bug.cgi?id=180924
<rdar://problem/36099093>

Reviewed by Tim Horton.

Source/WebCore:

Work towards dragging Blob-backed attachment elements as files on iOS and Mac. It doesn't make sense for the
attachment blob URL to stick around on the element after markup serialization, so this patch removes logic that
eagerly sets the blob URL upon setting an attachment's File. Instead, we just append this attribute when
generating markup.

This patch also augments existing WKAttachmentTests to ensure that these attributes are not present.

* editing/markup.cpp:
(WebCore::StyledMarkupAccumulator::appendCustomAttributes):
(WebCore::createFragmentFromMarkup):
* html/HTMLAttachmentElement.cpp:
(WebCore::HTMLAttachmentElement::setFile):
* rendering/HitTestResult.cpp:

Fixes a related issue where an attachment is backed by Blob data (and not a file path) would specify "file:///"
as its attachment file path in DragController when starting a drag. Instead, if there is no file path, fall back
to the blob URL.

This will be tested in a future patch once a WK2 dragging simulator for Mac is implemented, and support for
dragging out Blob-backed attachments as (platform) files is implemented.

(WebCore::HitTestResult::absoluteAttachmentURL const):

Tools:

Tweaks some existing tests to check that temporary attachment serialization attributes don't stick around on the
attachment elements.

* TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
(-[TestWKWebView hasAttribute:forQuerySelector:]):
(TestWebKitAPI::TEST):


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

Author: [email protected]

Source/WebCore/ChangeLog

@@ -1,3 +1,34 @@
+2017-12-18  Wenson Hsieh  <[email protected]>
+
+        [Attachment Support] The 'webkitattachmentbloburl' attribute should not persist after markup serialization
+        https://bugs.webkit.org/show_bug.cgi?id=180924
+        <rdar://problem/36099093>
+
+        Reviewed by Tim Horton.
+
+        Work towards dragging Blob-backed attachment elements as files on iOS and Mac. It doesn't make sense for the
+        attachment blob URL to stick around on the element after markup serialization, so this patch removes logic that
+        eagerly sets the blob URL upon setting an attachment's File. Instead, we just append this attribute when
+        generating markup.
+
+        This patch also augments existing WKAttachmentTests to ensure that these attributes are not present.
+
+        * editing/markup.cpp:
+        (WebCore::StyledMarkupAccumulator::appendCustomAttributes):
+        (WebCore::createFragmentFromMarkup):
+        * html/HTMLAttachmentElement.cpp:
+        (WebCore::HTMLAttachmentElement::setFile):
+        * rendering/HitTestResult.cpp:
+
+        Fixes a related issue where an attachment is backed by Blob data (and not a file path) would specify "file:///"
+        as its attachment file path in DragController when starting a drag. Instead, if there is no file path, fall back
+        to the blob URL.
+
+        This will be tested in a future patch once a WK2 dragging simulator for Mac is implemented, and support for
+        dragging out Blob-backed attachments as (platform) files is implemented.
+
+        (WebCore::HitTestResult::absoluteAttachmentURL const):
+
 2017-12-18  Chris Dumez  <[email protected]>
 
         Default scope used when registering a service worker is wrong

Source/WebCore/editing/markup.cpp

@@ -371,15 +371,19 @@ String StyledMarkupAccumulator::stringValueForRange(const Node& node, const Rang
     return nodeValue;
 }
 
-void StyledMarkupAccumulator::appendCustomAttributes(StringBuilder& out, const Element&element, Namespaces* namespaces)
+void StyledMarkupAccumulator::appendCustomAttributes(StringBuilder& out, const Element& element, Namespaces* namespaces)
 {
 #if ENABLE(ATTACHMENT_ELEMENT)
     if (!is<HTMLAttachmentElement>(element))
         return;
 
     const HTMLAttachmentElement& attachment = downcast<HTMLAttachmentElement>(element);
-    if (attachment.file())
-        appendAttribute(out, element, Attribute(webkitattachmentpathAttr, attachment.file()->path()), namespaces);
+    if (auto* file = attachment.file()) {
+        // These attributes are only intended for File deserialization, and are removed from the generated attachment
+        // element after we've deserialized and set its backing File.
+        appendAttribute(out, element, { webkitattachmentpathAttr, file->path() }, namespaces);
+        appendAttribute(out, element, { webkitattachmentbloburlAttr, { file->url() }}, namespaces);
+    }
 #else
     UNUSED_PARAM(out);
     UNUSED_PARAM(element);
@@ -764,11 +768,13 @@ Ref<DocumentFragment> createFragmentFromMarkup(Document& document, const String&
 
     for (auto& attachment : attachments) {
         auto attachmentPath = attachment->attachmentPath();
-        if (attachmentPath.isEmpty())
-            attachment->setFile(File::deserialize({ }, attachment->blobURL(), attachment->attachmentType(), attachment->attachmentTitle()));
-        else
+        auto blobURL = attachment->blobURL();
+        if (!attachmentPath.isEmpty())
             attachment->setFile(File::create(attachmentPath));
+        else if (!blobURL.isEmpty())
+            attachment->setFile(File::deserialize({ }, blobURL, attachment->attachmentType(), attachment->attachmentTitle()));
         attachment->removeAttribute(webkitattachmentpathAttr);
+        attachment->removeAttribute(webkitattachmentbloburlAttr);
     }
 #endif
     if (!baseURL.isEmpty() && baseURL != blankURL() && baseURL != document.baseURL())

Source/WebCore/html/HTMLAttachmentElement.cpp

@@ -120,8 +120,6 @@ void HTMLAttachmentElement::setFile(RefPtr<File>&& file, UpdateDisplayAttributes
 {
     m_file = WTFMove(file);
 
-    setAttributeWithoutSynchronization(HTMLNames::webkitattachmentbloburlAttr, m_file ? m_file->url() : emptyString());
-
     if (updateAttributes == UpdateDisplayAttributes::Yes) {
         if (m_file) {
             setAttributeWithoutSynchronization(HTMLNames::titleAttr, m_file->name());

Source/WebCore/rendering/HitTestResult.cpp

@@ -356,7 +356,12 @@ URL HitTestResult::absoluteAttachmentURL() const
     if (!attachmentFile)
         return URL();
 
-    return URL::fileURLWithFileSystemPath(attachmentFile->path());
+    auto filepath = attachmentFile->path();
+    if (!filepath.isEmpty())
+        return URL::fileURLWithFileSystemPath(filepath);
+
+    ASSERT(attachmentFile->url().protocolIsBlob());
+    return attachmentFile->url();
 }
 #endif
 

Tools/ChangeLog

@@ -1,3 +1,18 @@
+2017-12-18  Wenson Hsieh  <[email protected]>
+
+        [Attachment Support] The 'webkitattachmentbloburl' attribute should not persist after markup serialization
+        https://bugs.webkit.org/show_bug.cgi?id=180924
+        <rdar://problem/36099093>
+
+        Reviewed by Tim Horton.
+
+        Tweaks some existing tests to check that temporary attachment serialization attributes don't stick around on the
+        attachment elements.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:
+        (-[TestWKWebView hasAttribute:forQuerySelector:]):
+        (TestWebKitAPI::TEST):
+
 2017-12-18  Wenson Hsieh  <[email protected]>
 
         [Attachment Support] Support representing pasted or dropped content using attachment elements

Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm

@@ -224,6 +224,11 @@ - (void)waitForAttachmentElementSizeToBecome:(CGSize)expectedSize
     }
 }
 
+- (BOOL)hasAttribute:(NSString *)attributeName forQuerySelector:(NSString *)querySelector
+{
+    return [self stringByEvaluatingJavaScript:[NSString stringWithFormat:@"document.querySelector('%@').hasAttribute('%@')", querySelector, attributeName]].boolValue;
+}
+
 - (NSString *)valueOfAttribute:(NSString *)attributeName forQuerySelector:(NSString *)querySelector
 {
     return [self stringByEvaluatingJavaScript:[NSString stringWithFormat:@"document.querySelector('%@').getAttribute('%@')", querySelector, attributeName]];
@@ -425,6 +430,8 @@ void platformCopyPNG()
 
     [firstAttachment expectRequestedDataToBe:nil];
     [secondAttachment expectRequestedDataToBe:testImageData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 }
 
 TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingAndDeletingNewline)
@@ -490,6 +497,8 @@ void platformCopyPNG()
     [webView expectUpdatesAfterCommand:@"ToggleItalic" withArgument:nil expectedRemovals:@[] expectedInsertions:@[]];
     [webView expectUpdatesAfterCommand:@"ToggleUnderline" withArgument:nil expectedRemovals:@[] expectedInsertions:@[]];
     [attachment expectRequestedDataToBe:testHTMLData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 
     // Inserting text should delete the current selection, removing the attachment in the process.
     [webView expectUpdatesAfterCommand:@"InsertText" withArgument:@"foo" expectedRemovals:@[attachment.get()] expectedInsertions:@[]];
@@ -511,6 +520,8 @@ void platformCopyPNG()
     [webView expectUpdatesAfterCommand:@"InsertUnorderedList" withArgument:nil expectedRemovals:@[] expectedInsertions:@[]];
     [webView expectUpdatesAfterCommand:@"InsertUnorderedList" withArgument:nil expectedRemovals:@[] expectedInsertions:@[]];
     [attachment expectRequestedDataToBe:testHTMLData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 }
 
 TEST(WKAttachmentTests, AttachmentUpdatesWhenInsertingRichMarkup)
@@ -523,6 +534,8 @@ void platformCopyPNG()
         attachment = observer.observer().inserted[0];
         observer.expectAttachmentUpdates(@[ ], @[attachment.get()]);
     }
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
     {
         ObserveAttachmentUpdatesForScope observer(webView.get());
         [webView stringByEvaluatingJavaScript:@"document.querySelector('attachment').remove()"];
@@ -555,6 +568,8 @@ void platformCopyPNG()
         observer.expectAttachmentUpdates(@[], @[attachment.get()]);
     }
     [attachment expectRequestedDataToBe:testHTMLData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 }
 
 TEST(WKAttachmentTests, AttachmentDataForEmptyFile)