Closed Bug 364066 Opened 14 years ago Closed 13 years ago

Images resized only via height cause floated parents to improperly shrink-to-fit.

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: moz, Assigned: dbaron)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files)

Images resized (smaller or larger) only by their height inside of floated parents don't make their parents' widths change to match the new computed widths. Simple testcase coming.
Attached image Image for testcase.
Attached file Testcase.
The area on my Last.fm profile that displays the issue should be obvious (it's the cover-art box). The album cover images should touch sides, which they do in Safari and Camino pre-reflow-landing.
(I'm still not sure what the "regression" keyword is supposed to be used for.)
Keywords: regressiontestcase
(In reply to comment #4)
> (I'm still not sure what the "regression" keyword is supposed to be used for.)

Regression means it worked corrrectly in some earlier version, so that's correct in this case.

Changing OS to All/All because I can reproduce this bug on Windows.

I;m pretty sure this is a regrassion from the reflow branch landing both because of the timeframe and because of some changes to nsImageFrame (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsImageFrame.cpp&rev=1.395&mark=779-780,795-796#779).  I'm also pretty sure dbaron is aware of this bug, considering the comments in the code.
Keywords: regression
OS: Mac OS X 10.4 → All
Hardware: PC → All
Flags: in-testsuite?
Flags: blocking1.9?
Attached patch patchSplinter Review
I'm hoping to extend use this new nsIFrame method for SVG intrinsic sizing, and SVG final size calculation, at some point.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #263964 - Flags: superreview?(bzbarsky)
Attachment #263964 - Flags: review?(bzbarsky)
Flags: blocking1.9? → blocking1.9+
Comment on attachment 263964 [details] [diff] [review]
patch

>+GetPercentHeight(const nsStyleCoord& aStyle,
>+    NS_ASSERTION(pos->mHeight.GetUnit() == eStyleUnit_Auto,
>+                 "unknown height unit");

I don't think you can make this assertion.  pos->mHeight might be a percentage, but the containing block of |f| might have auto height.  In that case you'll end up in this clause...

>+    NS_ASSERTION(pos->mMaxHeight.GetUnit() == eStyleUnit_Null,
>+                 "unknown max-height unit");

Similar issue here.

>+    NS_NOTREACHED("unknown min-height unit");

And here.

>@@ -1366,6 +1421,40 @@ nsLayoutUtils::IntrinsicForContainer(nsI

So this new code is only needed if the specified width is auto (or at least not an absolute coord), no?  Is it worth making that optimization, or just don't worry about it?  I guess we get the pref/min intrinsic widths of non-auto-width block kids too, so this is no worse...

>+      nsSize ratio = aFrame->GetIntrinsicRatio(aRenderingContext);
>+      if (ratio.height != 0) {
>+
>+        nscoord h;

I'd put the blank line before the |if| myself.  But either way.

>+          result = h * ratio.width / ratio.height;

Do we really want integer devision here?  I'd think not...  Same in the other two places where you're adding division in this function.


>+++ b/layout/generic/nsIFrame.h
>+  virtual nsSize GetIntrinsicRatio(nsIRenderingContext* aRenderingContext) = 0;

So the rendering context is currently unused by the GetIntrinsicRatio impls.  Do you expect future impls to use it?

r+sr=bzbarsky with the assertions fixed and the divisions made into floating point divisions.
Attachment #263964 - Flags: superreview?(bzbarsky)
Attachment #263964 - Flags: superreview+
Attachment #263964 - Flags: review?(bzbarsky)
Attachment #263964 - Flags: review+
(In reply to comment #7)
> I don't think you can make this assertion.  pos->mHeight might be a percentage,
> but the containing block of |f| might have auto height.  In that case you'll
> end up in this clause...

I'm going to allow percent as well, but still assert, in all 3 cases.

> So this new code is only needed if the specified width is auto (or at least not
> an absolute coord), no?  Is it worth making that optimization, or just don't
> worry about it?  I guess we get the pref/min intrinsic widths of non-auto-width
> block kids too, so this is no worse...

It's already inside:

  } else if (styleWidth.GetUnit() != eStyleUnit_Coord &&
             (styleMinWidth.GetUnit() != eStyleUnit_Coord ||
              styleMaxWidth.GetUnit() != eStyleUnit_Coord ||
              styleMaxWidth.GetCoordValue() > styleMinWidth.GetCoordValue())) {

> >+          result = h * ratio.width / ratio.height;
> 
> Do we really want integer devision here?  I'd think not...  Same in the other
> two places where you're adding division in this function.

The intent was to multiply first and then divide, but I suppose that leaves a real risk of integer overflow.  I suppose I should instead do:

          result = 
            NSToCoordRound(h * (float(ratio.width) / float(ratio.height)));

> So the rendering context is currently unused by the GetIntrinsicRatio impls. 
> Do you expect future impls to use it?

Oops, I'll remove it.
Sounds great on all counts.
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 386575
You need to log in before you can comment on or make changes to this bug.