Changeset 226097 in webkit

Timestamp:

Dec 18, 2017 9:38:47 PM (6 years ago)

Author:

Wenson Hsieh

Message:

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

Location:

trunk

Files:

• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/markup.cpp (modified) (3 diffs)
• Source/WebCore/html/HTMLAttachmentElement.cpp (modified) (1 diff)
• Source/WebCore/rendering/HitTestResult.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm (modified) (6 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-12-18  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [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  <cdumez@apple.com>
 

trunk/Source/WebCore/editing/markup.cpp

@@ -372 +372 @@
 }
 
-void StyledMarkupAccumulator::appendCustomAttributes(StringBuilder& out, const Element&element, Namespaces* namespaces)
+void StyledMarkupAccumulator::appendCustomAttributes(StringBuilder& out, const Element& element, Namespaces* namespaces)
 {
 #if ENABLE(ATTACHMENT_ELEMENT)
@@ -379 +379 @@
     
     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);
@@ -765 +769 @@
     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

trunk/Source/WebCore/html/HTMLAttachmentElement.cpp

@@ -121 +121 @@
     m_file = WTFMove(file);
 
-    setAttributeWithoutSynchronization(HTMLNames::webkitattachmentbloburlAttr, m_file ? m_file->url() : emptyString());
-
     if (updateAttributes == UpdateDisplayAttributes::Yes) {
         if (m_file) {

trunk/Source/WebCore/rendering/HitTestResult.cpp

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

trunk/Tools/ChangeLog

@@ +1 @@
+2017-12-18  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [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  <wenson_hsieh@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm

@@ -225 +225 @@
 }
 
+- (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
 {
@@ -426 +431 @@
     [firstAttachment expectRequestedDataToBe:nil];
     [secondAttachment expectRequestedDataToBe:testImageData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 }
 
@@ -491 +498 @@
     [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.
@@ -512 +521 @@
     [webView expectUpdatesAfterCommand:@"InsertUnorderedList" withArgument:nil expectedRemovals:@[] expectedInsertions:@[]];
     [attachment expectRequestedDataToBe:testHTMLData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 }
 
@@ -524 +535 @@
         observer.expectAttachmentUpdates(@[ ], @[attachment.get()]);
     }
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
     {
         ObserveAttachmentUpdatesForScope observer(webView.get());
@@ -556 +569 @@
     }
     [attachment expectRequestedDataToBe:testHTMLData()];
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentbloburl" forQuerySelector:@"attachment"]);
+    EXPECT_FALSE([webView hasAttribute:@"webkitattachmentpath" forQuerySelector:@"attachment"]);
 }