Changeset 228240 in webkit


Ignore:
Timestamp:
Feb 7, 2018 1:31:15 PM (6 years ago)
Author:
Wenson Hsieh
Message:

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.

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:
Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r228239 r228240  
     12018-02-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        REGRESSION(r226396): File paths are inserted when dropping image files
     4        https://bugs.webkit.org/show_bug.cgi?id=182557
     5        <rdar://problem/37294120>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        Tweak an existing layout test that drops a file into a contenteditable, to check that no text is inserted into
     10        the editable element after dropping.
     11
     12        * editing/pasteboard/drag-files-to-editable-element-as-URLs-expected.txt:
     13        * editing/pasteboard/drag-files-to-editable-element-as-URLs.html:
     14
    1152018-02-07  John Wilander  <wilander@apple.com>
    216
  • trunk/LayoutTests/editing/pasteboard/drag-files-to-editable-element-as-URLs-expected.txt

    r221944 r228240  
    1 If we drag files onto an editable area, then attachments should be inserted into the editable area.
     1If we drag files onto an editable area, then attachments should not be inserted into the editable area since attachment elements are disabled.
    22
    33On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
     
    77PASS document.createElement("attachment") instanceof HTMLUnknownElement is true
    88PASS editable.querySelector("attachment") is null
     9PASS editable.textContent is ""
    910PASS successfullyParsed is true
    1011
  • trunk/LayoutTests/editing/pasteboard/drag-files-to-editable-element-as-URLs.html

    r221944 r228240  
    77<script src="../../resources/js-test-pre.js"></script>
    88<script>
    9 description('If we drag files onto an editable area, then attachments should be inserted into the editable area.');
     9description('If we drag files onto an editable area, then attachments should not be inserted into the editable area since attachment elements are disabled.');
    1010
    1111var editable = document.getElementById("editable");
     
    1515    shouldBeTrue('document.createElement("attachment") instanceof HTMLUnknownElement');
    1616    shouldBe('editable.querySelector("attachment")', 'null');
     17    shouldBe('editable.textContent', '""');
    1718    editable.innerHTML = '';
    1819}
  • trunk/Source/WebCore/ChangeLog

    r228239 r228240  
     12018-02-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        REGRESSION(r226396): File paths are inserted when dropping image files
     4        https://bugs.webkit.org/show_bug.cgi?id=182557
     5        <rdar://problem/37294120>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        Reverts unintended changes in <http://trac.webkit.org/r226396>. Before r226396, WebContentReader::readFilenames
     10        (a helper function in macOS-specific code) contained logic to create and insert attachment elements if
     11        ENABLE(ATTACHMENT_ELEMENT); otherwise, it would fall back to inserting the visible URL as a text node. Since we
     12        enable the attachment element on all Cocoa platforms via xcconfig files, this was effectively dead code.
     13
     14        However, when r226396 (which moved this out from macOS to Cocoa platform code) refactored this helper function,
     15        it also moved this chunk of code out of the !ENABLE(ATTACHMENT) conditional and into a PLATFORM(MAC) guard,
     16        which means that we now fall back to inserting file paths as text when attachment elements are disabled. To fix
     17        this, we simply remove the (previously) dead code.
     18
     19        A more subtle difference is that we no longer always return true from WebContentReader::readFilePaths. This
     20        means that when we drop files, we no longer skip over the early return in documentFragmentFromDragData when
     21        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
     22        behavior of initializing the document fragment.
     23
     24        Test: modified editing/pasteboard/drag-files-to-editable-element-as-URLs.html.
     25
     26        * editing/WebContentReader.cpp:
     27        (WebCore::WebContentReader::ensureFragment): Deleted.
     28
     29        Remove this helper, as it was only used in WebContentReader::readFilePaths.
     30
     31        * editing/WebContentReader.h:
     32        * editing/cocoa/WebContentReaderCocoa.mm:
     33        (WebCore::WebContentReader::readFilePaths):
     34
    1352018-02-07  John Wilander  <wilander@apple.com>
    236
  • trunk/Source/WebCore/editing/WebContentReader.cpp

    r226396 r228240  
    3232namespace WebCore {
    3333
    34 DocumentFragment& WebContentReader::ensureFragment()
    35 {
    36     ASSERT(frame.document());
    37     if (!fragment)
    38         fragment = frame.document()->createDocumentFragment();
    39     return *fragment;
    40 }
    41 
    4234void WebContentReader::addFragment(Ref<DocumentFragment>&& newFragment)
    4335{
  • trunk/Source/WebCore/editing/WebContentReader.h

    r226396 r228240  
    6464    }
    6565
    66     DocumentFragment& ensureFragment();
    6766    void addFragment(Ref<DocumentFragment>&&);
    6867
  • trunk/Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm

    r227351 r228240  
    650650
    651651    auto& document = *frame.document();
    652     bool readAnyFilePath = false;
    653     for (auto& path : paths) {
     652    if (!fragment)
     653        fragment = document.createDocumentFragment();
     654
    654655#if ENABLE(ATTACHMENT_ELEMENT)
    655         if (RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled()) {
     656    if (RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled()) {
     657        for (auto& path : paths) {
    656658            auto attachment = HTMLAttachmentElement::create(HTMLNames::attachmentTag, document);
    657659            attachment->setFile(File::create(path), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
    658             ensureFragment().appendChild(attachment);
    659             readAnyFilePath = true;
    660             continue;
    661         }
    662 #endif
    663 #if PLATFORM(MAC)
    664         // FIXME: Does (and should) any macOS client depend on inserting file paths as plain text in web content?
    665         // If not, we should just remove this.
    666         auto paragraph = createDefaultParagraphElement(document);
    667         paragraph->appendChild(document.createTextNode(userVisibleString([NSURL fileURLWithPath:path])));
    668         ensureFragment().appendChild(paragraph);
    669         readAnyFilePath = true;
    670 #endif
    671     }
    672     return readAnyFilePath;
    673 }
    674 
    675 }
     660            fragment->appendChild(attachment);
     661        }
     662    }
     663#endif
     664
     665    return true;
     666}
     667
     668}
  • trunk/Tools/ChangeLog

    r228225 r228240  
     12018-02-07  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        REGRESSION(r226396): File paths are inserted when dropping image files
     4        https://bugs.webkit.org/show_bug.cgi?id=182557
     5        <rdar://problem/37294120>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        Tweak some image pasting API tests to ensure that file paths are not inserted when pasting images backed by
     10        file paths on disk.
     11
     12        * TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm:
     13        (TEST):
     14
    1152018-02-07  Ms2ger  <Ms2ger@igalia.com>
    216
  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteImage.mm

    r226173 r228240  
    141141    EXPECT_WK_STREQ("image/gif", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
    142142    EXPECT_WK_STREQ("sunset-in-cupertino-400px.gif", [webView stringByEvaluatingJavaScript:@"file.name"]);
     143    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
    143144
    144145    [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     
    162163    EXPECT_WK_STREQ("image/jpeg", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
    163164    EXPECT_WK_STREQ("sunset-in-cupertino-600px.jpg", [webView stringByEvaluatingJavaScript:@"file.name"]);
     165    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
    164166
    165167    [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     
    183185    EXPECT_WK_STREQ("image/png", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
    184186    EXPECT_WK_STREQ("sunset-in-cupertino-200px.png", [webView stringByEvaluatingJavaScript:@"file.name"]);
     187    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
    185188
    186189    [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
     
    204207    EXPECT_WK_STREQ("image/tiff", [webView stringByEvaluatingJavaScript:@"file = dataTransfer.files[0]; file.type"]);
    205208    EXPECT_WK_STREQ("sunset-in-cupertino-100px.tiff", [webView stringByEvaluatingJavaScript:@"file.name"]);
     209    EXPECT_WK_STREQ("", [webView stringByEvaluatingJavaScript:@"editor.textContent"]);
    206210
    207211    [webView stringByEvaluatingJavaScript:@"insertFileAsImage(file)"];
Note: See TracChangeset for help on using the changeset viewer.