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)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [patch]needed-aviary1.0)
Attachments
(3 files, 1 obsolete file)
2.80 KB,
text/html
|
Details | |
7.50 KB,
patch
|
Details | Diff | Splinter Review | |
57.58 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•20 years ago
|
||
Assignee | ||
Comment 2•20 years ago
|
||
Assignee | ||
Comment 3•20 years ago
|
||
The patch in attachment 141634 [details] [diff] [review] fails tests 9 and 12 in http://fantasai.inkedblade.net/style/specs/css2.1/tests/min-max-replaced and test 12 in http://www.hixie.ch/tests/adhoc/css/box/replaced/intrinsic/001.html . Whose fault is that (tests, spec, or implementation)?
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [patch]
Target Milestone: --- → mozilla1.7beta
Assignee | ||
Comment 4•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
I think the problem in test 12 is a bug in fantasai's spec, although I'm not sure what the fix is.
Assignee | ||
Comment 7•20 years ago
|
||
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.
Comment 8•20 years ago
|
||
http://www.hixie.ch/tests/adhoc/css/box/replaced/intrinsic/001.html fixed
Assignee | ||
Comment 9•20 years ago
|
||
Oops, I never checked this in...
Target Milestone: mozilla1.7beta → mozilla1.8alpha
Assignee | ||
Updated•20 years ago
|
Attachment #141634 -
Flags: superreview?(bzbarsky)
Attachment #141634 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•20 years ago
|
||
Note also that this algorithm has found its way into http://www.w3.org/TR/2004/CR-CSS21-20040225/visudet.html#min-max-widths
Comment 11•20 years ago
|
||
I'll try to get to this by Wednesday, hopefully....
Comment 12•20 years ago
|
||
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+
Assignee | ||
Comment 13•20 years ago
|
||
*** Bug 241017 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 14•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #141634 -
Attachment is obsolete: true
Assignee | ||
Comment 15•20 years ago
|
||
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
Assignee | ||
Comment 16•16 years ago
|
||
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.
Assignee | ||
Comment 17•16 years ago
|
||
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.
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•