Bug 175639 - Add the support for mutating clipboard data via DataTransferItemList
Summary: Add the support for mutating clipboard data via DataTransferItemList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 170449
  Show dependency treegraph
 
Reported: 2017-08-16 14:08 PDT by Ryosuke Niwa
Modified: 2017-08-16 23:26 PDT (History)
8 users (show)

See Also:


Attachments
Patch (27.01 KB, patch)
2017-08-16 14:34 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed builds (25.71 KB, patch)
2017-08-16 15:29 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-08-16 14:08:32 PDT
Add make dataTransfer.items.add, dataTransfer.items.clear, and dataTransfer.items.remove work.
Also update dataTransfer.items when mutating clipboard data via setData & clearData.
Comment 1 Ryosuke Niwa 2017-08-16 14:34:27 PDT
Created attachment 318291 [details]
Patch
Comment 2 Ryosuke Niwa 2017-08-16 15:29:29 PDT
Created attachment 318293 [details]
Fixed builds
Comment 3 Wenson Hsieh 2017-08-16 21:01:23 PDT
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!
Comment 4 Ryosuke Niwa 2017-08-16 22:45:37 PDT
(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).
Comment 5 Ryosuke Niwa 2017-08-16 23:17:11 PDT
Committed r220829: <http://trac.webkit.org/changeset/220829>
Comment 6 Radar WebKit Bug Importer 2017-08-16 23:17:47 PDT
<rdar://problem/33934646>
Comment 7 Wenson Hsieh 2017-08-16 23:26:55 PDT
(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).