Bug 73334

Summary: getComputedStyle returns wrong value for margin-*
Product: WebKit Reporter: Mike Sherov <mike.sherov@gmail.com>
Component: CSSAssignee: Jarred Nicholls <jarred@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: Alexander.Mitin@gmail.com, darin@apple.com, iben@spilled-milk.com, jarred@webkit.org, jchaffraix@webkit.org, macpherson@chromium.org, paulirish@chromium.org, rniwa@webkit.org, shanestephens@google.com, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 118936    
Attachments:
Description Flags
Patch
none
Patch none

Description From 2011-11-29 10:34:08 PST
Since the change in: https://bugs.webkit.org/show_bug.cgi?id=13343 , getComputedStyle() returns the wrong value for margins. 

This is now against how IE9, FF3.6+, and Opera implements getComputedStyle() for margins. Open up the following jsfiddle in FF3.6+, Opera, IE9, and Chrome: 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 the comment in https://bugs.webkit.org/show_bug.cgi?id=13343 "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.

I understand that https://bugs.webkit.org/show_bug.cgi?id=13343 was implemented so that when the margin is not-defined it doesn't report the distance between the margin and it's parents edge. However, I believe there is a middle ground that when a pixel is explicitly specified, it should convert to px. It should still report 0px if unspecified (although unfortunately, IE9 doesn't do that). The following fiddles deals illustrate that case: http://jsfiddle.net/pyeNX/1/
------- Comment #1 From 2011-11-30 19:40:27 PST -------
Sorry, I want to clarify a few of my original comments:

"However, I believe there is a middle ground that when a pixel is explicitly specified, it should convert to px" should read "However, I believe there is a middle ground that when a percent is explicitly specified, it should convert to px."

I also want to bring up something I just brought up to the www-style mailing list: There is confusion amongst the browsers over what value to report in getComputedStyle() for margins when the value is set to auto. Now, it *may* be that every browser except IE9 is correct in reporting 0px for margin-right when margin is set to auto, as it is here: http://jsfiddle.net/pyeNX/13/ . However, isn't the margin here, expressed as pixels, greater than 0? Now, I know everyone but IE9 reports 0px, but I believe that "used value" would dictate this margin to be the actual pixel value used, which is clearly greater than 0! That's how the inner div gets centered!

I would argue that the correct behavior for margin should be as it is in IE9 (contrary to my previous comment):
if a unit (including percent) is specified: convert to pixels
when auto is specified: used value in pixels (as that's what the margin REALLY is)
when zero is specified: 0px
------- Comment #2 From 2011-12-01 17:51:19 PST -------
> I would argue that the correct behavior for margin should be as it is in IE9 (contrary to my previous comment)

The best way forward would be to ask for this very behavior to be specified / clarified in the specification by posting on the www-style list. Make sure you post the link to the relevant thread here as it would help the person correcting our implementation.
------- Comment #3 From 2011-12-01 19:00:45 PST -------
(In reply to comment #2)
> > I would argue that the correct behavior for margin should be as it is in IE9 (contrary to my previous comment)
> 
> The best way forward would be to ask for this very behavior to be specified / clarified in the specification by posting on the www-style list. Make sure you post the link to the relevant thread here as it would help the person correcting our implementation.

Julien, thanks for the feedback. I emailed that list last night, hopefully my message will be posted soon. I referenced this thread. Thanks! What are your thoughts on the issue?
------- Comment #4 From 2011-12-03 10:17:23 PST -------
The correct behavior is clearly IE9, based on CSSOM's definition of resolved values.  The original issue in bug #13343 was that the margin being returned was the space between the element's right-edge and its parent's right-edge, and not what the user supplied (if anything).

While correcting that, we went ahead and complied with the older CSS21 and CSS3 editor drafts, which stated to return the user "specified value" - specifically, if the user supplied a % then a % would be returned.  I will revert this because CSSOM now clearly defines that the resolved/used value should be returned for several of the box model properties like margin-*, width, height, etc.  This not only goes for a supplied % unit, but also for "auto".

I just need to fix some tests and I'll submit the patch later this evening.
------- Comment #5 From 2011-12-03 10:49:25 PST -------
Jarred,

Awesome. Glad we see eye to eye on this, but I do think the CSSOM needs some revising to clearly state what do to for "auto". I submitted this to the www-style group: http://lists.w3.org/Archives/Public/www-style/2011Dec/0110.html

Is there a place that it's already clearly defined that used value for "auto" for box model elements attributes like margin clearly means >0px? Obviously, I want to go to mozilla and opera next with this, and get them to change too, so I think it needs to be crystal clear. If so, link?

Thoughts?
------- Comment #6 From 2011-12-03 10:54:13 PST -------
Actually, here it is, SORT OF:

http://www.w3.org/TR/css3-values/#stages-examples

This example shows that "auto" for "width" needs to be the used value, which is >0. But it does not specifically mention it for margin. I wonder if that's a source of confusion.
------- Comment #7 From 2011-12-03 11:06:39 PST -------
That's definitely it Mike.  The definition of the used value is clear, and the definition of resolved values is also clear: http://dev.w3.org/csswg/cssom/#dom-window-getcomputedstyle

The spec states the legacy behavior of getComputedStyle was to return computed values, but now they have the concept of resolved values.  And for those properties listed, the resolved value === used value (for most of them, only if display != none).

Firefox/Opera have the same behavior they've had for almost 2 years - this is a relatively recent change in the spec for getComputedStyle, so Firefox and Opera are just behind IMHO, and IE9 happens to match.
------- Comment #8 From 2011-12-05 14:45:57 PST -------
Created an attachment (id=117942) [details]
Patch
------- Comment #9 From 2011-12-05 14:49:27 PST -------
(From update of attachment 117942 [details])
Given the change there is not enough test coverage. For example, to test the isBox predicate, we need tests with different display values.
------- Comment #10 From 2011-12-05 14:50:13 PST -------
(From update of attachment 117942 [details])
Thanks for contributing the patch. This does seem like a good code change. But we need some more test coverage.
------- Comment #11 From 2011-12-05 14:52:22 PST -------
(In reply to comment #10)
> (From update of attachment 117942 [details] [details])
> Thanks for contributing the patch. This does seem like a good code change. But we need some more test coverage.

Agreed, will do.
------- Comment #12 From 2011-12-05 14:56:39 PST -------
"And for those properties listed, the resolved value === used value (for most of them, only if display != none)." needs to definitely be tested. But thanks for the proposed patch Jarred! 

http://bugs.jquery.com/ticket/10639 thanks you too! The workaround is just slow and nasty: https://github.com/jquery/jquery/pull/616/files#L0R280
------- Comment #13 From 2011-12-05 14:56:57 PST -------
While I am generally enthusiastic about this, I am a little worried about moving out in front of the other browsers on this change, and also a bit concerned that we are doing this for one property, when presumably this affects many others too.
------- Comment #14 From 2011-12-05 15:04:46 PST -------
Darin, there are two parts here. The first part, returning % for margin where all the other browsers already return px, shouldn't be in dispute, right?

As far as the margin:auto returning >0px, IE9 does it this way, and I've got the ball rolling with Opera and FF for this:
Opera: https://twitter.com/#!/mikesherov/status/143056305492471808
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=381328

With regards to the concerns for doing this only on margins, CSSOM used values specifically prescribes to use "used value" for box model attributes that have "auto" specified, which in this case is only margins.
------- Comment #15 From 2011-12-05 15:05:38 PST -------
OK
------- Comment #16 From 2011-12-05 15:06:08 PST -------
(In reply to comment #12)
> "And for those properties listed, the resolved value === used value (for most of them, only if display != none)." needs to definitely be tested. But thanks for the proposed patch Jarred! 
> 

the "without-renderer" test covers this scenario but not 100% (it only checks margin-left and none of the others).  I'll write complete tests for margin-*.

(In reply to comment #13)
> While I am generally enthusiastic about this, I am a little worried about moving out in front of the other browsers on this change, and also a bit concerned that we are doing this for one property, when presumably this affects many others too.

This would make us behave the same as IE9/IE10.  margin-* was the only set with major compliance failures inconsistent with the other properties; our width, height, line-height, and padding-* properties are already compliant AFAIK but I won't say for certain.  If we can land margin-* with 100% test coverage I can do the same analysis and test coverage for the other properties.
------- Comment #17 From 2011-12-05 22:59:30 PST -------
Created an attachment (id=117992) [details]
Patch
------- Comment #18 From 2011-12-06 09:43:37 PST -------
(From update of attachment 117992 [details])
Clearing flags on attachment: 117992

Committed r102149: <http://trac.webkit.org/changeset/102149>
------- Comment #19 From 2011-12-06 09:43:47 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #20 From 2012-12-05 13:48:35 PST -------
Apologies for my question, but I have just stumbled over this case number as I have been experiencing an issue that I was not expecting. This thread and the one it stemmed from, has lead me to believe this concerns my issue. However I have noticed this bug has been marked resolved which is where my confusion is stemming from.

After reading the W3C specs I was expecting a block level value that was over-constrained to return to me a negative value, in regards to the right-margin.

However it's returning the value I supplied it via CSS.



in the following example:

http://jsfiddle.net/fezec/EVuHR/1/

The provided url makes use of a DIV container and a PARAGRAPH child element.
The DIV width is set to 500px and the PARAGRAPH is given 100px MARGIN and is set to be 400px in WIDTH.  The expected result is that the right margin would become 0px, as this is the only way the Block Element formula is satisfied.

Yet when viewing the calculated and layout within Firebug, the margin right displays the styled value, NOT the used value.


Am I missing something? Have I learned enough information to merely hang myself with it?
------- Comment #21 From 2012-12-10 11:40:04 PST -------
> Am I missing something? Have I learned enough information to merely hang myself with it?

Sorry but bugzilla is not a place for css questions, especially bugs closed for a long time.

If you think this is a WebKit bug, filing a _new_ bug is your best option. AFAICT the output of getComputedStyle is not totally standardized so you can ask www-style@w3.org for clarification on what is to be expected from the standard's perspective.
------- Comment #22 From 2013-07-22 16:56:42 PST -------
(In reply to comment #20)
> Apologies for my question, but I have just stumbled over this case number as I have been experiencing an issue that I was not expecting. This thread and the one it stemmed from, has lead me to believe this concerns my issue. However I have noticed this bug has been marked resolved which is where my confusion is stemming from.
> 
> After reading the W3C specs I was expecting a block level value that was over-constrained to return to me a negative value, in regards to the right-margin.

Indeed http://www.w3.org/TR/CSS2/visudet.html#blockwidth seems to indicate that's what we're supposed to do.

> The provided url makes use of a DIV container and a PARAGRAPH child element.
> The DIV width is set to 500px and the PARAGRAPH is given 100px MARGIN and is set to be 400px in WIDTH.  The expected result is that the right margin would become 0px, as this is the only way the Block Element formula is satisfied.
> 
> Yet when viewing the calculated and layout within Firebug, the margin right displays the styled value, NOT the used value.
> 
> Am I missing something? Have I learned enough information to merely hang myself with it?

I don't think you're missing anything.  However, I've tested this on Firefox 22 and Internet Explorer 10 and they both return 100px for getComputedStyle(document.querySelector('p')).marginRight in the example you provided.  Given that, we should probably amend the specification instead.