Bug 9605 - REGRESSION: pageX and pageY are both 0 for events created with initMouseEvent
: REGRESSION: pageX and pageY are both 0 for events created with initMouseEvent
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: XML DOM
: 420+
: Macintosh Mac OS X 10.4
: P1 Minor
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-26 09:16 PDT by Darin Adler
Modified: 2006-06-28 12:34 PDT (History)
2 users (show)

See Also:


Attachments
First attempt (4.78 KB, patch)
2006-06-27 06:09 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Improved patch (4.74 KB, patch)
2006-06-27 13:34 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Now with expected results (5.31 KB, patch)
2006-06-27 14:04 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff
Improved testcase (2.44 KB, patch)
2006-06-28 00:08 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2006-06-26 09:16:09 PDT
The fix for bug 8707 introduced this problem.
Comment 1 Darin Adler 2006-06-26 09:16:26 PDT
Bug 8707.
Comment 2 Rob Buis 2006-06-27 06:09:59 PDT
Created attachment 9060 [details]
First attempt

The patch inits pageX/pageY ok, and adds testing code to window-xy-properties.html to
verify it. I am not sure whether the other FIXME are part of this bug, I guess they are simple
to fix and either they should be protected or have setters.
Cheers,

Rob.
Comment 3 Darin Adler 2006-06-27 09:29:50 PDT
Comment on attachment 9060 [details]
First attempt

This looks wrong to me. The initMouseEvent function is passing clientX and clientY to a function that takes parameters named pageX and pageY.

The parameters to initMouseEvent are not the page x and y, but rather the client x and y. You can check this with other browsers, but I'm pretty sure this is true.
Comment 4 Rob Buis 2006-06-27 13:34:59 PDT
Created attachment 9066 [details]
Improved patch

Hi Darin,

You are right, this should fix it.
Cheers,

Rob.
Comment 5 Darin Adler 2006-06-27 13:43:13 PDT
Comment on attachment 9066 [details]
Improved patch

I think it's pretty ugly to have two different initCoordinates functions, but I think this will work right.

Ultimately we could change this all so that pageX/Y is always computed from clientX/Y. The only reason it's not like that today is Macintosh-specific stuff: the "viewport" coordinates for the web view are actually window coordinates on OS X so they can't be used directly for clientX/Y. Otherwise, we'd go change all the callers to pass in clientX/Y and the code would all match. It would be great to straighten that out some day.

Patch needs to include the patch to the test results too, though.

r=me, but review- because we need a patch that includes fast/dom/window-xy-properties-expected.txt.
Comment 6 Rob Buis 2006-06-27 14:04:13 PDT
Created attachment 9067 [details]
Now with expected results

Yup, it was not properly generated, now it is included in the patch.
Cheers,
Rob.
Comment 7 Darin Adler 2006-06-27 14:58:50 PDT
Comment on attachment 9067 [details]
Now with expected results

The values here aren't right for offsetX/Y. The reason for that is that they can't be correct until you dispatch the event to an object. We really should be testing the values of these after dispatching too.

r=me, we can deal with that later
Comment 8 Darin Adler 2006-06-27 20:47:01 PDT
Committed revision 15076.
Comment 9 Rob Buis 2006-06-28 00:08:03 PDT
Created attachment 9072 [details]
Improved testcase

The improved testcase now dispatches the event, so offsetX and offsetY are adjusted, the
test verifies it. Please let me know whether I need to make Changelog entries for the new patch to land.
Cheers,

Rob.
Comment 10 David Kilzer (:ddkilzer) 2006-06-28 04:08:49 PDT
Reopening bug per Comment #9.

Rob, please file a new bug, even for updated tests.  The best thing to do would be to set the blocks/depends on flag to reference the original bug.

See Bug 8707, Comment #27.

Comment 11 Darin Adler 2006-06-28 07:30:42 PDT
Comment on attachment 9072 [details]
Improved testcase

better -- r=me
Comment 12 Darin Adler 2006-06-28 07:35:17 PDT
Committed revision 15078.
Comment 13 Darin Adler 2006-06-28 07:36:58 PDT
(In reply to comment #10)
> Rob, please file a new bug, even for updated tests.  The best thing to do would
> be to set the blocks/depends on flag to reference the original bug.

Yes, Dave's right that we don't want to reuse a bug.

The reason is that the bug should represent the original problem and be reopened if it wasn't fixed. We don't want to reopen a bug to refine a fix, but rather just if the fix didn't work for some reason. This helps us be clearer on the "verification" step and understanding what is working and what isn't.
Comment 14 Rob Buis 2006-06-28 12:34:18 PDT
Hi Darin, Dave,

(In reply to comment #13)
> (In reply to comment #10)
> > Rob, please file a new bug, even for updated tests.  The best thing to do would
> > be to set the blocks/depends on flag to reference the original bug.
> 
> Yes, Dave's right that we don't want to reuse a bug.
> 
> The reason is that the bug should represent the original problem and be
> reopened if it wasn't fixed. We don't want to reopen a bug to refine a fix, but
> rather just if the fix didn't work for some reason. This helps us be clearer on
> the "verification" step and understanding what is working and what isn't.

Ok, will try to remember with future bug reports :) 
Cheers,

Rob.