572294 - when creating a drop event make sure the refpoint is relative to the widget we actually use

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

When we create a drop event based on an existing mouse event there is the potential that we use a different widget than the existing mouse event. So adjust the refpoint. I figured this was safer than switching to use the same widget as the mouse event.

I also noticed some comments above GenerateDragGesture don't seem to be valid anymore. As far as I can tell that code correctly deals with the mentioned cases now.

Comment 1

[Neil Deakin]

Seems ok, but roc would be a better reviewer

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

DOM created events have no widget, but they set the refpoint to the screenpoint. So handle the null widget case.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How can mouseEvent->widget be null? Surely it can't? Can we just assert it's not null and crash if it is?

Comment 4

[Timothy Nikkel (:tnikkel)]

When the drop event created here http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug508479.html?force=1#57 gets to the code in the patch the mouse event has no widget and we crash with v1 of the patch.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about, if the event has no widget we assert that the refpoint is 0,0 (since it's meaningless) and set our refpoint to (0,0) as well? Or something like that?

Comment 6

[Timothy Nikkel (:tnikkel)]

In the case I mentioned the refPoint gets set to (screenX, screenY) as passed to the initDragEvent function.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 451731 [details] [diff] [review]
patch v2

ok, can you make sure it's documented somewhere that a null widget means refpoint is in screen coordinates?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to comment #7)
> ok, can you make sure it's documented somewhere that a null widget means
> refpoint is in screen coordinates?

Neil, do you know if that is documented somewhere? Or where I should document it?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I believe it is document only in the code, in
nsDOMMouseEvent::InitMouseEvent

Comment 10

[Timothy Nikkel (:tnikkel)]

I think a good place for it would be in nsGUIEvent.h in the definition of nsEvent where we define the refPoint.

  // In widget relative coordinates, not modified by layout code.
  nsIntPoint  refPoint;

I'll change it to:

  // Relative to the widget of the event, or if there is no widget then it is
  // in screen coordinates. Not modified by layout code.

Comment 11

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/4f1ae85ff9d1