Bug 32230 – getComputedStyle() returns different values for different zoom levels

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simo Kinnunen]

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.

[Frances Berriman]

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

[Erik Arvidsson]

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.

[Ojan Vafai]

Created attachment 58483 [details]
better test case

[Erik Arvidsson]

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.

[Simon Fraser (smfr)]

Bug 42089 is somewhat related.

[Erik Arvidsson]

Created attachment 67278 [details]
Patch

[Darin Adler]

Comment on attachment 67278 [details]
Patch

Since the patch doesn’t apply, the EWS was unable to analyze it.

[Erik Arvidsson]

Comment on attachment 67278 [details]
Patch

I'll upload a new patch without the merge conflicts.

[Erik Arvidsson]

Created attachment 67576 [details]
Fixed merge conflicts

[Darin Adler]

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!

[Erik Arvidsson]

Created attachment 67621 [details]
Style fixes and reuse zoom adjusting code from RenderObject.h

[Erik Arvidsson]

(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.

[Darin Adler]

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.

[Erik Arvidsson]

Fixed in http://trac.webkit.org/changeset/67568

[Eric Seidel (no email)]

I think this broke http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/textCapitalizeEdgeCases.html on Leopard.

[Erik Arvidsson]

(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

[WebKit Review Bot]

http://trac.webkit.org/changeset/67568 might have broken Qt Linux Release

[Rob Brackett]

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.

[Matthew Delaney]

Reopening to fix up more of the cases.

[Matthew Delaney]

<rdar://problem/9025697>

[Matthew Delaney]

Created attachment 126390 [details]
Patch

[Matthew Delaney]

Committed r107301: <http://trac.webkit.org/changeset/107301>

[Adam Craven]

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.

[Eric Seidel (no email)]

Ideally we'd open a new bug with a test case and it can be resolved there. :)

[Adam Craven]

No problem - done: https://bugs.webkit.org/show_bug.cgi?id=105979