[Webkit-unassigned] [Bug 30754] Drags and Dragover events not in sync

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30754


Oliver Hunt <oliver at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42443|review?                     |review-
               Flag|                            |




--- Comment #9 from Oliver Hunt <oliver at apple.com>  2009-11-03 20:22:00 PDT ---
(From update of attachment 42443)

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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.