Closed Bug 520992 Opened 15 years ago Closed 11 years ago

getComputedStyle / height ignoring -moz-box-sizing:border-box

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: danswer, Assigned: jwir3)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)

If elem has a height set with elem.style.height = "200px";
then I would expect that same value to be returned with

var cs = document.defaultView.getComputedStyle(elem,null);
alert (cs.getPropertyValue('height'));

even in the presence elem.style['MozBoxSizing'] = "border-box";

However, the value returned is actually returned is the height of the content box.

Reproducible: Always

Steps to Reproduce:
The following javascript code demonstrates the problem:

var div=document.createElement('DIV');
div.style['MozBoxSizing'] = "border-box";
div.style.border = "7px solid red";
div.style['borderBottom'] = "13px solid green";
div.style.height = "200px";

var cs = document.defaultView.getComputedStyle(div,null);
div.innerHTML =  "Height before insertion: " +
                 cs.getPropertyValue('height');

document.body.appendChild(div);
div.innerHTML += "<br>Height after insertion: " +
                 cs.getPropertyValue('height');

div.innerHTML += "<br><br>The same value should be showing";
Actual Results:  
On the second line, the border widths are subtracted off to give 180px.

Expected Results:  
Both lines should show 200px,

I would expect that if I use the value obtained from getComputedStyle and plug it in again, then I should not experience any change in the displayed object.  This is not the case here.  If you do something like
elem.style.height = document.defaultView.getComputedStyle(elem,null).getPropertyValue('height')
then the element will shrink if it has a border and its box sizing is set to border-box (as opposed to the default content-box).

Csaba Gabor from Vienna
This attachment contains the javascript in the original report.  A div is created and then its -moz-box-sizing style property is set to border-box.  A border is set and then a height assigned.  Before rendering, getComputedStyle returns the set height.  After rendering (by virtue of document.appendChild), it seems to be the height of the content box that is returned.  This at best seems counter intuitive.
QA Contact: general → style-system
Blocks: 243412
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file simple testcase
Attachment #405046 - Attachment is obsolete: true
OS: Windows Vista → All
Attached patch b520992 (obsolete) — Splinter Review
This is a patch that fixes the behavior for the given testcase. dbaron requested that I look through areas that call getComputedStyle() and use the height, in order to make sure that we aren't using the wrong height internally. 

There are a couple of questions I have on this, which is why I'm not requesting review yet, just feedback.

I noticed that in html.css, we're making table's have a box-sizing of 'border-box' as a default style, so the addition of this code makes a few tests expect an incorrect size. There are probably at least three solutions to this problem - one would be to remove the style from html.css. I think this is an incorrect approach, because web authors are probably expecting the default behavior currently. Another approach would be to calculate the computed height/width differently, depending on whether or not moz-box-sizing is specified in something other than the default styles (browser.css, html.css, etc...). I think this would add too much unnecessary complexity, and would likely lead to problems in the future. 

The simplest, and (IMHO) best approach is probably to simply change the tests so they expect a different size. This doesn't eliminate the problem, though, that it might change behavior web authors are currently expecting. I do believe we're making it more correct, though, and if they want to override moz-box-sizing for their content, they are free to do so.

The other, more pressing issue I see is that inside of DoGetHeight() and DoGetWidth() in nsComputedDOMStyle, we have two similar branches of an if conditional. One checks to see if we should compute the width/height, based on whether the style has an associated inner frame. If it doesn't, then we utilize the height of the containing block. I didn't add in the height/width of the border and padding in this case, because I felt as though it should be retrieved from the computation of height/width of the containing block. I'm not sure this is correct, though. I'm not sure when this branch of code would be executed, and perhaps it's a situation where the containing block would not go through the same code path to compute it's height/width. If that's the case, I'm wondering if perhaps we should factor in a similar calculation of the border and padding for the containing block.

Any suggestions and insight you can give, bz, would be really appreciated!
Assignee: nobody → sjohnson
Attachment #731180 - Flags: feedback?(bzbarsky)
Comment on attachment 731180 [details] [diff] [review]
b520992

> The simplest, and (IMHO) best approach is probably to simply change the tests so they
> expect a different size.

That sounds right to me for the table thing.

> I'm not sure this is correct, though. I'm not sure when this branch of code would be
> executed

Most likely when you get computed width on a display:none element.

That's getting things from style data directly, so it doesn't need to be adjusted in any way, because box-sizing doesn't affect computed styles, which is what those are.

And the code to compute the size of the containing block typically doesn't need any box-sizing adjustments, since it's using the actual frame size.

The two obvious issues I see with the patch are:

1) It doesn't handle "box-sizing: padding-box".  
2) It should probably use the used border and padding from mInnerFrame rather
   than things from style structs.
Attachment #731180 - Flags: feedback?(bzbarsky) → feedback+
Attached patch b520992Splinter Review
(In reply to Boris Zbarsky (:bz) from comment #5)
> That sounds right to me for the table thing.

Turns out it was slightly more than just tables, but it was still pretty isolated. I've updated the tests to account for these changes and included them in a new patch.

> Most likely when you get computed width on a display:none element.
> 
> That's getting things from style data directly, so it doesn't need to be
> adjusted in any way, because box-sizing doesn't affect computed styles,
> which is what those are.

Ok, that's good to know.

> And the code to compute the size of the containing block typically doesn't
> need any box-sizing adjustments, since it's using the actual frame size.

Also great to know, thanks.

> The two obvious issues I see with the patch are:
> 
> 1) It doesn't handle "box-sizing: padding-box".  
> 2) It should probably use the used border and padding from mInnerFrame rather
>    than things from style structs.

I believe I've fixed these things, so hopefully this meets your standards. :) Thanks!
Attachment #731180 - Attachment is obsolete: true
Attachment #733127 - Flags: review?(bzbarsky)
Comment on attachment 733127 [details] [diff] [review]
b520992

>+++ b/layout/style/nsComputedDOMStyle.cpp
>+  // If we are using -moz-box-sizing: border-box, then we want the width
>+  // of the entire box, not just the content.

This should probably just say that we want to include the width of whatever parts 'width' controls, depending on the value of box-sizing.

>+  if (stylePos->mBoxSizing == NS_STYLE_BOX_SIZING_BORDER) {

This might be simpler as:

  switch (stylePos->mBoxSizing) {
    case NS_STYLE_BOX_SIZING_BORDER:
      borderPadding += mInnerFrame->GetUsedBorder().LeftRight();
      // fall through
    case NS_STYLE_BOX_SIZING_PADDING

etc.

I think I would also prefer to have a single function that just returns an nsMargin representing the box-sizing adjustment that the callers can then use LeftRight() or TopBottom() on, instead of duplicating this logic.

r=me with those changes.
Attachment #733127 - Flags: review?(bzbarsky) → review+
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd9decf97bb
Target Milestone: --- → mozilla23
fwiw, that code could probably be simplified using nsMargin's operator+=, kinda like so:

  nsMargin adjustment;
  switch(stylePos->mBoxSizing) {
    case NS_STYLE_BOX_SIZING_BORDER:
      adjustment += mInnerFrame->GetUsedBorder();
    case NS_STYLE_BOX_SIZING_PADDING:
      adjustment += mInnerFrame->GetUsedPadding();
  }
  return adjustment;
(In reply to Boris Zbarsky (:bz) from comment #10)
> fwiw, that code could probably be simplified using nsMargin's operator+=,
> kinda like so:
> 
>   nsMargin adjustment;
>   switch(stylePos->mBoxSizing) {
>     case NS_STYLE_BOX_SIZING_BORDER:
>       adjustment += mInnerFrame->GetUsedBorder();
>     case NS_STYLE_BOX_SIZING_PADDING:
>       adjustment += mInnerFrame->GetUsedPadding();
>   }
>   return adjustment;

Thanks, I'll post a followup to make this a bit clearer.
https://hg.mozilla.org/mozilla-central/rev/2bd9decf97bb
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Landed followup patch on inbound to address minor issues in comment 10:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5820d44601c1
Scott, you probably want to also remove the borderPaddingLeft variable and friends.
(In reply to Boris Zbarsky (:bz) from comment #14)
> Scott, you probably want to also remove the borderPaddingLeft variable and
> friends.

(In reply to Nathan Froyd (:froydnj) from comment #15)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/45835d0c9d58 for
> breaking the build (IRC nick said you were @ lunch):
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21474844&tree=Mozilla-
> Inbound

Yep, oversight on my part. Thanks for backing out. I'm pushing to try first now this time to ensure it doesn't break the build again :) -

https://tbpl.mozilla.org/?tree=Try&rev=b1c4297f6c81
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla23 → ---
Try looks green enough... pushing again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec1088730f6
(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Backout:
> https://hg.mozilla.org/mozilla-central/rev/45835d0c9d58

This is only for the followup patch - it doesn't affect the actual fix. It's mostly just code cleanup.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
setting Target Milestone to 24, please correct if I'm wrong
Target Milestone: --- → mozilla24
Target Milestone: mozilla24 → mozilla23
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: