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

RESOLVED FIXED in mozilla19

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla19
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Created attachment 670959 [details]
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.
Created attachment 670960 [details]
Testcase
Created attachment 671014 [details] [diff] [review]
Take box-sizing into account when computing intrinsic width based on a fixed height and an intrinsic ratio.
Attachment #671014 - Flags: review?(dholbert)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
...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)
Yeah, happy to file a followup to make this stuff handle calc() on the vertical dimensions.
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?
Attachment #671014 - Flags: review?(dholbert) → review+
> 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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/2297babdb84d
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla19
(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 :)
https://hg.mozilla.org/mozilla-central/rev/2297babdb84d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
> 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.
Thanks :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/04c905ed3501
https://hg.mozilla.org/mozilla-central/rev/04c905ed3501

Updated

5 years ago
Duplicate of this bug: 812496
Duplicate of this bug: 771414
You need to log in before you can comment on or make changes to this bug.