Bug 13343 – getComputedStyle returns wrong value for margin-right

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

[Michael Mantel]

getComputedStyle seems to return the wrong marginRight value for almost any block element that's not floated.  Looks like it returns the distance from the element's right edge to its parent's right edge.

[Michael Mantel]

Created attachment 14022 [details]
Testcase

This testcase shows a div with a width of 100px.  getComputedStyle reports its margin-right as the body's width minus 100, e.g. "906px".

[Dave Hyatt]

The behavior is correct as far as I know.  margin-left + width + margin-right = content width of containing block according to CSS2.1.  Have you checked Firefox?

[Michael Mantel]

Firefox and Opera both return 0px.  IE7 returns "auto".

The current behavior makes it hard to do dynamic resizing, like "make the parent element just wide enough to contain its children".

[Alex Russell]

also reported in Dojo: http://trac.dojotoolkit.org/ticket/3515

[Alexey Proskuryakov]

Marking confirmed per the above comments.

[Sjoerd Mulder]

Also found / reported in Backbase

[Paul Ovchar]

Incorrect behavior still happens. In my case: div (InnerDiv) placed into floated div with overflow hidden and fixed size. Inner div has paddings. In IE6/7, Opera 9 and Firefox getComputedStyle of InnerDiv for marginRight returns 0, but Safari returns negative number.

I reasearch that problem: IE6/7, Opera 9 and Firefox works correct and calculate full width of InnerDiv as Width+marginRight+marginLeft
but Safari calculate it as Width+marginRight+marginLeft+paddingRight+paddingLeft.

So, bug is not fixed.

[Jon Emerson]

In our application, we render our pages off-screen before attaching them to the document because in a couple popular browsers the performance of off-screen (detached) DOM creation is an order of magnitude greater than on-screen (attached) DOM creation.  Part of our rendering involves manually setting the width and heights on certain elements to fill the screen.  (We do this manually for cross-browser compatibility.)  If we can't know the margins of elements, then we can't size other elements appropriately.

Please fix this bug!  Thanks.
Jon

[Jon Emerson]

Oops I replied to the wrong bug.  Please ignore my comments here.  Sorry!

[Sjoerd Mulder]

Created attachment 23523 [details]
Improved testcase

I think the behavior in Safari is incorrect according to: 
http://www.w3.org/TR/CSS21/box.html#propdef-margin-right.

It says the computed value of margin-right / left is the percentage as specified or the absolute length.  So when i specify the margin, it should return that margin, the initial value (according to the specs) of the margin is 0. So by default it should return 0.

I think the behavior in Safari is correct according to: 
http://www.w3.org/TR/CSS21/visudet.html#blockwidth

"If 'width' is not 'auto' and 'border-left-width' + 'padding-left' + 'width' + 'padding-right' + 'border-right-width' + scrollbar width (if any) (plus any of 'margin-left' or 'margin-right' that are not 'auto') is larger than the width of the containing block, then any 'auto' values for 'margin-left' or 'margin-right' are, for the following rules, treated as zero.

If all of the above have a computed value other than 'auto', the values are said to be "over-constrained" and one of the used values will have to be different from its computed value. If the 'direction' property of the containing block has the value 'ltr', the specified value of 'margin-right' is ignored and the value is calculated so as to make the equality true. If the value of 'direction' is 'rtl', this happens to 'margin-left' instead. "

[John-David Dalton]

Any news on this bug being fixed ?

[Nigel White]

Yes, get it fixed!

There's a duplicate here: https://bugs.webkit.org/show_bug.cgi?id=13343

With a simple testcase attached

[Nigel White]

Created attachment 44533 [details]
Very simple testcase

This just shows an incorrect margin from computer style.

[Alexander Willner]

According Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=23816

[Alexey Proskuryakov]

*** Bug 24511 has been marked as a duplicate of this bug. ***

[Jake Archibald]

This is STILL an issue. Any chance someone could take a look?

A workaround is setting the element to display:none, measuring the margin then setting display back to block.

[Nigel White]

Temporarily setting it to inline-block works too. It's just a bodge though. As you say, can someone take a look?

[Jarred Nicholls]

Will be posting a proposed patch with additional/modified layout tests sometime today.

[Jarred Nicholls]

Created attachment 81218 [details]
Proposed patch

[Jarred Nicholls]

Created attachment 81220 [details]
CSS3 Version

This patch is compliant with CSS3 for computed margin values.  If "auto" is the supplied value, it will return "auto" as the computed value, per http://www.w3.org/TR/css3-box/#the-margin.  We can have a popular vote as to which patch (this or the previous) should be the fix.

[Alexander Willner]

@Jarred: nice. Does the test case "crbug_23816.html" (see comment #14) pass?

[Jarred Nicholls]

(In reply to comment #21)
> @Jarred: nice. Does the test case "crbug_23816.html" (see comment #14) pass?

Yes it does.  I am summing up my opinions on some compliance issues/inconsistencies...expect a pretty lengthy follow up comment shortly :)

[Jarred Nicholls]

Bare with me, this will be a lengthy comment, but I just wanted to weigh in my opinion on this bug.

My feeling is that the original behavior is the correct one, OR the current behavior of width/height (among others) is wrong.  We can use CSS 2.1 (http://www.w3.org/TR/CSS21/visudet.html#the-width-property & http://www.w3.org/TR/CSS21/box.html#margin-properties) or CSS 3 (http://www.w3.org/TR/css3-box/) as our guide.

Margin's computed values are said to be this: "the percentage as specified or the absolute length or ‘auto’".  Some have understood "absolute length" to be what was specified (including Firefox, Opera, IE, and everyone CC'ed on this bug).  The original behavior understood "absolute length" to mean "the used value".

Now if we take width/height's computed value definition: "the percentage or ‘auto’ as specified or the absolute length; ‘auto’ if the property does not apply".  You'll notice that "absolute length" is again used, when a percentage or auto isn't specified.  The difference here is that all browsers, including WebKit, always return "the used value" as the computed width.  So if we specified "auto", "50%", or "500px", we always get the used value in fixed pixels back.  If our container block is 1000px wide, the computed value for those 3 cases would be "1000px", "500px", and "500px" respectively.

So currently, margin-left: auto; and margin-right: auto; will return a computed value of the "used value" in fixed pixels.  If this isn't right, then width/height aren't right either.

So the question is, how do we define "absolute length".  Is it the absolute fixed length specified, or is it the used value?  It seems like the world has come to a conclusion that it's the specified value.  In the case margin is not specified, an initial value of 0 is used.  In the case of width/height though, it would appear that no browser is compliant.

I would expect width: auto; to return "auto", width: 50%; to return "50%", and width: 500px; to return "500px", and an undefined width to return "auto".  The CSS3 patch I've attached to this bug will make margin's compliant to CSS3, but it seems width/height (and several others) need fixing too.

So what does everyone think?  Do we go full compliant, or do we just try to act like the rest of the browsers?  Thanks for reading my rant :)

[Simon Fraser (smfr)]

If the specs are unclear, you should raise the issue on the www-style mailing list.

[Jarred Nicholls]

(In reply to comment #24)
> If the specs are unclear, you should raise the issue on the www-style mailing list.

Sure, my apologies.  Nevertheless, I think the first "Proposed patch" is the one to go with, as it will match every other major browser out there, and match CSS 2.1 for margins.  It should be ready to review.

[Dave Hyatt]

Comment on attachment 81218 [details]
Proposed patch

Can you get rid of the extraneous lines... there's an extra blank line in every case statement now.

[Jarred Nicholls]

Created attachment 82023 [details]
Proposed patch

removed blank lines, updated changelog date

[Simon Fraser (smfr)]

Comment on attachment 82023 [details]
Proposed patch

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

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-margin-auto.html:45
> +    document.write("width: " + window.getComputedStyle(foodiv, null).getPropertyValue("width"));
> +    document.write( "<br>");
> +    document.write("height: " + window.getComputedStyle(foodiv, null).getPropertyValue("height"));
> +    document.write( "<br>");
> +    document.write("margin-left: " + window.getComputedStyle(foodiv, null).getPropertyValue("margin-left"));
> +    document.write( "<br>");
> +    document.write("margin-right: " + window.getComputedStyle(foodiv, null).getPropertyValue("margin-right"));
> +    document.write( "<br>");
> +    document.write("margin-top: " + window.getComputedStyle(foodiv, null).getPropertyValue("margin-top"));
> +    document.write( "<br>");
> +    document.write("margin-bottom: " + window.getComputedStyle(foodiv, null).getPropertyValue("margin-bottom"));
> +    document.write( "<br>");
> +    document.write("padding-left: " + window.getComputedStyle(foodiv, null).getPropertyValue("padding-left"));
> +    document.write( "<br>");
> +    document.write("left: " + window.getComputedStyle(foodiv, null).getPropertyValue("left"));
> +    document.write( "<br>");

Please look at how script tests are done (/Tools/Scripts/make-script-test-wrappers and examples in fast/css/script-tests).

document.write is evil.

[Jarred Nicholls]

(In reply to comment #28)
> document.write is evil.

Will update, I adapted the existing test.  I should have updated it.

[Jarred Nicholls]

Created attachment 82162 [details]
Proposed patch

improved tests

[WebKit Commit Bot]

Comment on attachment 82162 [details]
Proposed patch

Rejecting attachment 82162 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
.................................................................................................................................................
fast/css/content ..
fast/css/counters .......................
fast/css/getComputedStyle .......
fast/css/getComputedStyle/computed-style-without-renderer.html -> failed

Exiting early after 1 failures. 6786 tests run.
150.97s total testing time

6785 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7878044

[Jarred Nicholls]

This must be a very recently added test.  I haven't looked at it yet but it's probably compensating for the old behavior.  Will take a look when at a machine.

[Jarred Nicholls]

Created attachment 82248 [details]
Proposed patch

Update expected margin values on computed-style-without-renderer-expected.txt for all platforms.

[Nigel White]

I agree with the proposed patch.

Having getComputedStyle return the size of the gap between the element's edge and the associated edge of its container is just not a *useful* operation.

Intuitively, getComputedStyle is used for finding out the end result of any CSS rules applied to the element at that time. You are asking what 'margin-right' style has been arrived at by evaluating all matched style rules.

[WebKit Commit Bot]

Comment on attachment 82248 [details]
Proposed patch

Clearing flags on attachment: 82248

Committed r78436: <http://trac.webkit.org/changeset/78436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mike Sherov]

This is now against how IE9, FF3.6+, and Opera implementation of getComputedStyle() for margins. Open up the following jsfiddle in FF3.6+, Opera, and IE9: http://jsfiddle.net/u4F8m/9/

It is also against the CSSOM editor's draft: http://dev.w3.org/csswg/cssom/#resolved-value
http://www.w3.org/TR/css3-values/


It also now makes margin inconsistent with how padding, width, and height work for resolved values in Webkit itself.

I would also argue that "Having getComputedStyle return the size of the gap between the element's edge and the associated edge of its container is just not a *useful* operation." is not true. It's very useful to now the exact pixel dimensions of a box. Why would I ever want to know that a margin is "10%"? 10% of what? The actual pixel value used is way more relevant.

Please consider reopening and reverting this change.

[Julien Chaffraix]

> Please consider reopening and reverting this change.

Could you please open a new bug? Your comment makes sense and needs to be investigated. However if posted on a closed bug, it is lost for WebKit developers as it won't show up on most bugzilla queries. Thanks!

[Mike Sherov]

new ticket: https://bugs.webkit.org/show_bug.cgi?id=73334