REGRESSION(r226396): File paths are inserted when dropping image files
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Feb 2018 21:31:15 +0000 (21:31 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 7 Feb 2018 21:31:15 +0000 (21:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182557
<rdar://problem/37294120>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Reverts unintended changes in <http://trac.webkit.org/r226396>. Before r226396, WebContentReader::readFilenames
(a helper function in macOS-specific code) contained logic to create and insert attachment elements if
ENABLE(ATTACHMENT_ELEMENT); otherwise, it would fall back to inserting the visible URL as a text node. Since we
enable the attachment element on all Cocoa platforms via xcconfig files, this was effectively dead code.

However, when r226396 (which moved this out from macOS to Cocoa platform code) refactored this helper function,
it also moved this chunk of code out of the !ENABLE(ATTACHMENT) conditional and into a PLATFORM(MAC) guard,
which means that we now fall back to inserting file paths as text when attachment elements are disabled. To fix
this, we simply remove the (previously) dead code.

A more subtle difference is that we no longer always return true from WebContentReader::readFilePaths. This
means that when we drop files, we no longer skip over the early return in documentFragmentFromDragData when
we've made a fragment, so we read the file path as a URL. To address this, we just restore the pre-macOS 10.13.4
behavior of initializing the document fragment.

Test: modified editing/pasteboard/drag-files-to-editable-element-as-URLs.html.

* editing/WebContentReader.cpp:
(WebCore::WebContentReader::ensureFragment): Deleted.

Remove this helper, as it was only used in WebContentReader::readFilePaths.

* editing/WebContentReader.h:
* editing/cocoa/WebContentReaderCocoa.mm:
(WebCore::WebContentReader::readFilePaths):

Tools:

Tweak some image pasting API tests to ensure that file paths are not inserted when pasting images backed by
file paths on disk.

* TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:
(TEST):

LayoutTests:

Tweak an existing layout test that drops a file into a contenteditable, to check that no text is inserted into
the editable element after dropping.

* editing/pasteboard/drag-files-to-editable-element-as-URLs-expected.txt:
* editing/pasteboard/drag-files-to-editable-element-as-URLs.html:

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

LayoutTests/ChangeLog
LayoutTests/editing/pasteboard/drag-files-to-editable-element-as-URLs-expected.txt
LayoutTests/editing/pasteboard/drag-files-to-editable-element-as-URLs.html
Source/WebCore/ChangeLog
Source/WebCore/editing/WebContentReader.cpp
Source/WebCore/editing/WebContentReader.h
Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm

index 4a983d4..a3c2cac 100644 (file)
@@ -1,3 +1,17 @@
+2018-02-07  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION(r226396): File paths are inserted when dropping image files
+        https://bugs.webkit.org/show_bug.cgi?id=182557
+        <rdar://problem/37294120>
+
+        Reviewed by Ryosuke Niwa.
+
+        Tweak an existing layout test that drops a file into a contenteditable, to check that no text is inserted into
+        the editable element after dropping.
+
+        * editing/pasteboard/drag-files-to-editable-element-as-URLs-expected.txt:
+        * editing/pasteboard/drag-files-to-editable-element-as-URLs.html:
+
 2018-02-07  John Wilander  <wilander@apple.com>
 
         Restrict Referer to just the origin for third parties in private mode and third parties ITP blocks cookies for in regular mode
index 75824c1..4517341 100644 (file)
@@ -1,4 +1,4 @@
-If we drag files onto an editable area, then attachments should be inserted into the editable area.
+If we drag files onto an editable area, then attachments should not be inserted into the editable area since attachment elements are disabled.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
@@ -6,6 +6,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE
 PASS window.HTMLAttachmentElement is undefined.
 PASS document.createElement("attachment") instanceof HTMLUnknownElement is true
 PASS editable.querySelector("attachment") is null
+PASS editable.textContent is ""
 PASS successfullyParsed is true
 
 TEST COMPLETE
index b5d08e8..d5d0243 100644 (file)
@@ -6,7 +6,7 @@
 <div id="editable" contentEditable=true style="width:200px; height:200px"></div>
 <script src="../../resources/js-test-pre.js"></script>
 <script>
-description('If we drag files onto an editable area, then attachments should be inserted into the editable area.');
+description('If we drag files onto an editable area, then attachments should not be inserted into the editable area since attachment elements are disabled.');
 
 var editable = document.getElementById("editable");
 if (window.eventSender) {
@@ -14,6 +14,7 @@ if (window.eventSender) {
     shouldBeUndefined('window.HTMLAttachmentElement');
     shouldBeTrue('document.createElement("attachment") instanceof HTMLUnknownElement');
     shouldBe('editable.querySelector("attachment")', 'null');
+    shouldBe('editable.textContent', '""');
     editable.innerHTML = '';
 }
 
index 24fb3d5..8a7e96f 100644 (file)
@@ -1,3 +1,37 @@
+2018-02-07  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION(r226396): File paths are inserted when dropping image files
+        https://bugs.webkit.org/show_bug.cgi?id=182557
+        <rdar://problem/37294120>
+
+        Reviewed by Ryosuke Niwa.
+
+        Reverts unintended changes in <http://trac.webkit.org/r226396>. Before r226396, WebContentReader::readFilenames
+        (a helper function in macOS-specific code) contained logic to create and insert attachment elements if
+        ENABLE(ATTACHMENT_ELEMENT); otherwise, it would fall back to inserting the visible URL as a text node. Since we
+        enable the attachment element on all Cocoa platforms via xcconfig files, this was effectively dead code.
+
+        However, when r226396 (which moved this out from macOS to Cocoa platform code) refactored this helper function,
+        it also moved this chunk of code out of the !ENABLE(ATTACHMENT) conditional and into a PLATFORM(MAC) guard,
+        which means that we now fall back to inserting file paths as text when attachment elements are disabled. To fix
+        this, we simply remove the (previously) dead code.
+
+        A more subtle difference is that we no longer always return true from WebContentReader::readFilePaths. This
+        means that when we drop files, we no longer skip over the early return in documentFragmentFromDragData when
+        we've made a fragment, so we read the file path as a URL. To address this, we just restore the pre-macOS 10.13.4
+        behavior of initializing the document fragment.
+
+        Test: modified editing/pasteboard/drag-files-to-editable-element-as-URLs.html.
+
+        * editing/WebContentReader.cpp:
+        (WebCore::WebContentReader::ensureFragment): Deleted.
+
+        Remove this helper, as it was only used in WebContentReader::readFilePaths.
+
+        * editing/WebContentReader.h:
+        * editing/cocoa/WebContentReaderCocoa.mm:
+        (WebCore::WebContentReader::readFilePaths):
+
 2018-02-07  John Wilander  <wilander@apple.com>
 
         Restrict Referer to just the origin for third parties in private mode and third parties ITP blocks cookies for in regular mode
index 544912a..cf41936 100644 (file)
 
 namespace WebCore {
 
-DocumentFragment& WebContentReader::ensureFragment()
-{
-    ASSERT(frame.document());
-    if (!fragment)
-        fragment = frame.document()->createDocumentFragment();
-    return *fragment;
-}
-
 void WebContentReader::addFragment(Ref<DocumentFragment>&& newFragment)
 {
     if (!fragment)
index df984ba..488b39b 100644 (file)
@@ -63,7 +63,6 @@ public:
     {
     }
 
-    DocumentFragment& ensureFragment();
     void addFragment(Ref<DocumentFragment>&&);
 
 private:
index 3de71ba..1931497 100644 (file)
@@ -649,27 +649,20 @@ bool WebContentReader::readFilePaths(const Vector<String>& paths)
         return false;
 
     auto& document = *frame.document();
-    bool readAnyFilePath = false;
-    for (auto& path : paths) {
+    if (!fragment)
+        fragment = document.createDocumentFragment();
+
 #if ENABLE(ATTACHMENT_ELEMENT)
-        if (RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled()) {
+    if (RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled()) {
+        for (auto& path : paths) {
             auto attachment = HTMLAttachmentElement::create(HTMLNames::attachmentTag, document);
             attachment->setFile(File::create(path), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
-            ensureFragment().appendChild(attachment);
-            readAnyFilePath = true;
-            continue;
+            fragment->appendChild(attachment);
         }
-#endif
-#if PLATFORM(MAC)
-        // FIXME: Does (and should) any macOS client depend on inserting file paths as plain text in web content?
-        // If not, we should just remove this.
-        auto paragraph = createDefaultParagraphElement(document);
-        paragraph->appendChild(document.createTextNode(userVisibleString([NSURL fileURLWithPath:path])));
-        ensureFragment().appendChild(paragraph);
-        readAnyFilePath = true;
-#endif
     }
-    return readAnyFilePath;
+#endif
+
+    return true;
 }
 
 }
index a83a1a1..27a13b7 100644 (file)
@@ -1,3 +1,17 @@
+2018-02-07  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION(r226396): File paths are inserted when dropping image files
+        https://bugs.webkit.org/show_bug.cgi?id=182557
+        <rdar://problem/37294120>
+
+        Reviewed by Ryosuke Niwa.
+
+        Tweak some image pasting API tests to ensure that file paths are not inserted when pasting images backed by
+        file paths on disk.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:
+        (TEST):
+
 2018-02-07  Ms2ger  <Ms2ger@igalia.com>
 
         [GTK] Enable WebKit.GeolocationTransitionTo{High,Low}Accuracy tests
index e0e4b3b..b22896b 100644 (file)
@@ -140,6 +140,7 @@ TEST(PasteImage, PasteGIFFile)
     EXPECT_WK_STREQ("1", [webView stringByEvaluatingJavaScript:@"dataTransfer.files.length"]);
     EXPECT_WK_STREQ("image/gif", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
     EXPECT_WK_STREQ("sunset-in-cupertino-400px.gif", [webView stringByEvaluatingJavaScript:@"file.name"]);
+    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
 
     [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     [webView waitForMessage:@"loaded"];
@@ -161,6 +162,7 @@ TEST(PasteImage, PasteJPEGFile)
     EXPECT_WK_STREQ("1", [webView stringByEvaluatingJavaScript:@"dataTransfer.files.length"]);
     EXPECT_WK_STREQ("image/jpeg", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
     EXPECT_WK_STREQ("sunset-in-cupertino-600px.jpg", [webView stringByEvaluatingJavaScript:@"file.name"]);
+    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
 
     [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     [webView waitForMessage:@"loaded"];
@@ -182,6 +184,7 @@ TEST(PasteImage, PastePNGFile)
     EXPECT_WK_STREQ("1", [webView stringByEvaluatingJavaScript:@"dataTransfer.files.length"]);
     EXPECT_WK_STREQ("image/png", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
     EXPECT_WK_STREQ("sunset-in-cupertino-200px.png", [webView stringByEvaluatingJavaScript:@"file.name"]);
+    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
 
     [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     [webView waitForMessage:@"loaded"];
@@ -203,6 +206,7 @@ TEST(PasteImage, PasteTIFFFile)
     EXPECT_WK_STREQ("1", [webView stringByEvaluatingJavaScript:@"dataTransfer.files.length"]);
     EXPECT_WK_STREQ("image/tiff", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
     EXPECT_WK_STREQ("sunset-in-cupertino-100px.tiff", [webView stringByEvaluatingJavaScript:@"file.name"]);
+    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
 
     [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     [webView waitForMessage:@"loaded"];