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.
Created attachment 8087 [details] test case that shows the problem
See the FIXME In MouseRelatedEvent::receivedTarget.
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.
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.
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.
(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?
NOTE: This bug may also fix some issues in Bug 8128.
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.
Created attachment 8963 [details] Improved patch+testcase I adjusted the testcase to also test clientX. Also some code layout fixes. Cheers, Rob.
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.
Created attachment 8970 [details] Improved patch+testcase
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.
*** Bug 8128 has been marked as a duplicate of this bug. ***
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.
Comment on attachment 9010 [details] Improved patch+testcase + fprintf(stderr, "WHEEL EVENT!!"); Oops. Don't land that! Otherwise, great. r=me
Committed revision 15023. Removed the printf() statement (and corresponding prepare-ChangeLog method entry) before committing.
Attachment 9010 [details] broke the build. WebCore/page/FrameView.cpp failed to compile. Backed out fix as revision 15024.
Comment on attachment 9010 [details] Improved patch+testcase So we need a version that compiles!
Created attachment 9018 [details] Now includes FrameView.cpp
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
Committed revision 15032.
*** Bug 6574 has been marked as a duplicate of this bug. ***
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.
(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.
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.
(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!
(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.
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.
Committed revision 15047. Bug 9605 and bug 9606 cover problems I realized are caused by the original fix (not caused by the cleanup).
Mass moving XML DOM bugs to the "DOM" Component.