Bug 175639

Created attachment 318291 [details] Patch

Created attachment 318293 [details] Fixed builds

Comment on attachment 318293 [details] Fixed builds View in context: https://bugs.webkit.org/attachment.cgi?id=318293&action=review This looks reasonable to be, but I'm a bit confused about one part. For instance, if you call items.add("WebKit1", "text/plain") and then items.add("WebKit2", "text/plain"), it is expected that the item list contains two items, and dataTransfer.getData("text/plain") would return "WebKit2" since the second add() call overwrote the first? Maybe we should include the case where we add more than one item at a time in the new test case. > Source/WebCore/ChangeLog:16 > + Because the identify and the order of DataTransferItem's need to be preserved, we can't simply I don't think there should be an apostrophe after DataTransferItem. > Source/WebCore/ChangeLog:17 > + re-popluate m_itemList in DataTransferItemList. Instead, whenever the clipboard content is mutated, Just to make sure -- it doesn't seem like this handles something modifying the NSPasteboard independently of the page, correct? > Source/WebCore/dom/DataTransferItem.cpp:81 > + bool isInDisabledMode = !m_list; IMO, this would look clearer if we folded the places where we check !m_list into a helper method on DataTransferItem called isDisabled() or isInDisabledMode(). > LayoutTests/editing/pasteboard/datatransfer-items-copy-plaintext.html:7 > +description('This tests copying plain text using dataTransfer.items. To manually test, click on "Copy text" and paste (Command+V on Mac Control+V elsewhere).'); As an aside: this patch should mean we also support adding plain text to the pasteboard when copying an image or link by tapping "Copy" on the share sheet in iOS. Let's remember to write a LayoutTest or API test for this!

(In reply to Wenson Hsieh from comment #3) > Comment on attachment 318293 [details] > Fixed builds > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318293&action=review > > This looks reasonable to be, but I'm a bit confused about one part. For > instance, if you call items.add("WebKit1", "text/plain") and then > items.add("WebKit2", "text/plain"), it is expected that the item list > contains two items, and dataTransfer.getData("text/plain") would return > "WebKit2" since the second add() call overwrote the first? Maybe we should > include the case where we add more than one item at a time in the new test > case. add() throws when there is an existing entry of the same type in the pasteboard. > > Source/WebCore/ChangeLog:16 > > + Because the identify and the order of DataTransferItem's need to be preserved, we can't simply > > I don't think there should be an apostrophe after DataTransferItem. Removed. > > Source/WebCore/ChangeLog:17 > > + re-popluate m_itemList in DataTransferItemList. Instead, whenever the clipboard content is mutated, > > Just to make sure -- it doesn't seem like this handles something modifying > the NSPasteboard independently of the page, correct? It doesn't. We should be more vigilant about checking changeCount everywhere. Once we implement the fake pasteboard we talked about in the person, this problem will go away though. > > > Source/WebCore/dom/DataTransferItem.cpp:81 > > + bool isInDisabledMode = !m_list; > > IMO, this would look clearer if we folded the places where we check !m_list > into a helper method on DataTransferItem called isDisabled() or > isInDisabledMode(). I did that earlier, and got rid of it after I realized it's better to check m_list elsewhere to make it clear m_list can't be null. I just added it back to be used in just this one place. > > LayoutTests/editing/pasteboard/datatransfer-items-copy-plaintext.html:7 > > +description('This tests copying plain text using dataTransfer.items. To manually test, click on "Copy text" and paste (Command+V on Mac Control+V elsewhere).'); > > As an aside: this patch should mean we also support adding plain text to the > pasteboard when copying an image or link by tapping "Copy" on the share > sheet in iOS. Let's remember to write a LayoutTest or API test for this! Really? We fire copy event on those case? We don't do that on macOS (neither does Firefox or Chrome).

Committed r220829: <http://trac.webkit.org/changeset/220829>

<rdar://problem/33934646>

(In reply to Ryosuke Niwa from comment #4) > (In reply to Wenson Hsieh from comment #3) > > Comment on attachment 318293 [details] > > Fixed builds > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=318293&action=review > > > > This looks reasonable to be, but I'm a bit confused about one part. For > > instance, if you call items.add("WebKit1", "text/plain") and then > > items.add("WebKit2", "text/plain"), it is expected that the item list > > contains two items, and dataTransfer.getData("text/plain") would return > > "WebKit2" since the second add() call overwrote the first? Maybe we should > > include the case where we add more than one item at a time in the new test > > case. > > add() throws when there is an existing entry of the same type in the > pasteboard. > > > > Source/WebCore/ChangeLog:16 > > > + Because the identify and the order of DataTransferItem's need to be preserved, we can't simply > > > > I don't think there should be an apostrophe after DataTransferItem. > > Removed. > > > > Source/WebCore/ChangeLog:17 > > > + re-popluate m_itemList in DataTransferItemList. Instead, whenever the clipboard content is mutated, > > > > Just to make sure -- it doesn't seem like this handles something modifying > > the NSPasteboard independently of the page, correct? > > It doesn't. We should be more vigilant about checking changeCount > everywhere. Once we implement the fake pasteboard we talked about in the > person, this problem will go away though. > > > > > > Source/WebCore/dom/DataTransferItem.cpp:81 > > > + bool isInDisabledMode = !m_list; > > > > IMO, this would look clearer if we folded the places where we check !m_list > > into a helper method on DataTransferItem called isDisabled() or > > isInDisabledMode(). > > I did that earlier, and got rid of it after I realized it's better to check > m_list elsewhere to make it clear m_list can't be null. I just added it back > to be used in just this one place. > > > > LayoutTests/editing/pasteboard/datatransfer-items-copy-plaintext.html:7 > > > +description('This tests copying plain text using dataTransfer.items. To manually test, click on "Copy text" and paste (Command+V on Mac Control+V elsewhere).'); > > > > As an aside: this patch should mean we also support adding plain text to the > > pasteboard when copying an image or link by tapping "Copy" on the share > > sheet in iOS. Let's remember to write a LayoutTest or API test for this! > > Really? We fire copy event on those case? We don't do that on macOS (neither > does Firefox or Chrome). Oh, interesting...that's correct, a quick test page like: <img id="image" src="icon.png"></img> <script>image.addEventListener("copy", e => console.log(e))</script> ...shows that both Mac and iOS don't fire the `copy` event when using context menu/share sheet, respectively. In that case, this only applies to iOS DnD, when starting a drag. I can add a test for this when working towards <rdar://problem/30291638> (iOS DnD parity with Mac).