801116 - Intrinsic width calculation for does not handle box-sizing correctly in the intrinsic ratio case

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Image for testcase

Consider an image with 100x100 intrinsic size, styled with:

  height: 200px; border: 50px solid; box-sizing: border-box;

The content height of the image will be 100px (due to the box-sizing), so the content width will be 100px (see intrinsic ratio) and then the width for parent intrinsic sizing purposes will be 200px.

But what our code seems to do is look at the height, multiply by the intrinsic ratio, get 200px, then add the border and padding, so get 300px, which is wrong.

What we should be doing is applying box-sizing to the values we multiply by the intrinsic ratio.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Take box-sizing into account when computing intrinsic width based on a fixed height and an intrinsic ratio.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 671014 [details] [diff] [review]
Take box-sizing into account when computing intrinsic width based on a fixed height and an intrinsic ratio.

>+        case NS_STYLE_BOX_SIZING_PADDING: {
>+          const nsStylePadding* stylePadding = aFrame->GetStylePadding();
>+          nscoord pad;
>+          if (GetAbsoluteCoord(stylePadding->mPadding.GetTop(), pad) ||
>+              GetPercentHeight(stylePadding->mPadding.GetTop(), aFrame, pad)) {
>+            heightTakenByBoxSizing += pad;

I don't think this is quite sufficient -- this doesn't handle e.g. "padding: calc(5px + 10%)", or even just "calc(5%)".  (GetAbsoluteCoord bails out if it's given a calc() expression that involves a percent, and GetPercentHeight bails out if it's given anything other than a raw percent value.)

I think we need a "GetCalcPercentHeight()" helper-function, which assumes it's being called on a calc-valued stylecoord, and which farms its work out to GetAbsoluteCoord, GetPercentHeight, and recursive calls to itself.

Comment 4

[Daniel Holbert [:dholbert]]

...though I suppose that affects existing code, too, e.g. this chunk of contextual code later in the patch:
>         if (GetAbsoluteCoord(styleHeight, h) ||
>             GetPercentHeight(styleHeight, aFrame, h)) {

Since we already have that issue here in existing code, I suppose we shouldn't block new code just for suffering from that same issue -- but we should at least have a followup on handling this calc thing (and using it basically anywhere we call GetAbsoluteCoord || GetPercentHeight)

(still reading patch, done soon)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yeah, happy to file a followup to make this stuff handle calc() on the vertical dimensions.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 671014 [details] [diff] [review]
Take box-sizing into account when computing intrinsic width based on a fixed height and an intrinsic ratio.

>         if (GetAbsoluteCoord(styleMaxHeight, h) ||
>             GetPercentHeight(styleMaxHeight, aFrame, h)) {
>+          h = NS_MAX(0, h - heightTakenByBoxSizing);
>           h = NSToCoordRound(h * (float(ratio.width) / float(ratio.height)));
>           if (h < result)
>             result = h;
>         }
> 
>         if (GetAbsoluteCoord(styleMinHeight, h) ||
>             GetPercentHeight(styleMinHeight, aFrame, h)) {
>+          h = NS_MAX(0, h - heightTakenByBoxSizing);
>           h = NSToCoordRound(h * (float(ratio.width) / float(ratio.height)));
>           if (h > result)
>             result = h;
>         }

This code is confusing.

We have a tentative height "h", and then we say
 tentative_height = tentative_height * (width/height);

...why?  We're computing a width value in that expression, but we're storing it in "h".  This is presumably an attempt at optimization -- but I don't think it buys us enough to be worth the code-readability-penalty.

I think it's worth fixing this in this patch, since this patch makes us do more things with "h", which aggravates the existing readability issue.

So -- could you change this so we're not using "h" for widths?  either by doing
  nsresult tentativeWidth =
    NSToCoordRound(...)
  if (tentativeWidth > result) {

...or by doing:
  result = NS_MAX(result, NSToCoordRound(...))

(with NS_MIN in the first chunk above and NS_MAX in the second, I think)

r=me with that

optional: maybe also include a reftest with "padding: calc(15%)" or something, marked as fails pending the other bug being fixed?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I think it's worth fixing this in this patch, since this patch makes us do more things
> with "h", which aggravates the existing readability issue.

Agreed.  I assigned the results of the NSToCoordRound to maxHeight and minHeight nscoord vars respectively at the two callsites.

> optional: maybe also include a reftest with "padding: calc(15%)" or something, marked as > fails pending the other bug being fixed?

Done.  I filed bug 801994 on the followup.

Comment 8

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2297babdb84d

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> > I think it's worth fixing this in this patch, since this patch makes us do more things
> > with "h", which aggravates the existing readability issue.
> 
> Agreed.  I assigned the results of the NSToCoordRound to maxHeight and
> minHeight nscoord vars respectively at the two callsites.

It's still a little confusing, since those are really *width* values (not height values) -- but it's better than it was, I suppose :)

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2297babdb84d

Comment 11

[Boris Zbarsky [:bzbarsky]]

> It's still a little confusing, since those are really *width* values

Er... so they are!  Totally my fault.  I'll rename them to maxWidth and minWidth.

Comment 12

[Daniel Holbert [:dholbert]]

Thanks :)

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c905ed3501

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04c905ed3501