|Summary:||getComputedStyle() returns different values for different zoom levels|
|Product:||WebKit||Reporter:||Simo Kinnunen <sorccu>|
|Component:||CSS||Assignee:||Erik Arvidsson <arv>|
|Severity:||Major||CC:||abarth, adammcraven, arv, bdakin, brettw, eric, fberriman, hyatt, macpherson, mdelaney7, menard, ojan, sam, simon.fraser, webkit.review.bot|
|Priority:||P2||Keywords:||GoogleBug, HasReduction, InRadar|
|Version:||528+ (Nightly build)|
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 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 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 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 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. :)