[Webkit-unassigned] [Bug 30754] Drags and Dragover events not in sync
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 3 21:14:50 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=30754
--- Comment #10 from Daniel Bates <dbates at webkit.org> 2009-11-03 21:14:49 PDT ---
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list