Bug 88430 - Element.getBoundingClientRect() and Element.getClientRects() return incorrect values in frames in a scaled page
Summary: Element.getBoundingClientRect() and Element.getClientRects() return incorrect...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-06 10:08 PDT by Xianzhu Wang
Modified: 2012-12-17 20:58 PST (History)
8 users (show)

See Also:


Attachments
patch (14.80 KB, patch)
2012-06-06 11:23 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v2 (removed extra code in layout test) (14.37 KB, patch)
2012-06-06 11:35 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v3 (using Frame::frameScaleFactor()) (14.31 KB, patch)
2012-06-06 13:28 PDT, Xianzhu Wang
jamesr: review+
jamesr: commit-queue-
Details | Formatted Diff | Diff
patch for landing (14.33 KB, patch)
2012-06-06 16:52 PDT, Xianzhu Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xianzhu Wang 2012-06-06 10:08:58 PDT
Element.getBoudingClientRect() and Element.getClientRects() should not scale back the rects if the element is in a frame because page scale only apply to elements in the main frame.
Comment 1 Xianzhu Wang 2012-06-06 11:23:05 PDT
Created attachment 146068 [details]
patch
Comment 2 Xianzhu Wang 2012-06-06 11:35:54 PDT
Created attachment 146075 [details]
patch v2 (removed extra code in layout test)
Comment 3 Alexandre Elias 2012-06-06 13:05:02 PDT
Looks like frame()->frameScaleFactor() was already introduced for this purpose in  bug 68081 , maybe we can reuse that?
Comment 4 Xianzhu Wang 2012-06-06 13:28:33 PDT
Created attachment 146098 [details]
patch v3 (using Frame::frameScaleFactor())

Thanks Alex for your good idea.
Comment 5 James Robinson 2012-06-06 15:39:45 PDT
Comment on attachment 146098 [details]
patch v3 (using Frame::frameScaleFactor())

View in context: https://bugs.webkit.org/attachment.cgi?id=146098&action=review

R=me

> Source/WebCore/ChangeLog:3
> +        Element.getBoudingClientRect() and Element.getClientRects() return incorrect values in frames in a scaled page

typo "bouding" -> "bounding"

> Source/WebCore/dom/Document.cpp:6041
> +

extra newline

> LayoutTests/fast/dom/Element/scale-page-bounding-client-rect-in-frame.html:1
> +<html>

please use <!DOCTYPE html> for this and other new test files unless you explicitly want to test quirks mode behavior
Comment 6 Xianzhu Wang 2012-06-06 16:51:51 PDT
Comment on attachment 146098 [details]
patch v3 (using Frame::frameScaleFactor())

View in context: https://bugs.webkit.org/attachment.cgi?id=146098&action=review

>> Source/WebCore/ChangeLog:3
>> +        Element.getBoudingClientRect() and Element.getClientRects() return incorrect values in frames in a scaled page
> 
> typo "bouding" -> "bounding"

Done (also other places)

>> Source/WebCore/dom/Document.cpp:6041
>> +
> 
> extra newline

Done.

>> LayoutTests/fast/dom/Element/scale-page-bounding-client-rect-in-frame.html:1
>> +<html>
> 
> please use <!DOCTYPE html> for this and other new test files unless you explicitly want to test quirks mode behavior

Done (also other places).

Also changed "xxxPageScale" in the change to "xxxFrameScale" as per Grace's suggestion.
Comment 7 Xianzhu Wang 2012-06-06 16:52:07 PDT
Created attachment 146146 [details]
patch for landing
Comment 8 WebKit Review Bot 2012-06-06 23:57:38 PDT
Comment on attachment 146146 [details]
patch for landing

Clearing flags on attachment: 146146

Committed r119690: <http://trac.webkit.org/changeset/119690>
Comment 9 WebKit Review Bot 2012-06-06 23:57:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Fraser (smfr) 2012-12-17 20:58:01 PST
I wish we could have just fixed absoluteQuads() to not include frame scale, rather than adding code to adjustFloatQuadsForScrollAndAbsoluteZoomAndFrameScale() to remove frame scale from the quads again.

We don't have a clear policy about when localToAbsolute() stuff includes frame scale, but could add one.