getComputedStyle() should return zoom-adjusted offset values

[Adam Craven]

Created attachment 181121 [details]
Test Case

In WebKit, getComputedStyle() returns scaled values (seems to be +10%) for properties such as left, right, top and bottom (when the 'position' of an element is 'relative'). This makes it impossible to rely on these values.

Firefox reports the values accurately.

Please see the attached test case.


The jQuery community has stumbled upon the issue a few times over the years... This issue prevents jQuery.animate() from functioning correctly in WebKit. http://bugs.jquery.com/ticket/4993#comment:9
An example of that issue can be seen here: http://jsfiddle.net/sFYjc/2/


Related to https://bugs.webkit.org/show_bug.cgi?id=32230

[Adam Craven]

To be more accurate, forget about "(seems to be +10%)"... it is scaled according to the zoom level... The 10% is for when the zoom is 110%...

[Mike Sherov]

Please correct the title to getComputedStyle please. There's a typo.

[Adam Craven]

Sorry, typo fixed...

[Emil A Eklund]

Seems wrong. Thanks for the report and the test case.

[Emil A Eklund]

Created attachment 188671 [details]
Patch

[Darin Adler]

Comment on attachment 188671 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:670
> +        return zoomAdjustedPixelValue(l.value(), style);

Does this work correctly when "l" is a percentage? The test case does not cover that.

[Emil A Eklund]

Good point, I'll make sure to add tests for percentage values.

[Emil A Eklund]

Created attachment 189181 [details]
Patch

[Emil A Eklund]

Darin, when you get a chance could you please take another look?

[Emil A Eklund]

Ping?

[Darin Adler]

Comment on attachment 189181 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:662
> +        renderer->document()->updateLayoutIgnorePendingStylesheets();
> +        LayoutUnit containingBlockSize = (propertyID == CSSPropertyLeft || propertyID == CSSPropertyRight) ?
> +            toRenderBox(renderer)->containingBlockLogicalWidthForContent() :
> +            toRenderBox(renderer)->containingBlockLogicalHeightForContent(ExcludeMarginBorderPadding);

This code sequence is incorrect. Calling updateLayout can result in the renderer being destroyed, so it’s not safe to use renderer after that. The renderer may no longer exist, or may not be a box.

Generally speaking, when we update layout, all renderers are invalid and must be refetched. I believe that includes renderView.

[Darin Adler]

Typically this means that calls to updateLayout need to be made at a higher level, not inside functions that take render tree arguments, but before even fetching the renderers.

[Emil A Eklund]

Oh, of course. That makes more sense. Thank you!

[Mike Sherov]

Will this patch also fix the values for offsetWidth and offsetHeight?

[Mike Sherov]

offsetWidth and offsetHeight are manifesting themselves as http://bugs.jquery.com/ticket/13543 in jQuery Core.

[Emil A Eklund]

(In reply to comment #15)
> offsetWidth and offsetHeight are manifesting themselves as http://bugs.jquery.com/ticket/13543 in jQuery Core.

No, that has to do with our pixel-snapping logic (see http://trac.webkit.org/wiki/LayoutUnit). For precise box metrics we recommend using getBoundingClientRect which returns values with decimal precision.

[Emil A Eklund]

Created attachment 192295 [details]
Patch

[Emil A Eklund]

(In reply to comment #11)

Found a better way to do it, thanks. Please take another look.

[Eric Seidel (no email)]

I thought christian had recently posted a patch for this?  I must be confused.

[Christian Biesinger]

I didn't post a patch for this... I was mostly working on flexbox stuff lately.

[Eric Seidel (no email)]

Sorry, I'm thinking of Mithro.

[Mike Sherov]

Just chiming in here: Mithro is working on https://bugs.webkit.org/show_bug.cgi?id=29084, which hits similar code paths.

jQuery is eagerly awaiting both of these fixes ;-)

[Emil A Eklund]

29084 hasn't been updated in weeks. What do you want me to do with this bug? Mark it as a duplicate or land it in preparation for the other change?

[Tim 'mithro' Ansell]

Hi eae,

Sorry about blocking you, I've been away on holidays for the last two weeks (attending PyCon in the US). I'll be back onto this as my first priority after the Easter break (I may get to some of it today too).

In my bug I have feedback from Eric that I need to respond too and I'd like to merge your test into my patch to make sure it covers all your issues.

Looking at your code, your test seems to only cover a small section of these usage for these values. Does your patch break any of the other layout tests?

Sorry once again.

 - Tim 'mithro' Ansell

[Emil A Eklund]

(In reply to comment #24)
> Looking at your code, your test seems to only cover a small section of these usage for these values. Does your patch break any of the other layout tests?

No worries. The new tests covers how zoomed values are handled, it doesn't try to test any of the existing behavior as we already have other tests for that. The cr-linux and mac ews bots runs the tests and as you can see they are both green indicating that all tests pass.

[Radar WebKit Bug Importer]

<rdar://problem/25531120>

[Simon Fraser (smfr)]

This will be fixed by the patch in bug 29084.

*** This bug has been marked as a duplicate of bug 29084 ***