Bug 182557 - REGRESSION(r226396): File paths are inserted when dropping image files
Summary: REGRESSION(r226396): File paths are inserted when dropping image files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-06 16:30 PST by Wenson Hsieh
Modified: 2018-02-07 13:31 PST (History)
7 users (show)

See Also:


Attachments
Fixes the bug (8.34 KB, patch)
2018-02-06 18:06 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fixes the bug (8.31 KB, patch)
2018-02-06 18:21 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
ChangeLog tweak (8.32 KB, patch)
2018-02-07 09:09 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Test copy/paste as well. (11.79 KB, patch)
2018-02-07 11:07 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-02-06 16:30:33 PST
<https://trac.webkit.org/r226396> unintentionally changed behavior when dropping files when attachment elements are disabled.
Comment 1 Radar WebKit Bug Importer 2018-02-06 16:33:55 PST
<rdar://problem/37294120>
Comment 2 Wenson Hsieh 2018-02-06 16:45:50 PST
This function used to be:

    bool Editor::WebContentReader::readFilenames(const Vector<String>& paths)
    {
        if (paths.isEmpty())
            return false;
        if (!frame.document())
            return false;
        Document& document = *frame.document();
        fragment = document.createDocumentFragment();
        for (auto& text : paths) {
    #if ENABLE(ATTACHMENT_ELEMENT)
            auto attachment = HTMLAttachmentElement::create(attachmentTag, document);
            attachment->setFile(File::create([[NSURL fileURLWithPath:text] path]).ptr());
            fragment->appendChild(attachment);
    #else
            auto paragraph = createDefaultParagraphElement(document);
            paragraph->appendChild(document.createTextNode(frame.editor().client()->userVisibleString([NSURL fileURLWithPath:text])));
            fragment->appendChild(paragraph);
    #endif
        }
        return true;
    }

...and it is now:

    bool WebContentReader::readFilePaths(const Vector<String>& paths)
    {
        if (paths.isEmpty() || !frame.document())
            return false;

        auto& document = *frame.document();
        bool readAnyFilePath = false;
        for (auto& path : paths) {
    #if ENABLE(ATTACHMENT_ELEMENT)
            if (RuntimeEnabledFeatures::sharedFeatures().attachmentElementEnabled()) {
                auto attachment = HTMLAttachmentElement::create(HTMLNames::attachmentTag, document);
                attachment->setFile(File::create(path), HTMLAttachmentElement::UpdateDisplayAttributes::Yes);
                ensureFragment().appendChild(attachment);
                readAnyFilePath = true;
                continue;
            }
    #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;
    }

...so I had unintentionally resurrected the dead codepath to append the visible URL as a text node. There's also a more subtle difference here, which is that we used to always return true and initialize the document fragment here in this function, but now we only do so if we actually appended anything to the document fragment.

A fix here is to (1) just remove the dead codepath, since there's no version of macOS where !ENABLE(ATTACHMENT_ELEMENT), and (2) revert to the old behavior where we always create the document fragment after calling this function.
Comment 3 Wenson Hsieh 2018-02-06 16:48:35 PST
>             // 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.

Ironically, I had even considered just removing this code in that same patch, but decided not to so that there'd be less chance of breakage. I failed to see that this was dead code to begin with :/
Comment 4 Wenson Hsieh 2018-02-06 18:06:52 PST
Created attachment 333240 [details]
Fixes the bug
Comment 5 Wenson Hsieh 2018-02-06 18:21:00 PST
Created attachment 333247 [details]
Fixes the bug
Comment 6 Wenson Hsieh 2018-02-07 09:09:38 PST
Created attachment 333291 [details]
ChangeLog tweak
Comment 7 Alexey Proskuryakov 2018-02-07 09:30:39 PST
Given that the original way this was discovered was by pasting, not by dropping, I wonder if both paths need to be tested.
Comment 8 Wenson Hsieh 2018-02-07 09:38:31 PST
(In reply to Alexey Proskuryakov from comment #7)
> Given that the original way this was discovered was by pasting, not by
> dropping, I wonder if both paths need to be tested.

Given that the bug lies in WebContentReader code common to both dropping and pasting, I don't think we need a separate test that uses paste to trigger this bug...
Comment 9 Wenson Hsieh 2018-02-07 11:07:28 PST
Created attachment 333305 [details]
Test copy/paste as well.
Comment 10 Wenson Hsieh 2018-02-07 11:08:49 PST
(In reply to Wenson Hsieh from comment #8)
> (In reply to Alexey Proskuryakov from comment #7)
> > Given that the original way this was discovered was by pasting, not by
> > dropping, I wonder if both paths need to be tested.
> 
> Given that the bug lies in WebContentReader code common to both dropping and
> pasting, I don't think we need a separate test that uses paste to trigger
> this bug...

Well, then again, no harm in more test coverage :)

We can adjust some existing API tests to exercise copy/paste.
Comment 11 Wenson Hsieh 2018-02-07 13:07:15 PST
Comment on attachment 333305 [details]
Test copy/paste as well.

Thanks for the review!
Comment 12 WebKit Commit Bot 2018-02-07 13:31:20 PST
Comment on attachment 333305 [details]
Test copy/paste as well.

Clearing flags on attachment: 333305

Committed r228240: <https://trac.webkit.org/changeset/228240>
Comment 13 WebKit Commit Bot 2018-02-07 13:31:21 PST
All reviewed patches have been landed.  Closing bug.