Bug 105979 - getComputedStyle() should return zoom-adjusted offset values
Summary: getComputedStyle() should return zoom-adjusted offset values
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Major
Assignee: Nobody
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks: 110007
  Show dependency treegraph
 
Reported: 2013-01-02 17:45 PST by Adam Craven
Modified: 2016-04-04 11:29 PDT (History)
18 users (show)

See Also:


Attachments
Test Case (2.00 KB, text/html)
2013-01-02 17:45 PST, Adam Craven
no flags Details
Patch (5.39 KB, patch)
2013-02-15 17:15 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (12.82 KB, patch)
2013-02-19 15:22 PST, Emil A Eklund
darin: review-
Details | Formatted Diff | Diff
Patch (13.94 KB, patch)
2013-03-08 15:39 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Craven 2013-01-02 17:45:20 PST
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
Comment 1 Adam Craven 2013-01-02 17:59:38 PST
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%...
Comment 2 Mike Sherov 2013-01-02 18:03:37 PST
Please correct the title to getComputedStyle please. There's a typo.
Comment 3 Adam Craven 2013-01-02 18:18:39 PST
Sorry, typo fixed...
Comment 4 Emil A Eklund 2013-02-15 16:11:10 PST
Seems wrong. Thanks for the report and the test case.
Comment 5 Emil A Eklund 2013-02-15 17:15:43 PST
Created attachment 188671 [details]
Patch
Comment 6 Darin Adler 2013-02-16 12:43:11 PST
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.
Comment 7 Emil A Eklund 2013-02-19 10:43:23 PST
Good point, I'll make sure to add tests for percentage values.
Comment 8 Emil A Eklund 2013-02-19 15:22:30 PST
Created attachment 189181 [details]
Patch
Comment 9 Emil A Eklund 2013-02-21 14:40:36 PST
Darin, when you get a chance could you please take another look?
Comment 10 Emil A Eklund 2013-02-22 10:30:23 PST
Ping?
Comment 11 Darin Adler 2013-02-26 09:21:49 PST
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.
Comment 12 Darin Adler 2013-02-26 09:22:51 PST
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.
Comment 13 Emil A Eklund 2013-02-26 13:27:00 PST
Oh, of course. That makes more sense. Thank you!
Comment 14 Mike Sherov 2013-03-08 10:19:29 PST
Will this patch also fix the values for offsetWidth and offsetHeight?
Comment 15 Mike Sherov 2013-03-08 10:20:57 PST
offsetWidth and offsetHeight are manifesting themselves as http://bugs.jquery.com/ticket/13543 in jQuery Core.
Comment 16 Emil A Eklund 2013-03-08 10:25:13 PST
(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.
Comment 17 Emil A Eklund 2013-03-08 15:39:19 PST
Created attachment 192295 [details]
Patch
Comment 18 Emil A Eklund 2013-03-11 13:53:37 PDT
(In reply to comment #11)

Found a better way to do it, thanks. Please take another look.
Comment 19 Eric Seidel 2013-03-15 12:17:38 PDT
I thought christian had recently posted a patch for this?  I must be confused.
Comment 20 Christian Biesinger 2013-03-15 13:25:42 PDT
I didn't post a patch for this... I was mostly working on flexbox stuff lately.
Comment 21 Eric Seidel 2013-03-15 13:50:47 PDT
Sorry, I'm thinking of Mithro.
Comment 22 Mike Sherov 2013-03-15 13:53:48 PDT
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 ;-)
Comment 23 Emil A Eklund 2013-03-27 03:32:03 PDT
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?
Comment 24 Tim 'mithro' Ansell 2013-03-27 20:48:42 PDT
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
Comment 25 Emil A Eklund 2013-03-28 03:03:14 PDT
(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.
Comment 26 Radar WebKit Bug Importer 2016-04-04 10:43:31 PDT
<rdar://problem/25531120>