Bug 32230 - getComputedStyle() returns different values for different zoom levels
Summary: getComputedStyle() returns different values for different zoom levels
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Erik Arvidsson
URL:
Keywords: GoogleBug, HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2009-12-07 10:33 PST by Simo Kinnunen
Modified: 2013-01-02 17:50 PST (History)
15 users (show)

See Also:


Attachments
Test case (1.19 KB, text/html)
2009-12-07 10:33 PST, Simo Kinnunen
no flags Details
better test case (443 bytes, text/html)
2010-06-11 10:16 PDT, Ojan Vafai
no flags Details
Patch (36.92 KB, patch)
2010-09-10 18:23 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Fixed merge conflicts (37.15 KB, patch)
2010-09-14 11:06 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Style fixes and reuse zoom adjusting code from RenderObject.h (38.57 KB, patch)
2010-09-14 17:29 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Occurrence of the bug after the first fix with display:none (1.45 KB, text/html)
2010-10-06 20:52 PDT, Rob Brackett
no flags Details
Patch (19.62 KB, patch)
2012-02-09 15:35 PST, Matthew Delaney
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simo Kinnunen 2009-12-07 10:33:45 PST
Created attachment 44419 [details]
Test case

In WebKit, getComputedStyle() returns scaled values for properties such as font-size, width and height. This makes it impossible to rely on these values. The following might make it easier to understand:

(el.style.fontSize = '20px') !== getComputedStyle(document.body, '').fontSize // if zoom level != 1

What makes this especially troubling is this:

(document.body.style.fontSize = getComputedStyle(document.body, '').fontSize) !== getComputedStyle(document.body, '').fontSize // if zoom level != 1

Firefox and Opera always return the same value regardless of zoom level.
Comment 1 Frances Berriman 2010-02-18 07:28:24 PST
Hi,

We've identified the same problem within our unit and manual tests for appearance methods.  Firefox, Opera and IE all report the values accurately, but Webkit browsers will return a zoomed+orig value.  

For example, create 2 elements with text inside.  Set the first to be 16px in size, and then use JS to apply get element 1's font-size value and apply it to element 2.  When normal zoom, both elements will appear as 16px, but zoom the page by 1, and element 2 will be larger than element 1 in a webkit browser.  Of course, the issue becomes more pronounced the further the user zooms. 

Thanks,

Frances
http://bbc.co.uk/glow
Comment 2 Erik Arvidsson 2010-04-22 11:38:49 PDT
Bug 25750 solved the same problem regarding getBoundingClientRects so I'm CC:ing some people from that list.

Gecko returns the same computed value no matter the zoom level so there should be no doubt what the right behavior is.
Comment 3 Ojan Vafai 2010-06-11 10:16:14 PDT
Created attachment 58483 [details]
better test case
Comment 4 Erik Arvidsson 2010-09-02 11:45:13 PDT
I looked at the code for this and I think a solution would be to replace all calls like this in CSSComputedStyleDeclaration.cpp

CSSPrimitiveValue::create(value, CSSPrimitiveValue::CSS_PX);

with something like

createZoomAdjustedPixelValue(value, renderer)

which would call WebCore::adjustForLocalZoom on the value and then return a new CSSPrimitiveValue.
Comment 5 Simon Fraser (smfr) 2010-09-02 11:57:11 PDT
Bug 42089 is somewhat related.
Comment 6 Erik Arvidsson 2010-09-10 18:23:19 PDT
Created attachment 67278 [details]
Patch
Comment 7 Darin Adler 2010-09-13 15:10:19 PDT
Comment on attachment 67278 [details]
Patch

Since the patch doesn’t apply, the EWS was unable to analyze it.
Comment 8 Erik Arvidsson 2010-09-13 15:37:34 PDT
Comment on attachment 67278 [details]
Patch

I'll upload a new patch without the merge conflicts.
Comment 9 Erik Arvidsson 2010-09-14 11:06:44 PDT
Created attachment 67576 [details]
Fixed merge conflicts
Comment 10 Darin Adler 2010-09-14 11:10:41 PDT
Comment on attachment 67576 [details]
Fixed merge conflicts

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

Great! I’m a little worried about the rounding caused by zooming and then scaling back, but I like how you are fitting this into the existing code.

> WebCore/css/CSSComputedStyleDeclaration.cpp:336
> +        // Needed because the pixel value truncates (rather than rounds) when scaling up.
> +        if (zoomFactor > 1)
> +            value++;
I’m not sure this fudge factor is going to do the exact right thing in all cases. Is adding exactly one the right thing to do? Can we make a test case that is right on the edge of the rounding?

> WebCore/css/CSSComputedStyleDeclaration.cpp:391
> +        if (l.type() == WebCore::Fixed)
> +          return zoomAdjustedPixelValue(l.value(), style);
Looks like the indentation is wrong here. The return should be indented four spaces.

I am not going to mark this review+ yet because that will cause the EWS bots to not process the patch!
Comment 11 Erik Arvidsson 2010-09-14 17:29:44 PDT
Created attachment 67621 [details]
Style fixes and reuse zoom adjusting code from RenderObject.h
Comment 12 Erik Arvidsson 2010-09-14 17:32:13 PDT
(In reply to comment #10)
> (From update of attachment 67576 [details])
> I’m not sure this fudge factor is going to do the exact right thing in all cases. Is adding exactly one the right thing to do? Can we make a test case that is right on the edge of the rounding?

This is the same code as used elsewhere for adjusting the zoom value. I refactored the code to use the same code.
Comment 13 Darin Adler 2010-09-14 22:19:05 PDT
Comment on attachment 67621 [details]
Style fixes and reuse zoom adjusting code from RenderObject.h

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

Looks good.

> WebCore/css/CSSComputedStyleDeclaration.cpp:341
> +    double zoomFactor = style->effectiveZoom();
> +    if (zoomFactor != 1)
> +        value = value / zoomFactor;
> +
> +    return CSSPrimitiveValue::create(value, CSSPrimitiveValue::CSS_NUMBER);
Why the if statement and special handling of 1? Is it a performance optimization? This would read better if it just said value / style->effectiveZoom() without a local variable.
Comment 14 Erik Arvidsson 2010-09-15 14:35:33 PDT
Fixed in http://trac.webkit.org/changeset/67568
Comment 15 Eric Seidel (no email) 2010-09-15 14:54:42 PDT
I think this broke http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/textCapitalizeEdgeCases.html on Leopard.
Comment 16 Erik Arvidsson 2010-09-15 15:10:54 PDT
(In reply to comment #15)
> I think this broke http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/textCapitalizeEdgeCases.html on Leopard.

I don't think it broke that one but I'm currently fixing these 2 tests

animations/fill-mode-transform.html
editing/pasteboard/page-zoom.html
Comment 17 WebKit Review Bot 2010-09-15 15:20:16 PDT
http://trac.webkit.org/changeset/67568 might have broken Qt Linux Release
Comment 18 Rob Brackett 2010-10-06 20:52:24 PDT
Created attachment 70033 [details]
Occurrence of the bug after the first fix with display:none

This issue still occurs when an element (or its ancestor) has display:none.
Comment 19 Matthew Delaney 2012-02-09 15:25:36 PST
Reopening to fix up more of the cases.
Comment 20 Matthew Delaney 2012-02-09 15:25:44 PST
<rdar://problem/9025697>
Comment 21 Matthew Delaney 2012-02-09 15:35:35 PST
Created attachment 126390 [details]
Patch
Comment 22 Matthew Delaney 2012-02-09 16:05:15 PST
Committed r107301: <http://trac.webkit.org/changeset/107301>
Comment 23 Adam Craven 2013-01-02 08:26:50 PST
I think there still might be a problem here... but this time with the computed value for 'left'.

An example of the issue can be seen here: http://jsfiddle.net/sFYjc/2/

(The jQuery community has stumbled upon the issue a few times over the years... http://bugs.jquery.com/ticket/4993#comment:9 )

I am running Chrome 23.0.1271.97 m.
Comment 24 Eric Seidel (no email) 2013-01-02 08:28:55 PST
Ideally we'd open a new bug with a test case and it can be resolved there. :)
Comment 25 Adam Craven 2013-01-02 17:50:48 PST
No problem - done: https://bugs.webkit.org/show_bug.cgi?id=105979