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

Status: RESOLVED FIXED

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

Description

[Csaba Gabor]

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

Comment 1

[Csaba Gabor]

Attachment: Demo with div showing getComputedStyle/height

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.

Comment 3

[j.j.]

Attachment: simple testcase

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b520992

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!

Comment 5

[Boris Zbarsky [: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.

Comment 6

[Scott Johnson (:jwir3)]

Attachment: b520992

(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!

Comment 7

[Boris Zbarsky [: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.

Comment 8

[Scott Johnson (:jwir3)]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=2c5e2bfb05b8

Comment 9

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd9decf97bb

Comment 10

[Boris Zbarsky [:bzbarsky]]

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;

Comment 11

[Scott Johnson (:jwir3)]

(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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd9decf97bb

Comment 13

[Scott Johnson (:jwir3)]

Landed followup patch on inbound to address minor issues in comment 10:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5820d44601c1

Comment 14

[Boris Zbarsky [:bzbarsky]]

Scott, you probably want to also remove the borderPaddingLeft variable and friends.

Comment 15

[Nathan Froyd [:froydnj]]

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

Comment 16

[Scott Johnson (:jwir3)]

(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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backout:
https://hg.mozilla.org/mozilla-central/rev/45835d0c9d58

Comment 18

[Scott Johnson (:jwir3)]

Try looks green enough... pushing again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec1088730f6

Comment 19

[Scott Johnson (:jwir3)]

(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.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/aec1088730f6

Comment 21

[j.j.]

setting Target Milestone to 24, please correct if I'm wrong