<rdar://problem/30664519>
Created attachment 302850 [details] Patch
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
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
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
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
Created attachment 302879 [details] Fix attachment dragging on Mac
Created attachment 302880 [details] Fix the Windows build
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.
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.
Created attachment 302976 [details] Patch for landing
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:?
(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.
(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.