Bug 73334

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

Mike Sherov
Reported 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/
Attachments
Patch (8.58 KB, patch)
2011-12-05 14:45 PST, Jarred Nicholls
no flags
Patch (19.67 KB, patch)
2011-12-05 22:59 PST, Jarred Nicholls
no flags
Mike Sherov
Comment 1 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
Julien Chaffraix
Comment 2 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.
Mike Sherov
Comment 3 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?
Jarred Nicholls
Comment 4 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.
Mike Sherov
Comment 5 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?
Mike Sherov
Comment 6 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.
Jarred Nicholls
Comment 7 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.
Jarred Nicholls
Comment 8 2011-12-05 14:45:57 PST
Darin Adler
Comment 9 2011-12-05 14:49:27 PST
Comment on attachment 117942 [details] Patch Given the change there is not enough test coverage. For example, to test the isBox predicate, we need tests with different display values.
Darin Adler
Comment 10 2011-12-05 14:50:13 PST
Comment on attachment 117942 [details] Patch Thanks for contributing the patch. This does seem like a good code change. But we need some more test coverage.
Jarred Nicholls
Comment 11 2011-12-05 14:52:22 PST
(In reply to comment #10) > (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. Agreed, will do.
Mike Sherov
Comment 12 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
Darin Adler
Comment 13 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.
Mike Sherov
Comment 14 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.
Darin Adler
Comment 15 2011-12-05 15:05:38 PST
OK
Jarred Nicholls
Comment 16 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.
Jarred Nicholls
Comment 17 2011-12-05 22:59:30 PST
Jarred Nicholls
Comment 18 2011-12-06 09:43:37 PST
Comment on attachment 117992 [details] Patch Clearing flags on attachment: 117992 Committed r102149: <http://trac.webkit.org/changeset/102149>
Jarred Nicholls
Comment 19 2011-12-06 09:43:47 PST
All reviewed patches have been landed. Closing bug.
iben
Comment 20 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?
Julien Chaffraix
Comment 21 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.
Ryosuke Niwa
Comment 22 2013-07-22 16:56:42 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.