After r222656, we consider images on the pasteboard to be files. This is causing DataTransfer.getData to return the empty string for all types, bringing back https://bugs.webkit.org/show_bug.cgi?id=170637.
It looks like the test added in r221343 (editing/pasteboard/drag-drop-href-as-text-data.html) didn't start failing after r222656 because the image being dragged hasn't been decoded yet, so image data isn't written to the pasteboard, which means that the change in r222656 to consider images as files doesn't affect this test. We'll also need to fix this test to correctly verify that HTTP and HTTPS URLs are exposed to the web, and not anything else.
<rdar://problem/34990050>
Created attachment 323797 [details] Patch
Comment on attachment 323797 [details] Patch Attachment 323797 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4854815 New failing tests: editing/pasteboard/paste-image-does-not-reveal-file-url.html editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html
Created attachment 323799 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 323797 [details] Patch Attachment 323797 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4854824 New failing tests: editing/pasteboard/paste-image-does-not-reveal-file-url.html editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html
Created attachment 323800 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 323797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323797&action=review > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7 > +link.href = https://www.webkit.org/ > +url: "https://www.webkit.org/" > +text/uri-list: "https://www.webkit.org/" > +link.href = file:///foo/bar/baz > +url: "" > +text/uri-list: "" Looks like we're not seeing the image file?
Comment on attachment 323797 [details] Patch Attachment 323797 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4854833 New failing tests: editing/pasteboard/paste-image-does-not-reveal-file-url.html editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html
Created attachment 323801 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 323797 [details] Patch Attachment 323797 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4854840 New failing tests: editing/pasteboard/data-transfer-item-list-add-file-multiple-times.html
Created attachment 323802 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
(In reply to Ryosuke Niwa from comment #8) > Comment on attachment 323797 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323797&action=review > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7 > > +link.href = https://www.webkit.org/ > > +url: "https://www.webkit.org/" > > +text/uri-list: "https://www.webkit.org/" > > +link.href = file:///foo/bar/baz > > +url: "" > > +text/uri-list: "" > > Looks like we're not seeing the image file? We're still seeing the image file, we're just not exposing any URL string in this case because it's a file URL. That makes me think — I should augment this test to additionally check that DataTransfer.files contains the image file we're dragging.
(In reply to Wenson Hsieh from comment #13) > (In reply to Ryosuke Niwa from comment #8) > > Comment on attachment 323797 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review > > > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7 > > > +link.href = https://www.webkit.org/ > > > +url: "https://www.webkit.org/" > > > +text/uri-list: "https://www.webkit.org/" > > > +link.href = file:///foo/bar/baz > > > +url: "" > > > +text/uri-list: "" > > > > Looks like we're not seeing the image file? > > We're still seeing the image file, we're just not exposing any URL string in > this case because it's a file URL. That makes me think — I should augment > this test to additionally check that DataTransfer.files contains the image > file we're dragging. Actually, this test already tries to do this! I'll need to investigate further...
(In reply to Wenson Hsieh from comment #14) > (In reply to Wenson Hsieh from comment #13) > > (In reply to Ryosuke Niwa from comment #8) > > > Comment on attachment 323797 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review > > > > > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7 > > > > +link.href = https://www.webkit.org/ > > > > +url: "https://www.webkit.org/" > > > > +text/uri-list: "https://www.webkit.org/" > > > > +link.href = file:///foo/bar/baz > > > > +url: "" > > > > +text/uri-list: "" > > > > > > Looks like we're not seeing the image file? > > > > We're still seeing the image file, we're just not exposing any URL string in > > this case because it's a file URL. That makes me think — I should augment > > this test to additionally check that DataTransfer.files contains the image > > file we're dragging. > > Actually, this test already tries to do this! I'll need to investigate > further... It's also worth mentioning this only reproduces in DumpRenderTree; the file is exposed correctly on the DataTransfer when manually testing drag-drop-href-as-url.html in Minibrowser (both WK1 and WK2) or Safari.
(In reply to Wenson Hsieh from comment #15) > (In reply to Wenson Hsieh from comment #14) > > (In reply to Wenson Hsieh from comment #13) > > > (In reply to Ryosuke Niwa from comment #8) > > > > Comment on attachment 323797 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review > > > > > > > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7 > > > > > +link.href = https://www.webkit.org/ > > > > > +url: "https://www.webkit.org/" > > > > > +text/uri-list: "https://www.webkit.org/" > > > > > +link.href = file:///foo/bar/baz > > > > > +url: "" > > > > > +text/uri-list: "" > > > > > > > > Looks like we're not seeing the image file? > > > > > > We're still seeing the image file, we're just not exposing any URL string in > > > this case because it's a file URL. That makes me think — I should augment > > > this test to additionally check that DataTransfer.files contains the image > > > file we're dragging. > > > > Actually, this test already tries to do this! I'll need to investigate > > further... > > It's also worth mentioning this only reproduces in DumpRenderTree; the file > is exposed correctly on the DataTransfer when manually testing > drag-drop-href-as-url.html in Minibrowser (both WK1 and WK2) or Safari. Ok, after some investigation: We're failing to expose the dragged image as a file in the layout test because we bail early due to one of the (old changeCount) != (new changeCount) security checks in PasteboardCocoa.mm. This is because we call -setData:forType: on drop in -[WebHTMLView pasteboard:provideDataForType:], which is a part of the codepath for supporting promises when dragging image elements. Calling -setData:forType: on drop here bumps the _changeCount of DumpRenderTree's LocalPasteboard in the middle of handling the drop, making us unable to retrieve any further data. This doesn't affect most tests, because we don't usually fall down this promised image data codepath in the other drag and drop paths. However, according to <https://developer.apple.com/documentation/appkit/nspasteboard/1533544-changecount>, NSPasteboard's changeCount only increments when the pasteboard's owner changes, not whenever pasteboard data is changed (e.g. via -setData:forType:). This is consistent with what happens in Safari/MiniBrowser when dragging an image element normally, which is why the test case works when testing manually. The fix here is to tweak DRT's LocalPasteboard to behave like NSPasteboard, and only increment the changeCount when the owner changes. After making this change, the layout test runs as expected (with files).
(In reply to Wenson Hsieh from comment #16) > (In reply to Wenson Hsieh from comment #15) > > (In reply to Wenson Hsieh from comment #14) > > > (In reply to Wenson Hsieh from comment #13) > > > > (In reply to Ryosuke Niwa from comment #8) > > > > > Comment on attachment 323797 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=323797&action=review > > > > > > > > > > > LayoutTests/editing/pasteboard/drag-drop-href-as-url-expected.txt:7 > > > > > > +link.href = https://www.webkit.org/ > > > > > > +url: "https://www.webkit.org/" > > > > > > +text/uri-list: "https://www.webkit.org/" > > > > > > +link.href = file:///foo/bar/baz > > > > > > +url: "" > > > > > > +text/uri-list: "" > > > > > > > > > > Looks like we're not seeing the image file? > > > > > > > > We're still seeing the image file, we're just not exposing any URL string in > > > > this case because it's a file URL. That makes me think — I should augment > > > > this test to additionally check that DataTransfer.files contains the image > > > > file we're dragging. > > > > > > Actually, this test already tries to do this! I'll need to investigate > > > further... > > > > It's also worth mentioning this only reproduces in DumpRenderTree; the file > > is exposed correctly on the DataTransfer when manually testing > > drag-drop-href-as-url.html in Minibrowser (both WK1 and WK2) or Safari. > > Ok, after some investigation: > > We're failing to expose the dragged image as a file in the layout test > because we bail early due to one of the (old changeCount) != (new > changeCount) security checks in PasteboardCocoa.mm. This is because we call > -setData:forType: on drop in -[WebHTMLView pasteboard:provideDataForType:], > which is a part of the codepath for supporting promises when dragging image > elements. Calling -setData:forType: on drop here bumps the _changeCount of > DumpRenderTree's LocalPasteboard in the middle of handling the drop, making > us unable to retrieve any further data. This doesn't affect most tests, > because we don't usually fall down this promised image data codepath in the > other drag and drop paths. > > However, according to > <https://developer.apple.com/documentation/appkit/nspasteboard/1533544- > changecount>, NSPasteboard's changeCount only increments when the > pasteboard's owner changes, not whenever pasteboard data is changed (e.g. > via -setData:forType:). This is consistent with what happens in > Safari/MiniBrowser when dragging an image element normally, which is why the > test case works when testing manually. > > The fix here is to tweak DRT's LocalPasteboard to behave like NSPasteboard, > and only increment the changeCount when the owner changes. After making this > change, the layout test runs as expected (with files). That makes sense. I've changed the call to provideDataForType in LocalPasteboard's declareTypes to getData at some point but I guess there's another code path in which it happens during setData.
Comment on attachment 323797 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323797&action=review > Source/WebCore/dom/DataTransfer.cpp:151 > + return url.protocolIs("http") || url.protocolIs("https"); URL provides the function protocolIsInHTTPFamily for this kind of check.
(In reply to Darin Adler from comment #18) > Comment on attachment 323797 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323797&action=review > > > Source/WebCore/dom/DataTransfer.cpp:151 > > + return url.protocolIs("http") || url.protocolIs("https"); > > URL provides the function protocolIsInHTTPFamily for this kind of check. Thanks! Changed to use protocolIsInHTTPFamily.
Created attachment 323842 [details] Patch
Comment on attachment 323842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323842&action=review > Source/WebCore/platform/Pasteboard.h:192 > static bool isSafeTypeForDOMToReadAndWrite(const String&); > + static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&); I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea. The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing. But despite those thoughts I don’t have new function names to propose at this time. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:128 > + RetainPtr<id> value = [m_pasteboard valuesForPasteboardType:type inItemSet:[NSIndexSet indexSetWithIndex:0]].firstObject; If we used the retainPtr function here: auto value = retainPtr(xxx); we might get a more specific type than RetainPtr<id>, which would be nice for passing to pasteboardMayContainFilePaths; the compiler doesn’t currently complain about passing id to a more specific type and maybe it never will, but still I think it might be better. > Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:137 > + if ([value isKindOfClass:[NSURL class]]) > + result = [(NSURL *)value absoluteString]; > + > + if ([value isKindOfClass:[NSAttributedString class]]) > + result = [(NSAttributedString *)value string]; > + > + if ([value isKindOfClass:[NSString class]]) > + result = (NSString *)value; Maybe chain these with else if? > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:92 > + if (pasteboardType == String(NSURLPboardType)) { Above you used the format String { kUTTypeURL }, but here it’s the old style. > LayoutTests/editing/pasteboard/files-during-page-drags.html:84 > + testFailed("This test is not interactive, please run using DumpRenderTree"); DumpRenderTree is the old one, WebKItTestRunner is the new one. If we are just mentioning one, than I suggest it be the new one.
Comment on attachment 323842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323842&action=review Thanks for the review! >> Source/WebCore/platform/Pasteboard.h:192 >> + static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&); > > I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea. > > The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing. > > But despite those thoughts I don’t have new function names to propose at this time. I see...then perhaps "bindings" would be a better term to describe this? We do use this in some other places in these same codepaths (e.g. typesSafeForBindings), so maybe it would be good to rename isSafeTypeForDOMToReadAndWrite and other places where we use "DOM" terminology to reference "bindings" instead. >> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:128 >> + RetainPtr<id> value = [m_pasteboard valuesForPasteboardType:type inItemSet:[NSIndexSet indexSetWithIndex:0]].firstObject; > > If we used the retainPtr function here: > > auto value = retainPtr(xxx); > > we might get a more specific type than RetainPtr<id>, which would be nice for passing to pasteboardMayContainFilePaths; the compiler doesn’t currently complain about passing id to a more specific type and maybe it never will, but still I think it might be better. Ok — done! >> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:137 >> + result = (NSString *)value; > > Maybe chain these with else if? Done. I suppose adding the else makes the intent here more explicit, though it will result in no change in behavior, since NSURL, NSAttributedString, and NSString are not subclasses of each other. >> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:92 >> + if (pasteboardType == String(NSURLPboardType)) { > > Above you used the format String { kUTTypeURL }, but here it’s the old style. Good catch! Changed to use { } initializer. >> LayoutTests/editing/pasteboard/files-during-page-drags.html:84 >> + testFailed("This test is not interactive, please run using DumpRenderTree"); > > DumpRenderTree is the old one, WebKItTestRunner is the new one. If we are just mentioning one, than I suggest it be the new one. Hm...this drag and drop test (files-during-page-drags.html) only runs in WebKit1 though, using DumpRenderTree, and not WK2, which uses WebKitTestRunner. So the description here would be accurate if it mentions DumpRenderTree instead of WebKitTestRunner.
Created attachment 323876 [details] Patch for EWS
Comment on attachment 323876 [details] Patch for EWS Clearing flags on attachment: 323876 Committed r223340: <https://trac.webkit.org/changeset/223340>
Comment on attachment 323842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323842&action=review >>> Source/WebCore/platform/Pasteboard.h:192 >>> + static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&); >> >> I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea. >> >> The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing. >> >> But despite those thoughts I don’t have new function names to propose at this time. > > I see...then perhaps "bindings" would be a better term to describe this? We do use this in some other places in these same codepaths (e.g. typesSafeForBindings), so maybe it would be good to rename isSafeTypeForDOMToReadAndWrite and other places where we use "DOM" terminology to reference "bindings" instead. Yes, I do like the idea of using the word "bindings" instead of the acronym "DOM" wherever it’s practical to make the replacement.
(In reply to Darin Adler from comment #25) > Comment on attachment 323842 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323842&action=review > > >>> Source/WebCore/platform/Pasteboard.h:192 > >>> + static bool canExposeURLToDOMWhenPasteboardContainsFiles(const String&); > >> > >> I don’t love the word "DOM" here to mean "what we expose to the web in JavaScript bindings" even though it is the term of art. Partly, just like URL, it just doesn’t flow well in these styles of function names since it’s an acronym. Don’t currently have a better idea. > >> > >> The older function uses the terminology "is safe", and the newer function use the terminology "can" to mean the same thing. > >> > >> But despite those thoughts I don’t have new function names to propose at this time. > > > > I see...then perhaps "bindings" would be a better term to describe this? We do use this in some other places in these same codepaths (e.g. typesSafeForBindings), so maybe it would be good to rename isSafeTypeForDOMToReadAndWrite and other places where we use "DOM" terminology to reference "bindings" instead. > > Yes, I do like the idea of using the word "bindings" instead of the acronym > "DOM" wherever it’s practical to make the replacement. Sounds good! I'll do this renaming in a followup. (An aside: this also reminds me that StaticPasteboard should be renamed to something sensible, now that it has taken on a lot more responsibility than it did upon introduction...)