Bug 24736 - event.offsetX and offsetY are wrong with full page zoom
Summary: event.offsetX and offsetY are wrong with full page zoom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-20 21:14 PDT by Simon Fraser (smfr)
Modified: 2009-03-23 11:57 PDT (History)
1 user (show)

See Also:


Attachments
Testcase: zoom, then hover and look at the offsetX/offsetY numbers (6.91 KB, text/html)
2009-03-20 21:15 PDT, Simon Fraser (smfr)
no flags Details
Patch, changelog (4.07 KB, patch)
2009-03-22 13:08 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-03-20 21:14:53 PDT
Page zoom is taken into account when setting event.pageX and pageY, but event.offestX and offsetY are not fixed up accordingly.
Comment 1 Simon Fraser (smfr) 2009-03-20 21:15:25 PDT
Created attachment 28817 [details]
Testcase: zoom, then hover and look at the offsetX/offsetY numbers
Comment 2 Simon Fraser (smfr) 2009-03-20 21:39:08 PDT
offsetX/offsetY are also used for slider thumb tracking, so this breaks that.
Comment 3 Simon Fraser (smfr) 2009-03-20 22:44:57 PDT
Bug 24733 fixes related issues with pageX, pageY
Comment 4 Simon Fraser (smfr) 2009-03-22 13:08:01 PDT
Created attachment 28838 [details]
Patch, changelog

This also fixes bug 24748.
Comment 5 Cameron Zwarich (cpst) 2009-03-22 21:34:01 PDT
Doesn't this patch need tests?
Comment 6 Simon Fraser (smfr) 2009-03-22 21:36:41 PDT
There's no way to test full page zoom as the user sees it. That should be an RFE for DumpRenderTree.
Comment 7 Simon Fraser (smfr) 2009-03-22 21:39:56 PDT
Hmm, maybe zoom: style can be used to test.
Comment 8 Cameron Zwarich (cpst) 2009-03-22 21:45:36 PDT
(In reply to comment #7)
> Hmm, maybe zoom: style can be used to test.

That's what existing zoom fixes use, as far as I can tell.
Comment 9 Darin Adler 2009-03-23 08:59:11 PDT
Comment on attachment 28838 [details]
Patch, changelog

> +        * dom/Document.cpp:
> +        (WebCore::Document::elementFromPoint):
> +        Document::elementFromPoint() needs to take full-page zoom into account.

How can the caller tell that the x and y here are supposed to be "non-zoomed" coordinates? Is there a naming scheme for such things?

> +    float zoomFactor = frame() ? frame()->pageZoomFactor() : 1.0f;

It's becoming more and more clear that the zoom factor needs to be stored in the Document or the FrameView, not the Frame.

Also, elementFromPoint seems like it should be a simple forwarder to a function in FrameView or RenderView. We may be obliged to put some of these operations in the DOM because of the public DOM API, but internally we should try to make a bit of model/view separation.

Where are the regression tests for these changes?

I am really tempted to say review- because of the lack of regression tests, but I'm going to say review+ counting on you adding them.
Comment 10 Simon Fraser (smfr) 2009-03-23 11:52:54 PDT
I will add some tests with this commit, but it's not possible to fully test FPZ via DRT. I filed bug 24761.
Comment 11 Simon Fraser (smfr) 2009-03-23 11:57:11 PDT
http://trac.webkit.org/changeset/41914