When pasting an image, we shouldn't reveal its local file path in text/plain or text/uri-list. In fact, we should just hide all non-file types as we do for drag & drop.
<rdar://problem/34757924>
Created attachment 322305 [details] Fixes the bug
Comment on attachment 322305 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=322305&action=review > Source/WebCore/platform/StaticPasteboard.h:61 > + bool containsFiles() final { return 0; } I prefer `return false`, since the return type is a bool. > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163 > + // Enforce changeCount ourselves for security. We check after reading instead of before to be It seems this piece of code (and comment) keeps popping up in multiple places in this file :/ Now that we have a PasteboardCocoa.mm, it seems we should pull this out into a separate helper in this file?
(In reply to Wenson Hsieh from comment #3) > Comment on attachment 322305 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322305&action=review > > > Source/WebCore/platform/StaticPasteboard.h:61 > > + bool containsFiles() final { return 0; } > > I prefer `return false`, since the return type is a bool. Oops, I meant to use "false" there. > > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163 > > + // Enforce changeCount ourselves for security. We check after reading instead of before to be > > It seems this piece of code (and comment) keeps popping up in multiple > places in this file :/ Now that we have a PasteboardCocoa.mm, it seems we > should pull this out into a separate helper in this file? Perhaps. We probably need a function that takes lambda to do this.
Created attachment 322312 [details] Replaced 0 by false
Comment on attachment 322312 [details] Replaced 0 by false View in context: https://bugs.webkit.org/attachment.cgi?id=322312&action=review > Source/WebCore/dom/DataTransfer.cpp:178 > + if (m_pasteboard->containsFiles()) Heads up: with this change, we might need to rebaseline the API test DataInteractionTests.DataTransferGetDataWhenDroppingImageWithFileURL to remove the @"text/uri-list": @"" expectation.
(In reply to Wenson Hsieh from comment #6) > Comment on attachment 322312 [details] > Replaced 0 by false > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322312&action=review > > > Source/WebCore/dom/DataTransfer.cpp:178 > > + if (m_pasteboard->containsFiles()) > > Heads up: with this change, we might need to rebaseline the API test > DataInteractionTests.DataTransferGetDataWhenDroppingImageWithFileURL to > remove the @"text/uri-list": @"" expectation. Oh that's a good point. Let me check that...
Looks like there's a slight issue with PlatformPasteboard::numberOfFiles() in iOS. We need to replace numberOfFiles with droppedFileURLs's size instead. It claims to have a file even when it's just a text content and won't return a file URL.
(In reply to Ryosuke Niwa from comment #8) > Looks like there's a slight issue with PlatformPasteboard::numberOfFiles() > in iOS. We need to replace numberOfFiles with droppedFileURLs's size > instead. It claims to have a file even when it's just a text content and > won't return a file URL. Hmm...-droppedFileURLs will return 0 prior to the drop. If we just return -droppedFileURLs in PlatformPasteboard::numberOfFiles, I suspect that we would break uploading by dropping into inputs of type file.
Actually, I think I was misreading the test. DataTransferGetDataWhenDroppingRespectsPresentationStyle should return "Files" now because it's doing registerFileRepresentationForTypeIdentifier.
Rather, the actual observation is that there are two test cases in DataTransferGetDataWhenDroppingRespectsPresentationStyle, and the one with UIPreferredPresentationStyleAttachment needs to be rebaselined.
(In reply to Ryosuke Niwa from comment #11) > Rather, the actual observation is that there are two test cases in > DataTransferGetDataWhenDroppingRespectsPresentationStyle, and the one with > UIPreferredPresentationStyleAttachment needs to be rebaselined. Yes, that's correct. A bit more detail: this test involves two parts: (1) dropping an item provider containing text, with UIPreferredPresentationStyleAttachment set and (2) dropping an item provider containing text, with UIPreferredPresentationStyleInline set. The fact that -registerFileRepresentationForTypeIdentifier is used to supply the data is actually not important. The former simulates dropping a text file (e.g. from Files.app) while the latter involves dropping a text selection. I would expect "Files" to appear in the list of types in the former case, but not the latter.
Created attachment 322313 [details] Patch for landing
Comment on attachment 322313 [details] Patch for landing Clearing flags on attachment: 322313 Committed r222688: <http://trac.webkit.org/changeset/222688>
All reviewed patches have been landed. Closing bug.
API test DataInteractionTests.ExternalSourceDataTransferItemGetPlainTextFileAsEntry is failing on iOS after this change: /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:1063 Value of: [webView stringByEvaluatingJavaScript:@"output.value"] Actual: "Found data transfer item (kind: 'string', type: 'text/plain') Found data transfer item (kind: 'file', type: 'text/plain') FILE: /foo.txt ('text/plain', 28 bytes)" Expected: [expectedOutput componentsJoinedByString:@"\n"] Which is: "Found data transfer item (kind: 'file', type: 'text/plain') FILE: /foo.txt ('text/plain', 28 bytes)" https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/219
(In reply to Ryan Haddad from comment #16) > API test > DataInteractionTests.ExternalSourceDataTransferItemGetPlainTextFileAsEntry > is failing on iOS after this change: > > /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/ > ios/DataInteractionTests.mm:1063 > Value of: [webView stringByEvaluatingJavaScript:@"output.value"] > Actual: "Found data transfer item (kind: 'string', type: 'text/plain') > Found data transfer item (kind: 'file', type: 'text/plain') > FILE: /foo.txt ('text/plain', 28 bytes)" > Expected: [expectedOutput componentsJoinedByString:@"\n"] > Which is: "Found data transfer item (kind: 'file', type: 'text/plain') > FILE: /foo.txt ('text/plain', 28 bytes)" > > https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/219 I suspect this is because the custom pasteboard data is not enabled for these iOS 11 bots; we should ifdef this test out, like the other DataTransfer(Get|Set)Data tests in this file. We still have coverage on internal bots, and API tests are not run in EWS anyways.
The API tests have been skipped in https://trac.webkit.org/changeset/222743.