Bug 79443

<rdar://problem/10925082>

Could you diagnose what's going wrong? It's hard for me to debug this bug given I don't have an iCloud account. There's also https://bugs.webkit.org/show_bug.cgi?id=79172. Does Daniel's patch there fix this bug as well?

I'm taking a look at this now. I'm working on creating a test case, but a quick debugging session showed me that m_dragDestinationAction & DragDestinationActionDHTML is true in DragController::performDrag() when m_isHandlingDrag is false. This means that the code block bound by if (m_isHandlingDrag) prior to r105396 is now being executed, and that's causing this particular behavior difference.

Created attachment 130013 [details] Test case Sam and I looked at this and created a test case. The issue is that iCloud is changing the drop target from display:block to display:none in its drop event handler. When concludeEditDrag() hit tests for the drop target later, it gets a different element in return. This seems like a WebKit bug that we would dispatch an event to one element but then call concludeEditDrag() on another. We should use the same element. Note that Firefox handles this case correctly.

I should point out that r105396 isn't really to blame here; it just exposed a long-standing bug. As far as I can tell, DragController::concludeEditDrag() has determined the drag target by hit testing since the beginning of time. I'll leave 'REGRESSION(...' in the title since it's helpful to know when the behavior change started to occur.

Created attachment 130674 [details] Patch

Attachment 130674 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 130674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130674&action=review >> LayoutTests/ChangeLog:11 >> + EventSender.beginDragWithFiles(). > > Line contains tab character. [whitespace/tab] [5] Will fix before landing.

Comment on attachment 130674 [details] Patch Attachment 130674 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11854336 New failing tests: fast/forms/drag-out-of-textarea.html editing/pasteboard/drop-text-events.html editing/pasteboard/drag-drop-input-textarea.html editing/selection/drag-text-delay.html fast/events/5056619.html fast/forms/drag-into-textarea.html editing/pasteboard/drag-drop-url-text.html fast/events/content-changed-during-drop.html

(In reply to comment #9) > (From update of attachment 130674 [details]) > Attachment 130674 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11854336 > > New failing tests: > fast/forms/drag-out-of-textarea.html > editing/pasteboard/drop-text-events.html > editing/pasteboard/drag-drop-input-textarea.html > editing/selection/drag-text-delay.html > fast/events/5056619.html > fast/forms/drag-into-textarea.html > editing/pasteboard/drag-drop-url-text.html > fast/events/content-changed-during-drop.html Seems likely that I broke these. I'll take a look.

It's sad that http://queues.webkit.org/results/11854336 make no mention of the test failures listed in comment #9.

I think http://queues.webkit.org/results/11850353 is the link the review bot meant to include.

Created attachment 130769 [details] Patch I got a little carried away with that last patch. This version is much more to the point.

Comment on attachment 130769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130769&action=review > Source/WebCore/page/DragController.cpp:226 > + if (m_fileInputElementUnderMouse) { > + m_fileInputElementUnderMouse->setCanReceiveDroppedFiles(false); > + m_fileInputElementUnderMouse = 0; > } Could you explain why we need to always do this? > Source/WebCore/page/DragController.cpp:467 > + // m_fileInputElementUnderMouse should be the element we hit tested for, > + // unless it was made display:none by a drop event handler. > + ASSERT(m_fileInputElementUnderMouse == element || !m_fileInputElementUnderMouse->renderer()); I don't think this assertion is accurate. You can also move the element to elsewhere in the document or to a different frame, right? > Source/WebCore/page/DragController.cpp:558 > + // m_fileInputElementUnderMouse should be the element we hit tested > + // for, unless it was made display:none by a drop event handler. > + ASSERT(m_fileInputElementUnderMouse == result.innerNonSharedNode() || !m_fileInputElementUnderMouse->renderer()); Ditto.

(In reply to comment #14) > (From update of attachment 130769 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130769&action=review > > > Source/WebCore/page/DragController.cpp:226 > > + if (m_fileInputElementUnderMouse) { > > + m_fileInputElementUnderMouse->setCanReceiveDroppedFiles(false); > > + m_fileInputElementUnderMouse = 0; > > } > > Could you explain why we need to always do this? This was moved out of concludeEditDrag() in order to keep m_fileInputElementUnderMouse alive. If m_fileInputElementUnderMouse is non-null then we must be doing an edit drag, so this should be the same as doing it in concludeEditDrag(). I could probably assert that an edit drag is taking place. > > > Source/WebCore/page/DragController.cpp:467 > > + // m_fileInputElementUnderMouse should be the element we hit tested for, > > + // unless it was made display:none by a drop event handler. > > + ASSERT(m_fileInputElementUnderMouse == element || !m_fileInputElementUnderMouse->renderer()); > > I don't think this assertion is accurate. You can also move the element to elsewhere in the document or to a different frame, right? True, but in those cases m_fileInputElementUnderMouse would have been nulled out in tryDocumentDrag() by virtue of the mouse no longer being over a file input element. > > > Source/WebCore/page/DragController.cpp:558 > > + // m_fileInputElementUnderMouse should be the element we hit tested > > + // for, unless it was made display:none by a drop event handler. > > + ASSERT(m_fileInputElementUnderMouse == result.innerNonSharedNode() || !m_fileInputElementUnderMouse->renderer()); > > Ditto. Yeah, you're right about this one. m_fileInputElementUnderMouse would be set to NULL when moved outside of an input element, but element would still be non-null. I need to fix this.

Comment on attachment 130769 [details] Patch r- since the assertion in canProcessDrag() is wrong in some cases.

Created attachment 130893 [details] Patch An even simpler approach, without the bogus ASSERT in canProcessDrag().

Comment on attachment 130893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130893&action=review > Source/WebCore/page/DragController.cpp:463 > + ASSERT(fileInput == element || !fileInput->renderer()); Makes a lot more sense :D

Committed r110243: <http://trac.webkit.org/changeset/110243>