Bug 79443 – REGRESSION (r105396): Dragging an iWork document into icloud.com opens it in the Mac app instead of uploading it to icloud.com

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

Steps to reproduce:
1. Navigate to the URL (sign in to iCloud if necessary)
2. Select the Numbers tab and drag a Numbers document from the Finder into the browser window

Expected result:
A progress window appears as the Numbers document is uploaded to iCloud, then it is added to the view of documents in the cloud

Actual result:
The document opens in the Mac Numbers app

Regression:
Caused by <http://trac.webkit.org/r105396>

<rdar://problem/10925082>

[Ryosuke Niwa]

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?

[Andy Estes]

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.

[Andy Estes]

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.

[Andy Estes]

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.

[Andy Estes]

Created attachment 130674 [details]
Patch

[WebKit Review Bot]

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.

[Andy Estes]

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.

[WebKit Review Bot]

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

[Andy Estes]

(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.

[Andy Estes]

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

[Andy Estes]

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

[Andy Estes]

Created attachment 130769 [details]
Patch

I got a little carried away with that last patch. This version is much more to the point.

[Ryosuke Niwa]

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.

[Andy Estes]

(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.

[Andy Estes]

Comment on attachment 130769 [details]
Patch

r- since the assertion in canProcessDrag() is wrong in some cases.

[Andy Estes]

Created attachment 130893 [details]
Patch

An even simpler approach, without the bogus ASSERT in canProcessDrag().

[Ryosuke Niwa]

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

[Andy Estes]

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