Bug 168916 – [WK2] Data interaction should support attachment elements

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

<rdar://problem/30664519>

[Wenson Hsieh]

Created attachment 302850 [details]
Patch

[Build Bot]

Comment on attachment 302850 [details]
Patch

Attachment 302850 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3203868

New failing tests:
editing/pasteboard/drag-and-drop-attachment-contenteditable.html

[Build Bot]

Created attachment 302860 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 302850 [details]
Patch

Attachment 302850 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3203876

New failing tests:
editing/pasteboard/drag-and-drop-attachment-contenteditable.html

[Build Bot]

Created attachment 302861 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Wenson Hsieh]

Created attachment 302879 [details]
Fix attachment dragging on Mac

[Wenson Hsieh]

Created attachment 302880 [details]
Fix the Windows build

[Ryosuke Niwa]

Comment on attachment 302880 [details]
Fix the Windows build

View in context: https://bugs.webkit.org/attachment.cgi?id=302880&action=review

> Source/WebCore/page/DragController.cpp:660
> +        // The user is attempting to initiate a drag from an attachment element.

I don't think this comment adds anything to what's already conveyed by the code. Please remove.

> Source/WebCore/page/DragController.cpp:662
> +        bool isSingleAttachmentSelection = selection.isRange() && selection.start().anchorNode() == startElement && selection.end().anchorNode() == startElement;

I know you're just moving the code but this check doesn't make any sense.
A position could be before or after the attachment element.
We should be checking that the selection starts at the beginning of the attachment and ends at the end instead.

> Source/WebCore/page/DragController.cpp:669
> +        // If the attachment element is the only thing selected, or the selection does not encompass the attachment,
> +        // use single element drag behavior.

Again, I don't think this comment adds anything beyond what's written in the code.

[Wenson Hsieh]

Comment on attachment 302880 [details]
Fix the Windows build

View in context: https://bugs.webkit.org/attachment.cgi?id=302880&action=review

Thanks Ryosuke!

>> Source/WebCore/page/DragController.cpp:660
>> +        // The user is attempting to initiate a drag from an attachment element.
> 
> I don't think this comment adds anything to what's already conveyed by the code. Please remove.

Removed.

>> Source/WebCore/page/DragController.cpp:662
>> +        bool isSingleAttachmentSelection = selection.isRange() && selection.start().anchorNode() == startElement && selection.end().anchorNode() == startElement;
> 
> I know you're just moving the code but this check doesn't make any sense.
> A position could be before or after the attachment element.
> We should be checking that the selection starts at the beginning of the attachment and ends at the end instead.

Good point -- I'll change this to compare the actual start/end positions of the range against the attachment element.

>> Source/WebCore/page/DragController.cpp:669
>> +        // use single element drag behavior.
> 
> Again, I don't think this comment adds anything beyond what's written in the code.

Removed.

[Wenson Hsieh]

Created attachment 302976 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 302976 [details]
Patch for landing

Clearing flags on attachment: 302976

Committed r213176: <http://trac.webkit.org/changeset/213176>

Comment on attachment 302976 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=302976&action=review

> Source/WebCore/page/DragController.cpp:1019
> +                // Otherwise, if no file URL is specified, call out to the injected bundle to populate the pasteboard with data.

It’s strange to make references to “the injected bundle” in WebCore code. Doesn’t this code also run in WebKit1 where it calls -webView:didWriteSelectionToPasteboard:?

[Wenson Hsieh]

(In reply to comment #12)
> Comment on attachment 302976 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302976&action=review
> 
> > Source/WebCore/page/DragController.cpp:1019
> > +                // Otherwise, if no file URL is specified, call out to the injected bundle to populate the pasteboard with data.
> 
> It’s strange to make references to “the injected bundle” in WebCore code.
> Doesn’t this code also run in WebKit1 where it calls
> -webView:didWriteSelectionToPasteboard:?

Unfortunately, it seems the webView:(will|did)WriteSelectionToPasteboard: delegate hooks are not implemented in WK1 on iOS. I agree though, calling out the injected bundle in WebCore is strange, seeing as it's only a concept on iOS. The comment should really say "call out to the client layer" or something similar.

[Wenson Hsieh]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 302976 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=302976&action=review
> > 
> > > Source/WebCore/page/DragController.cpp:1019
> > > +                // Otherwise, if no file URL is specified, call out to the injected bundle to populate the pasteboard with data.
> > 
> > It’s strange to make references to “the injected bundle” in WebCore code.
> > Doesn’t this code also run in WebKit1 where it calls
> > -webView:didWriteSelectionToPasteboard:?
> 
> Unfortunately, it seems the webView:(will|did)WriteSelectionToPasteboard:
> delegate hooks are not implemented in WK1 on iOS. I agree though, calling
> out the injected bundle in WebCore is strange, seeing as it's only a concept
> on iOS. The comment should really say "call out to the client layer" or

*on WK2, not just on iOS

> something similar.

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Comment on attachment 302976 [details]
> > > Patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=302976&action=review
> > > 
> > > > Source/WebCore/page/DragController.cpp:1019
> > > > +                // Otherwise, if no file URL is specified, call out to the injected bundle to populate the pasteboard with data.
> > > 
> > > It’s strange to make references to “the injected bundle” in WebCore code.
> > > Doesn’t this code also run in WebKit1 where it calls
> > > -webView:didWriteSelectionToPasteboard:?
> > 
> > Unfortunately, it seems the webView:(will|did)WriteSelectionToPasteboard:
> > delegate hooks are not implemented in WK1 on iOS. I agree though, calling
> > out the injected bundle in WebCore is strange, seeing as it's only a concept
> > on iOS. The comment should really say "call out to the client layer" or
> 
> *on WK2, not just on iOS

Dunno, I see at least -webView:didWriteSelectionToPasteboard: in WebKit1. I am also not entirely clear on why any of this code is behind PLATFORM(COCOA) guards. The editor client may be used on other platforms.