Closed Bug 234686 Opened 20 years ago Closed 20 years ago

fix scaling of images in response to min/max-width/height

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Whiteboard: [patch]needed-aviary1.0)

Attachments

(3 files, 1 obsolete file)

Scaling of images in response to min/max-width/height isn't quite right, as
noted in an XXX comment in nsImageFrame::GetDesiredSize.  The previous bug on
this was bug 18477 (done before CSS really described the right approach).

Two proposals for fixing this are:
  http://www.w3.org/TR/2003/WD-CSS21-20030915/visudet.html#min-max-widths
  http://fantasai.inkedblade.net/style/specs/css2.1/min-max-replaced

Testcases are:
  http://www.hixie.ch/tests/adhoc/css/box/replaced/intrinsic/001.html
  http://dbaron.org/css/test/sec1004b
  http://dbaron.org/css/test/sec1007b
Status: NEW → ASSIGNED
Whiteboard: [patch]
Target Milestone: --- → mozilla1.7beta
The problem with image 9 is the result of:

  <p><img src="min-max-8.gif" alt="" id="img4"> (8)</p> <!-- w=25  h=25  -->
  <p><img src="min-max-9.gif" alt="" id="img5"> (9)</p> <!-- w=50  h=25  -->

(incorrect ids) which is fixed in Ian's copy of the test.
I think the problem in test 12 is a bug in fantasai's spec, although I'm not
sure what the fix is.
testcase fixed.

As for 12 indicating a bug, please clarify.
Er, no, the problem with test 12 is also a bug in the test.  There's a missing
':' in the CSS rule with selector #img12.
Oops, I never checked this in...
Target Milestone: mozilla1.7beta → mozilla1.8alpha
Attachment #141634 - Flags: superreview?(bzbarsky)
Attachment #141634 - Flags: review?(bzbarsky)
I'll try to get to this by Wednesday, hopefully....
Blocks: 241017
Comment on attachment 141634 [details] [diff] [review]
patch implementing proposal in comment 1

First of all, I assume that you're doing all the integer arithmetic on purpose
and that this is the reason why when you have an expression which should
conceptually be |a * (b/c)| you do |(a*b)/c|?  I guess the rounding we do here
is not a big deal (that is, always rounding down is ok)....

If so, please document all of that so people don't try to "fix" it later. 
Also, some places you write |a * b / c|; while this is the same as |(a*b)/c|,
it's worth parenthesizing to emphasize that the multiplication happens first.

Also, you may want to add a pointer to the relevant spec section at the
beginning of the computations so it's clear where all this is coming from...

>+      // 'auto' width, 'auto' height
>+      if (minWidth > maxWidth)
>+        maxWidth = minWidth;
>+      if (minHeight > maxHeight)
>+        maxHeight = minHeight;

Isn't this handled by nsHTMLReflowState::ComputeMinMaxValues?  If desired, we
can assert that |maxWidth >= minWidth| and similarly for height, but no reason
to enforce it here, I don't think.

>+      nscoord maxWidthScaled, minWidthScaled, maxHeightScaled, minHeightScaled;

"maxWidthScaled" being a measure of height is not very intuitive... The fact
that this is the height to use if we are using max-width for the width should
be clearly documented; it's very non-obvious from the variable name. (Maybe
name it "heightAtMaxWidth" instead?  Not sure that's much better.)  Similarly
for the other three quantities involved....

>+      if (intrinsicWidth > maxWidth) {
>+        if (intrinsicHeight > maxHeight) {
>+        } else if (intrinsicHeight < minHeight) {
>+          width = maxWidth;
>+          height = minHeight;
>+        } else {
>+          width = maxWidth;
>+          height = maxWidthScaled;
>+        }

These two cases can both use |width = maxWidth; height = maxWidthScaled;|, no? 
I realize you implemented exactly what the table in the spec says, and maybe we
want to leave it that way for clarity (the review applies either way), but then
again maybe we want to change the table in the spec to be simpler.

Indeed, the two cases we are looking at, in the spec's notation, are:

"w > max-width" and "(w > max-width) and (h < min-height)". 

Constraint Violation	Resolved Width		Resolved Height
-------------------------------------------------------------------------
    w > max-width	   max-width	 max(max-width * h/w, min-height)
-------------------------------------------------------------------------
   (w > max-width) 
	and		   max-width		  min-height   
   (h < min-height)


Now notice that if |w > max-width| then |max-width * h/w < h|; hence when |h <
min-height| we will automatically have |min-height > max-width * h/w|.

So the table in the spec could combine the two cases:

Constraint Violation	Resolved Width		Resolved Height
-------------------------------------------------------------------------
   (w > max-width) 
	and		   max-width	 max(max-width * h/w, min-height)
   (h <= max-height)

The intuitive justification here is that if w > max-width, we have to shrink w
for the constraints to be satisfied.  Given that h <= max-height, shrinking h
(to keep the aspect ratio) can't possibly violate the max-height constraint, so
we just need to ensure it does not violate the min-height constraint.  The
precise relationship of h and min-height does not matter; all that matter is
whether the scaled h is less than min-height or not.  The spec splits out the
case when we know for sure it will be, but I see no benefit to doing so.

I'll send mail to www-style with these comments....

>+      } else if (intrinsicWidth < minWidth) {
>+        if (intrinsicHeight > maxHeight) {
>+          width = minWidth;
>+          height = maxHeight;
>+        } else if (intrinsicHeight < minHeight) {
>+        } else {
>+          width = minWidth;
>+          height = minWidthScaled;
>+        }

Again, the two cases (the first "if" and last "else") can both use minWidth and
minWidthScaled.  Same reasoning as above.  So something like:

if (intrinsicWidth < minWidth) {
  if (intrinsicHeight >= minHeight) {
     // use minWidth, minWidthScaled
  } else {
     // Do complicated stuff
  }
}

>+      // 'auto' width, non-'auto' height
>+      height = MINMAX(height, minHeight, maxHeight);

Shouldn't "height" already satisfy that constraint?  nsHTMLReflowState should
ensure that.  I'd just change line to assert the constraint.

>+      // non-'auto' width, 'auto' height
>+      width = MINMAX(width, minWidth, maxWidth);

Same.

>+      // non-'auto' width, non-'auto' height
>+      height = MINMAX(height, minHeight, maxHeight);
>+      width = MINMAX(width, minWidth, maxWidth);

Same.

r+sr=bzbarsky.
Attachment #141634 - Flags: superreview?(bzbarsky)
Attachment #141634 - Flags: superreview+
Attachment #141634 - Flags: review?(bzbarsky)
Attachment #141634 - Flags: review+
*** Bug 241017 has been marked as a duplicate of this bug. ***
Attached patch patchSplinter Review
This addresses all of bz's comments except changing the things that
nsHTMLReflowState should do to assertions -- I don't trust nsHTMLReflowState,
and I don't trust assertions to catch problems that occur only for edge cases.
Fix checked in to trunk, 2004-07-04 17:17 -0700.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [patch] → [patch]needed-aviary1.0
Attached patch add testcasesSplinter Review
Add testcases from http://fantasai.inkedblade.net/style/specs/css2.1/tests/min-max-replaced , per bug 413361 comment 11.

Note that these tests only cover the width:auto + height:auto cases.
Tests checked in to trunk 2008-01-23 17:19 -0800, with a bunch marked failing on Mac (due to image scaling) at 18:05.

Not yet marking in-testsuite+ because we could really use more tests for the non-auto height and/or width cases.
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.