Bug 13343 - getComputedStyle returns wrong value for margin-right
Summary: getComputedStyle returns wrong value for margin-right
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 24511 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-04-12 12:22 PDT by Michael Mantel
Modified: 2011-11-29 10:34 PST (History)
22 users (show)

See Also:


Attachments
Testcase (224 bytes, text/html)
2007-04-12 12:24 PDT, Michael Mantel
no flags Details
Improved testcase (974 bytes, text/html)
2008-09-18 01:05 PDT, Sjoerd Mulder
no flags Details
Very simple testcase (644 bytes, text/html)
2009-12-09 05:49 PST, Nigel White
no flags Details
Proposed patch (16.01 KB, patch)
2011-02-04 08:05 PST, Jarred Nicholls
hyatt: review-
Details | Formatted Diff | Diff
CSS3 Version (15.08 KB, patch)
2011-02-04 08:21 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff
Proposed patch (15.99 KB, patch)
2011-02-10 12:21 PST, Jarred Nicholls
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (15.24 KB, patch)
2011-02-11 12:52 PST, Jarred Nicholls
simon.fraser: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (23.64 KB, patch)
2011-02-12 17:23 PST, Jarred Nicholls
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Mantel 2007-04-12 12:22:12 PDT
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.
Comment 1 Michael Mantel 2007-04-12 12:24:29 PDT
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".
Comment 2 Dave Hyatt 2007-04-12 13:30:19 PDT
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?
Comment 3 Michael Mantel 2007-04-12 14:11:34 PDT
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".

Comment 4 Alex Russell 2007-06-27 16:32:21 PDT
also reported in Dojo: http://trac.dojotoolkit.org/ticket/3515
Comment 5 Alexey Proskuryakov 2007-10-05 05:17:26 PDT
Marking confirmed per the above comments.
Comment 6 Sjoerd Mulder 2008-02-11 05:24:59 PST
Also found / reported in Backbase
Comment 7 Paul Ovchar 2008-05-22 08:35:33 PDT
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.
Comment 8 Jon Emerson 2008-08-19 02:47:18 PDT
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
Comment 9 Jon Emerson 2008-08-19 02:50:04 PDT
Oops I replied to the wrong bug.  Please ignore my comments here.  Sorry!
Comment 10 Sjoerd Mulder 2008-09-18 01:05:56 PDT
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. "
Comment 11 John-David Dalton 2009-07-03 07:32:03 PDT
Any news on this bug being fixed ?
Comment 12 Nigel White 2009-12-02 04:11:41 PST
Yes, get it fixed!

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

With a simple testcase attached
Comment 13 Nigel White 2009-12-09 05:49:49 PST
Created attachment 44533 [details]
Very simple testcase

This just shows an incorrect margin from computer style.
Comment 14 Alexander Willner 2010-04-17 08:29:43 PDT
According Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=23816
Comment 15 Alexey Proskuryakov 2010-04-17 11:54:32 PDT
*** Bug 24511 has been marked as a duplicate of this bug. ***
Comment 16 Jake Archibald 2010-04-26 08:36:13 PDT
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.
Comment 17 Nigel White 2010-04-27 00:36:07 PDT
Temporarily setting it to inline-block works too. It's just a bodge though. As you say, can someone take a look?
Comment 18 Jarred Nicholls 2011-02-04 06:19:06 PST
Will be posting a proposed patch with additional/modified layout tests sometime today.
Comment 19 Jarred Nicholls 2011-02-04 08:05:46 PST
Created attachment 81218 [details]
Proposed patch
Comment 20 Jarred Nicholls 2011-02-04 08:21:46 PST
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.
Comment 21 Alexander Willner 2011-02-04 08:25:24 PST
@Jarred: nice. Does the test case "crbug_23816.html" (see comment #14) pass?
Comment 22 Jarred Nicholls 2011-02-04 08:30:51 PST
(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 :)
Comment 23 Jarred Nicholls 2011-02-04 08:53:32 PST
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 :)
Comment 24 Simon Fraser (smfr) 2011-02-10 10:22:08 PST
If the specs are unclear, you should raise the issue on the www-style mailing list.
Comment 25 Jarred Nicholls 2011-02-10 10:27:33 PST
(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.
Comment 26 Dave Hyatt 2011-02-10 12:06:48 PST
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.
Comment 27 Jarred Nicholls 2011-02-10 12:21:43 PST
Created attachment 82023 [details]
Proposed patch

removed blank lines, updated changelog date
Comment 28 Simon Fraser (smfr) 2011-02-11 10:43:53 PST
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.
Comment 29 Jarred Nicholls 2011-02-11 10:51:37 PST
(In reply to comment #28)
> document.write is evil.

Will update, I adapted the existing test.  I should have updated it.
Comment 30 Jarred Nicholls 2011-02-11 12:52:49 PST
Created attachment 82162 [details]
Proposed patch

improved tests
Comment 31 WebKit Commit Bot 2011-02-12 13:03:44 PST
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
Comment 32 Jarred Nicholls 2011-02-12 13:53:17 PST
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.
Comment 33 Jarred Nicholls 2011-02-12 17:23:48 PST
Created attachment 82248 [details]
Proposed patch

Update expected margin values on computed-style-without-renderer-expected.txt for all platforms.
Comment 34 Nigel White 2011-02-13 07:41:38 PST
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.
Comment 35 WebKit Commit Bot 2011-02-13 11:23:37 PST
Comment on attachment 82248 [details]
Proposed patch

Clearing flags on attachment: 82248

Committed r78436: <http://trac.webkit.org/changeset/78436>
Comment 36 WebKit Commit Bot 2011-02-13 11:23:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 37 Mike Sherov 2011-11-23 18:44:18 PST
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.
Comment 38 Julien Chaffraix 2011-11-29 10:04:06 PST
> 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!
Comment 39 Mike Sherov 2011-11-29 10:34:52 PST
new ticket: https://bugs.webkit.org/show_bug.cgi?id=73334