Summary: | REGRESSION(r226396): File paths are inserted when dropping image files | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, bdakin, commit-queue, rniwa, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2018-02-06 16:30:33 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. > // 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 :/
Created attachment 333240 [details]
Fixes the bug
Created attachment 333247 [details]
Fixes the bug
Created attachment 333291 [details]
ChangeLog tweak
Given that the original way this was discovered was by pasting, not by dropping, I wonder if both paths need to be tested. (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... Created attachment 333305 [details]
Test copy/paste as well.
(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 on attachment 333305 [details]
Test copy/paste as well.
Thanks for the review!
Comment on attachment 333305 [details] Test copy/paste as well. Clearing flags on attachment: 333305 Committed r228240: <https://trac.webkit.org/changeset/228240> All reviewed patches have been landed. Closing bug. |