Bug 182557 – REGRESSION(r226396): File paths are inserted when dropping image files

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

<https://trac.webkit.org/r226396> unintentionally changed behavior when dropping files when attachment elements are disabled.

[Radar WebKit Bug Importer]

<rdar://problem/37294120>

[Wenson Hsieh]

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.

[Wenson Hsieh]

>             // 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 :/

[Wenson Hsieh]

Created attachment 333240 [details]
Fixes the bug

[Wenson Hsieh]

Created attachment 333247 [details]
Fixes the bug

[Wenson Hsieh]

Created attachment 333291 [details]
ChangeLog tweak

[Alexey Proskuryakov]

Given that the original way this was discovered was by pasting, not by dropping, I wonder if both paths need to be tested.

[Wenson Hsieh]

(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...

[Wenson Hsieh]

Created attachment 333305 [details]
Test copy/paste as well.

[Wenson Hsieh]

(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.

[Wenson Hsieh]

Comment on attachment 333305 [details]
Test copy/paste as well.

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 333305 [details]
Test copy/paste as well.

Clearing flags on attachment: 333305

Committed r228240: <https://trac.webkit.org/changeset/228240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.