Bug 79443 - REGRESSION (r105396): Dragging an iWork document into icloud.com opens it in the Mac app instead of uploading it to icloud.com
: REGRESSION (r105396): Dragging an iWork document into icloud.com opens it in ...
Status: RESOLVED FIXED
: WebKit
Event Handling
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Major
Assigned To:
: https://www.icloud.com/#iwork
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2012-02-23 21:56 PST by
Modified: 2012-03-08 18:24 PST (History)


Attachments
Test case (518 bytes, text/html)
2012-03-03 19:30 PST, Andy Estes
no flags Details
Patch (17.70 KB, patch)
2012-03-07 12:54 PST, Andy Estes
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2012-03-07 22:11 PST, Andy Estes
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.20 KB, patch)
2012-03-08 13:36 PST, Andy Estes
rniwa: review+
aestes: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-23 21:56:52 PST
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>
------- Comment #1 From 2012-02-23 21:57:22 PST -------
<rdar://problem/10925082>
------- Comment #2 From 2012-02-23 22:03:35 PST -------
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?
------- Comment #3 From 2012-02-27 13:00:47 PST -------
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.
------- Comment #4 From 2012-03-03 19:30:01 PST -------
Created an attachment (id=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.
------- Comment #5 From 2012-03-05 15:23:32 PST -------
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.
------- Comment #6 From 2012-03-07 12:54:07 PST -------
Created an attachment (id=130674) [details]
Patch
------- Comment #7 From 2012-03-07 12:58:06 PST -------
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 #8 From 2012-03-07 13:42:25 PST -------
(From update of attachment 130674 [details])
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 #9 From 2012-03-07 14:01:10 PST -------
(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
------- Comment #10 From 2012-03-07 15:30:37 PST -------
(In reply to comment #9)
> (From update of attachment 130674 [details] [details])
> Attachment 130674 [details] [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.
------- Comment #11 From 2012-03-07 16:49:48 PST -------
It's sad that http://queues.webkit.org/results/11854336 make no mention of the test failures listed in comment #9.
------- Comment #12 From 2012-03-07 16:51:38 PST -------
I think http://queues.webkit.org/results/11850353 is the link the review bot meant to include.
------- Comment #13 From 2012-03-07 22:11:58 PST -------
Created an attachment (id=130769) [details]
Patch

I got a little carried away with that last patch. This version is much more to the point.
------- Comment #14 From 2012-03-08 00:10:37 PST -------
(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?

> 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.
------- Comment #15 From 2012-03-08 00:31:47 PST -------
(In reply to comment #14)
> (From update of attachment 130769 [details] [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 #16 From 2012-03-08 00:33:13 PST -------
(From update of attachment 130769 [details])
r- since the assertion in canProcessDrag() is wrong in some cases.
------- Comment #17 From 2012-03-08 13:36:20 PST -------
Created an attachment (id=130893) [details]
Patch

An even simpler approach, without the bogus ASSERT in canProcessDrag().
------- Comment #18 From 2012-03-08 13:40:46 PST -------
(From update of attachment 130893 [details])
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
------- Comment #19 From 2012-03-08 18:24:49 PST -------
Committed r110243: <http://trac.webkit.org/changeset/110243>