Bug 30754 – Drags and Dragover events not in sync

• Component: DOM
• Status: RESOLVED FIXED

[Francisco Tolmasky]

Per the spec (http://www.whatwg.org/specs/web-apps/current-work/#current-drag-operation) it appears every dragover should be preceded by a drag event, but it is possible to get multiple dragovers for every single drag event.

In the sample URL, drag around a bit and you will see something like this:

...
drag
dragover
dragover
dragover
drag
...

[Daniel Bates]

Confirmed this issue only affects the Mac build (*).

(*) confirmed with r50039.

[Daniel Bates]

Err, I should be more precise. Confirmed that 1) this issue affects the Mac build and 2) it does not affect the Windows build.

(In reply to comment #1)
> Confirmed this issue only affects the Mac build (*).
> 
> (*) confirmed with r50039.

[Daniel Bates]

Created attachment 41815 [details]
Layout test

Can also be manually run.

[Daniel Bates]

Created attachment 41816 [details]
Self contained test

For convenience, this is a self-contained version of the test included in the patch Layout test.

Compare the test results to Firefox.

[Daniel Bates]

CC'ing Oliver since he has also done work in the Mac drag-and-drop code.

[Daniel Bates]

Confirmed that this bug also affects Windows XP.

[Daniel Bates]

CC'ing Adam on this, since it is both a Windows and drag-and-drop issue.

[Daniel Bates]

Created attachment 42443 [details]
Patch with test case

Fixes this bug by making our drag-and-drop processing model conform to section 7.9.4 of the HTML 5 spec., <http://dev.w3.org/html5/spec/Overview.html#drag-and-drop-processing-model>.

I probably am going to get called out on the very large comment I wrote in method EventHandler::updateDragAndDrop. This method was non-trivial to understand (at least to me) due to the triple recursion involved as well as who calls this code. I tried to re-factor this code, but I felt it made it less clear what was happening. I am open to suggestions on refactoring this.

I gave a proof in my comment as to why/how my fix works using the added field, m_shouldOnlyFireDragOverEvent. However, in testing with r49924 on Windows the mouse up, mouse down, and mouse move events don't work the same as in the Mac build (r50425). This should not necessarily (in my opinion) hold us back from going forward with this fix. Fixing this bug is definitely a step forward on the Windows build. Hence, I am marking this for review. But we may need to fix some more Windows drag-and-drop bugs for the drag-and-drop code to be on par with the Mac build/HTML 5 compliant. I will identify and file a formal bug(s) once I confirm with the ToT Windows build.

Let me know if you have any suggestions on my approach to fixing this bug.

[Oliver Hunt]

Comment on attachment 42443 [details]
Patch with test case


>  
>          if (m_dragTarget) {
> -            Frame* frame = (m_dragTarget->hasTagName(frameTag) || m_dragTarget->hasTagName(iframeTag)) ? static_cast<HTMLFrameElementBase*>(m_dragTarget.get())->contentFrame() : 0;
> +            Frame* frame = dragTargetUseToBeAFrameElement? static_cast<HTMLFrameElementBase*>(m_dragTarget.get())->contentFrame() : 0;

Space before ?

> +
> +        if (newTarget) {
> +            // Q: Why do we do this?
> +            //
> +            // A: As per section 7.9.4 of the HTML 5 spec., we must fire the dragover event on the new target (if it exists).
> +            //    Notice, there are three drag events that could preceed a dragover event (in order of firing): drag, dragenter,
> +            //    and dragleave. Lets look at the interesting case when we enter a targer area. Then, in the first call to this
> +            //    function, we may see the following events fired: drag, dragenter, (dragleave)?. In a subsequent call to this
> +            //    function, we may see the following events fired (drag)?, dragover.
> +            //
> +            // Q: Why do we set a field here as opposed to explicitly calling dispatchDragEvent to fire the dragover event?
> +            //
> +            // A: This function is called during the processing of various OS-generated mouse events, including mouse up,
> +            //    mouse down, and mouse move. Notice, it is always the case that this function is called at least twice
> +            //    (one on mouse up and one on mouse down) (*). Moreover, on either a mouse up or mouse down event, it is always
> +            //    the case that m_dragTarget == newTarget (by (**)). So, a dragover event is always fired (if we are over a target, i.e. 
> +            //    newTarget is not null). Therefore, we do not explicitly call dispatchDragEvent here because it could ultimately
> +            //    result in the appearance that two dragover events fired.
> +            //
> +            // Q: Isn't setting m_shouldOnlyFireDragOverEvent inane? How can we be guranteed that we will be called again? How 
> +            //    can we be guranteed that we will execute the else clause of the ambient if-else statement?
> +            //
> +            // A: By (*) we are guranteed to be called at least twice, and by (**) at least one of those calls executes the "else"
> +            //    clause of the ambient if-else statement. So, we can set a flag here to fire the dragover event and we are
> +            //    guaranteed that the dragover event will be fired on some subsequent call to this function.
> +            //
> +            // (**) Let the smallest unit of measurement be a pixel. Let A and B be two distinct target areas (i.e. there is at least one 
> +            //     pixel in each target area that does not overlap some pixel in the other target area - so that it is possible to mouse
> +            //     up/down in both target areas). We say that a mouse move event is fired whenever the coordinates of the mouse change by
> +            //     one pixel in some direction.
> +            //
> +            // Claim: On a mouse up, mouse down, or mouse move event, m_dragTarget == newTarget.
> +            // Proof: Proof by contradiction. Suppose on a mouse up, mouse down, or mouse move event that m_dragTarget != newTarget.
> +            //        Without loss of generality, lets consider the case of a mouse up event.
> +            //        Notice, we must have a corresponding mouse down event that preceeded the mouse up event.
> +            //        Assume the mouse down event occurs in target area m_dragTarget := A, and that on a mouse move event we set m_dragTarget == newTarget.
> +            //        Suppose a mouse up event is fired and m_dragTarget != newTarget.
> +            //        Then the cursor must be positioned over B (i.e. newTarget == B).
> +            //        Since A != B (by distinctness), the mouse must have moved at least one pixel in some direction.
> +            //        But then m_dragTarget == newTarget (since a mouse move event fired).
> +            //        Contradiction, by assumption, m_dragTarget != newTarget.
> +            //        Therefore, m_dragTarget == newTarget.

Your supposition is entirely correct -- this comment is _far_ too long, it shouldn't need to be more than 3 or 4 lines, and we have far more complex logic that has less, as reading the code gives you a reliably up to date reference for the behaviour, and regression tests should prevent unintentional changes.

r- because of this.

> +            else {
> +                // Normally we would fire a drag then a dragover event here. However when dealing with sub-frames, we may
> +                // need to fire only a dragover event. This may be because we have to notify the old target, which is in
> +                // a different frame from that of the new target, to fire a dragleave event. Hence, we check the field
> +                // m_shouldOnlyFireDragOverEvent to determine if we need to fire only a dragover event.

This comment also seems overly verbose
>      
> -void EventHandler::dragSourceMovedTo(const PlatformMouseEvent& event)
> +void EventHandler::dragSourceMovedTo(const PlatformMouseEvent&)
>  {
> -    if (dragState().m_dragSrc && dragState().m_dragSrcMayBeDHTML)
> -        // for now we don't care if event handler cancels default behavior, since there is none
> -        dispatchDragSrcEvent(eventNames().dragEvent, event);
>  }

This function is now empty so should be removed, which of course will require updating various webkit clients.

[Daniel Bates]

(In reply to comment #9)
> (From update of attachment 42443 [details])
> 
> >  
> >          if (m_dragTarget) {
> > -            Frame* frame = (m_dragTarget->hasTagName(frameTag) || m_dragTarget->hasTagName(iframeTag)) ? static_cast<HTMLFrameElementBase*>(m_dragTarget.get())->contentFrame() : 0;
> > +            Frame* frame = dragTargetUseToBeAFrameElement? static_cast<HTMLFrameElementBase*>(m_dragTarget.get())->contentFrame() : 0;
> 
> Space before ?

Shouldn't be, but I'll check. Notice, I changed "m_dragTarget->hasTagName(frameTag) || m_dragTarget->hasTagName(iframeTag)" to "dragTargetUseToBeAFrameElement".

> 
> > +
> > +        if (newTarget) {
> > +            // Q: Why do we do this?
> > +            //
> > +            // A: As per section 7.9.4 of the HTML 5 spec., we must fire the dragover event on the new target (if it exists).
> > +            //    Notice, there are three drag events that could preceed a dragover event (in order of firing): drag, dragenter,
> > +            //    and dragleave. Lets look at the interesting case when we enter a targer area. Then, in the first call to this
> > +            //    function, we may see the following events fired: drag, dragenter, (dragleave)?. In a subsequent call to this
> > +            //    function, we may see the following events fired (drag)?, dragover.
> > +            //
> > +            // Q: Why do we set a field here as opposed to explicitly calling dispatchDragEvent to fire the dragover event?
> > +            //
> > +            // A: This function is called during the processing of various OS-generated mouse events, including mouse up,
> > +            //    mouse down, and mouse move. Notice, it is always the case that this function is called at least twice
> > +            //    (one on mouse up and one on mouse down) (*). Moreover, on either a mouse up or mouse down event, it is always
> > +            //    the case that m_dragTarget == newTarget (by (**)). So, a dragover event is always fired (if we are over a target, i.e. 
> > +            //    newTarget is not null). Therefore, we do not explicitly call dispatchDragEvent here because it could ultimately
> > +            //    result in the appearance that two dragover events fired.
> > +            //
> > +            // Q: Isn't setting m_shouldOnlyFireDragOverEvent inane? How can we be guranteed that we will be called again? How 
> > +            //    can we be guranteed that we will execute the else clause of the ambient if-else statement?
> > +            //
> > +            // A: By (*) we are guranteed to be called at least twice, and by (**) at least one of those calls executes the "else"
> > +            //    clause of the ambient if-else statement. So, we can set a flag here to fire the dragover event and we are
> > +            //    guaranteed that the dragover event will be fired on some subsequent call to this function.
> > +            //
> > +            // (**) Let the smallest unit of measurement be a pixel. Let A and B be two distinct target areas (i.e. there is at least one 
> > +            //     pixel in each target area that does not overlap some pixel in the other target area - so that it is possible to mouse
> > +            //     up/down in both target areas). We say that a mouse move event is fired whenever the coordinates of the mouse change by
> > +            //     one pixel in some direction.
> > +            //
> > +            // Claim: On a mouse up, mouse down, or mouse move event, m_dragTarget == newTarget.
> > +            // Proof: Proof by contradiction. Suppose on a mouse up, mouse down, or mouse move event that m_dragTarget != newTarget.
> > +            //        Without loss of generality, lets consider the case of a mouse up event.
> > +            //        Notice, we must have a corresponding mouse down event that preceeded the mouse up event.
> > +            //        Assume the mouse down event occurs in target area m_dragTarget := A, and that on a mouse move event we set m_dragTarget == newTarget.
> > +            //        Suppose a mouse up event is fired and m_dragTarget != newTarget.
> > +            //        Then the cursor must be positioned over B (i.e. newTarget == B).
> > +            //        Since A != B (by distinctness), the mouse must have moved at least one pixel in some direction.
> > +            //        But then m_dragTarget == newTarget (since a mouse move event fired).
> > +            //        Contradiction, by assumption, m_dragTarget != newTarget.
> > +            //        Therefore, m_dragTarget == newTarget.
> 
> Your supposition is entirely correct -- this comment is _far_ too long, it
> shouldn't need to be more than 3 or 4 lines, and we have far more complex logic
> that has less, as reading the code gives you a reliably up to date reference
> for the behaviour, and regression tests should prevent unintentional changes.
> 
> r- because of this.

Ha, I knew it. Will condense into a short comment.

> 
> > +            else {
> > +                // Normally we would fire a drag then a dragover event here. However when dealing with sub-frames, we may
> > +                // need to fire only a dragover event. This may be because we have to notify the old target, which is in
> > +                // a different frame from that of the new target, to fire a dragleave event. Hence, we check the field
> > +                // m_shouldOnlyFireDragOverEvent to determine if we need to fire only a dragover event.
> 
> This comment also seems overly verbose

Will fix.

> >      
> > -void EventHandler::dragSourceMovedTo(const PlatformMouseEvent& event)
> > +void EventHandler::dragSourceMovedTo(const PlatformMouseEvent&)
> >  {
> > -    if (dragState().m_dragSrc && dragState().m_dragSrcMayBeDHTML)
> > -        // for now we don't care if event handler cancels default behavior, since there is none
> > -        dispatchDragSrcEvent(eventNames().dragEvent, event);
> >  }
> 
> This function is now empty so should be removed, which of course will require
> updating various webkit clients.

Will remove.

[Daniel Bates]

Created attachment 42524 [details]
Patch 1 with test case

Patch 1 of 2.

Modifies EventHandler::updateDragAndDrop to conform to the drag and drop processing model described in section 7.9.4 of the HTML 5 spec. <http://dev.w3.org/html5/spec/Overview.html#drag-and-drop-processing-model>.

[Daniel Bates]

Created attachment 42525 [details]
Patch 2

Patch 2 of 2.

This patch removes the method EventHandler::dragSourceMovedTo, which is no longer needed.

No new test cases are needed as we can use the existing tests.

[Oliver Hunt]

Comment on attachment 42525 [details]
Patch 2

have you checked that qt and gtk builds don't call this? r=me assuming that you have, otherwise you'll need to fix them as well before landing

[Daniel Bates]

From searching through the entire source, neither the qt- nor gtk- port call EventHandler::dragSourceMovedTo.

(In reply to comment #13)
> (From update of attachment 42525 [details])
> have you checked that qt and gtk builds don't call this? r=me assuming that you
> have, otherwise you'll need to fix them as well before landing

[Daniel Bates]

Will need to update the patches before I land, due to Changeset 50566 <http://trac.webkit.org/changeset/50566>.

[Daniel Bates]

Comment on attachment 42524 [details]
Patch 1 with test case

This patch was made obsolete by Changeset 50566.

[Daniel Bates]

Created attachment 42708 [details]
Patch 1 with test case

Because of the changes made in Changeset 50566 <http://trac.webkit.org/changeset/50566>, I needed make more than a handful of changes to the original patch.

Also, this patch corrects an issue where the drag event did not precede a dragleave event when the drag operation is canceled. I modified EventHandler::cancelDragAndDrop to fire the drag event before a dragleave under this circumstance.

[Daniel Bates]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/events/drag-and-drop-fire-drag-dragover-expected.txt
Adding         LayoutTests/fast/events/drag-and-drop-fire-drag-dragover.html
Sending        LayoutTests/fast/events/drag-in-frames-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.DragSupport.exp
Sending        WebCore/page/EventHandler.cpp
Sending        WebCore/page/EventHandler.h
Sending        WebKit/mac/ChangeLog
Sending        WebKit/mac/WebView/WebFrame.mm
Sending        WebKit/mac/WebView/WebFrameInternal.h
Sending        WebKit/mac/WebView/WebHTMLView.mm
Sending        WebKit/win/ChangeLog
Sending        WebKit/win/WebDropSource.cpp
Transmitting file data ..............
Committed revision 50786.
<http://trac.webkit.org/changeset/50786>

[Daniel Bates]

I checked out both the GTK and QT builds and I thought there would be no problems. Turns out they both did not pass the test /fast/events/drag-and-drop-fire-drag-dragover.html.

I may need to get a hold of a VM image of both the GTK and QT builds to look into making these builds pass the test.

If you feel we should roll this patch out in the meantime, that is fine. Let me know your thoughts.

(In reply to comment #13)
> (From update of attachment 42525 [details])
> have you checked that qt and gtk builds don't call this? r=me assuming that you
> have, otherwise you'll need to fix them as well before landing

[Daniel Bates]

It appears that both QT and GTK don't support DHTML drag-and-drop. This can also be seen be looking at their respective Skip files. 

The GTK Skip file, references bug #30576.

I cannot find a Qt bug report on the "missing drag-and-drop support" (from Qt skip file), so I filed bug #31332.

I will try to get a VM image setup with both the Qt and GTK builds to look into the drag-and-drop issue.

In the meantime, I would suggest adding the test /fast/events/drag-and-drop-fire-drag-dragover.html to the Skip files for the Qt and GTK builds. I filed bug #31334 to add this test case to the Skip files.

For your reference, here are the diffs of the test results under GTK and Qt:

GTK Diff: <http://build.webkit.org/results/GTK%20Linux%20Release/r50786%20%285680%29/fast/events/drag-and-drop-fire-drag-dragover-pretty-diff.html>

Qt Diff: <http://build.webkit.org/results/Qt%20Linux%20Release/r50786%20%283684%29/fast/events/drag-and-drop-fire-drag-dragover-pretty-diff.html>

(In reply to comment #19)
> I checked out both the GTK and QT builds and I thought there would be no
> problems. Turns out they both did not pass the test
> /fast/events/drag-and-drop-fire-drag-dragover.html.
> 
> I may need to get a hold of a VM image of both the GTK and QT builds to look
> into making these builds pass the test.
> 
> If you feel we should roll this patch out in the meantime, that is fine. Let me
> know your thoughts.
> 
> (In reply to comment #13)
> > (From update of attachment 42525 [details] [details])
> > have you checked that qt and gtk builds don't call this? r=me assuming that you
> > have, otherwise you'll need to fix them as well before landing