Bug 8707 – event.clientX and event.clientY should be relative to the viewport, not the canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Maciej Stachowiak]

When the main view is scrolled, event.clientX and event.clientY should give coordiates relative to the viewport, not the canvas. In other words, if you mouse the same distance from the favorites bar above the content view, you should get the same y coordinate, regardless of how far the page is scrolled. Opera, IE and Mozilla all agree on this, Safari does not.

See forthcoming test case.

[Maciej Stachowiak]

Created attachment 8087 [details]
test case that shows the problem

[Darin Adler]

See the FIXME In MouseRelatedEvent::receivedTarget.

[Rob Buis]

Created attachment 8906 [details]
First attempt

The patch makes it work for me, let me know whether other code
paths need to be taken into account as well...
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8906 [details]
First attempt

Sure, this fixes clientX and clientY, but what does it do to all the other X and Y properties on the event? We need a test case that checks all of those.

I remember trying a code change like this one, and it broke lots of things.

All bug fixes need a test, and this one lacks a test.

[Rob Buis]

Created attachment 8939 [details]
Now with testcase

The new patch fixes the problem, also I added a testcase which tests all window xy props.
Another option is to add setClientX/setClientY to MouseRelatedEvent, let me know what is
the best choice and whether there is an even better solution. Also I can format the code a
bit, just wanted to know first whether the approach was right.
Cheers,

Rob.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> The new patch fixes the problem, also I added a testcase which tests all window
> xy props.

The test should probably also scroll horizontally in addition to vertically to make sure the X-coordinates are set correctly, too.

Have you tried running the test case (manually) in Firefox or MSIE to see if Safari now matches their behavior?

[David Kilzer (:ddkilzer)]

NOTE: This bug may also fix some issues in Bug 8128.

[Rob Buis]

Hi,

(In reply to comment #6)
> (In reply to comment #5)
> > The new patch fixes the problem, also I added a testcase which tests all window
> > xy props.
> 
> The test should probably also scroll horizontally in addition to vertically to
> make sure the X-coordinates are set correctly, too.

Yep, will do.

> Have you tried running the test case (manually) in Firefox or MSIE to see if
> Safari now matches their behavior?

Indeed I had to do that manually. From my tests it matches the behavior.
Cheers,

Rob.

[Rob Buis]

Created attachment 8963 [details]
Improved patch+testcase

I adjusted the testcase to also test clientX. Also some code layout fixes.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 8963 [details]
Improved patch+testcase

This seems like a big step in the right direction.

But I think it's confusing to have local variables named offsetX and offsetY and parameter to the MouseEvent/WheelEvent constructors that are entirely different things than MouseEvent's own offsetX and offsetY properties.

Similarly, it doesn't make sense to me that the clientX and clientY parameters are no longer clientX and clientY! If their meaning is changing, then the name should change to. And it should only be called clientX and clientY if that's really what these are named.

Also, since the WheelEvent constructor does take a pointer to the abstract view, then we can get from that to the FrameView, so we don't need to add another parameter to the constructors -- not sure if that's better or not, but I think it probably is.

[Rob Buis]

Created attachment 8970 [details]
Improved patch+testcase

[Darin Adler]

Comment on attachment 8970 [details]
Improved patch+testcase

Almost there!

I still think that the local variables in EventTargetNode::dispatchMouseEvent should not be named offsetX/Y. Instead I think we should have pageX/Y local variables and do -= statements inside the if statement.

+    if (FrameView *view = document()->view()) {

Formatting wrong here -- * should be next to FrameView.

-                                                   document()->defaultView(), e.globalX(), e.globalY(), pos.x(), pos.y(),
-                                                   e.ctrlKey(), e.altKey(), e.shiftKey(), e.metaKey());
+                                                   document()->defaultView(), e.globalX(), e.globalY(),
+                                                   pos.x(), pos.y(), e.ctrlKey(), e.altKey(), e.shiftKey(), e.metaKey());

Should roll this change out -- it's just reformatting.

What guarantees that neither frame() or view() will be 0 in the WheelEvent constructor?

Between these small issues, I think I still give this a review-, but it's about ready to land. The only one I'm really concerned about is the WheelEvent constructor nil-check issue.

Also, the layout test does not test the wheel event code.

[David Kilzer (:ddkilzer)]

*** Bug 8128 has been marked as a duplicate of this bug. ***

[Rob Buis]

Created attachment 9010 [details]
Improved patch+testcase

Hi Darin,

This should address most of your points. About the wheel event, I tried to include
code into DumpRenderTree to generate wheel events, much like mouse events. However
I could not find a suitable NSEvent method to create them! So I think the solution is for
someone to indicate to me what I do wrong or generate them some other way, or opt
for a manual test for the wheel events?
Cheers,

Rob.

[Darin Adler]

Comment on attachment 9010 [details]
Improved patch+testcase

+    fprintf(stderr, "WHEEL EVENT!!");

Oops. Don't land that!

Otherwise, great. r=me

[David Kilzer (:ddkilzer)]

Committed revision 15023.

Removed the printf() statement (and corresponding prepare-ChangeLog method entry) before committing.

[David Kilzer (:ddkilzer)]

Attachment 9010 [details] broke the build.  WebCore/page/FrameView.cpp failed to compile.

Backed out fix as revision 15024.

[Darin Adler]

Comment on attachment 9010 [details]
Improved patch+testcase

So we need a version that compiles!

[Rob Buis]

Created attachment 9018 [details]
Now includes FrameView.cpp

[Darin Adler]

Comment on attachment 9018 [details]
Now includes FrameView.cpp

It's a real shame that the code to compute pageX and pageY is in 3 different places.

r=me

[David Kilzer (:ddkilzer)]

Committed revision 15032.

[David Kilzer (:ddkilzer)]

*** Bug 6574 has been marked as a duplicate of this bug. ***

[Rob Buis]

Created attachment 9041 [details]
Code cleanup

I know the previous patch landed, however as Darin pointed out calculating the
clientX/clientY in three places is a waste. This patch calculates it in one place and is
, I feel, cleaner. The results are the same as before.
Also, I think it needs to be checked whether fast/events/objc-event-api.html regressed
due to the previous patch.
Cheers,

Rob.

[David Kilzer (:ddkilzer)]

(In reply to comment #23)
> Also, I think it needs to be checked whether fast/events/objc-event-api.html
> regressed due to the previous patch.

It didn't regress from this patch (Attachment 9018 [details]).  It regressed due to Bug 9181 which added this output.  Bug 9579 should fix the test.

Reopening bug for the second patch.

[Rob Buis]

Hi,

(In reply to comment #24)
> (In reply to comment #23)
> > Also, I think it needs to be checked whether fast/events/objc-event-api.html
> > regressed due to the previous patch.
> 
> It didn't regress from this patch (Attachment 9018 [details] [edit]).  It regressed due to Bug
> 9181 which added this output.  Bug 9579 should fix the test.

Phew :) Thnx for checking.

> Reopening bug for the second patch.

Yep, I think it is an improvement.
Cheers,

Rob.

[Darin Adler]

(In reply to comment #23) 
> I know the previous patch landed, however as Darin pointed out calculating the
> clientX/clientY in three places is a waste. This patch calculates it in one
> place and is, I feel, cleaner.

Code cleanup is fine, but please don't reopen the bug to do it!

[Darin Adler]

(In reply to comment #24)
> Reopening bug for the second patch.

In the future, that's not how I'd like to handle things like this.

[Darin Adler]

Comment on attachment 9041 [details]
Code cleanup

I've found a number of problems with the original change; also this changes lots of things to be pageX/Y but leaves them named clientX/Y. I'll post a patch soon.

[Darin Adler]

Committed revision 15047.

Bug 9605 and bug 9606 cover problems I realized are caused by the original fix (not caused by the cleanup).

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.