Closed Bug 413361 Opened 13 years ago Closed 13 years ago

clicking thumbnail image on web page resizes border wider, does not resize image

Categories

(Core :: Layout: Block and Inline, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta3

People

(Reporter: StenBiz, Assigned: dbaron)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre

The photos and cartoons illustrating articles on this newspaper are shown as thumbnails, with instructions to "Click thumbnails to see larger image". On Firefox 2.0.0.11 this works OK - the thumbnail is replaced in the same window with a larger border with the full-sized image. On Firefox 3 clicking makes the border wider and slightly taller, and the thumbnail is centered in the border. (It appears the vertical resize is just to hold the caption and not to accommodate the image, so probably the image area is only resizing wider and not taller.) The resize works correctly in these browsers:

Firefox 2.0.0.11
Safari 3.0.4 (523.12.2)
Opera 9.24

Note that in all these browsers the text is not reflowed and is partly covered up by the larger image; probably not a bug(?).

Reproducible: Always

Steps to Reproduce:
1. Navigate to page
2. click cartoon at upper left (labelled "Click thumbnails to see larger image")
3. Note: problem seems to afflict all thumbnail image on this newspaper; the given URL is one example.
Actual Results:  
border expands horizontally and a little bit vertically, image does not resize but is centered in box

Expected Results:  
border should have expanded proportionally horizontally and vertically and image should have expanded to fit.
Attached file testcase
This has to do with conflicting width, min-width, and max-width CSS declarations. Apparently we respect max-width even if min-width is larger (but only if width is also specified?)

Not confirming yet 'cause I don't have time to look at the standards (or search for dupes) right now.
Component: General → Layout: Block and Inline
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout.block-and-inline
Version: unspecified → Trunk
So, when width >= min_width > max_width, the final width is computed as max_width.
The spec[1], if I'm reading it correctly, suggests that min-width should always win, i.e. the final width should be max(min_width, min(width, max_width)), which in this case is min_width, not max_width.

I couldn't find a duplicate, so confirming.

Also, this appears to be a regression from 1.8.  

[1] http://www.w3.org/TR/CSS21/visudet.html#min-max-widths
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
The problem is that NS_CSS_MINMAX presupposes that the min and max satisfy the constraint that max >= min.  This is enforced in the reflow state by nsHTMLReflowState::ComputeMinMaxValues.  But the code in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions doesn't enforce that except in the "both auto" case.

The solution is to move those 4 lines in that function up out of that if... and to write a bunch of tests for that code.  The tests being the key (and hard) part.

Oh, and NS_CSS_MINMAX should probably assert if min > max when it's called.
Flags: blocking1.9?
Yeah, the NS_CSS_MINMAX macro doesn't handle this correctly.  Previously the macro was only used in nsImageFrame and nsBulletFrame; now it's used in more places, and in ways (ComputeSize) that nsHTMLReflowState doesn't correct for.
BTW, I wrote the above comment before seeing Boris's; I think we should fix NS_CSS_MINMAX by making it a function template.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P2
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.9 M11
And I think the main testing thing we need to do here is get fantasai's tests from bug 234686 (i.e., http://fantasai.inkedblade.net/style/specs/css2.1/tests/min-max-replaced ) into the tree, plus Uri's testcase above.  I'm sure more could be written, but I think that ought to be enough for now.
I'd be happy with the template solution, yeah.  I guess the extra compares won't hurt that much perf-wise.

And yeah, getting those tests into the tree would cover things as far as I'm concerned.  I didn't realize we already had tests for this.  ;)
Attached patch patchSplinter Review
This fixes the bug as described and adds Uri's test (which I assume he's ok with).

fantasai's tests cover the width:auto/height:auto cases pretty well (but nothing else); still waiting for her permission to add those.
Attachment #298782 - Flags: superreview?(bzbarsky)
Attachment #298782 - Flags: review?(bzbarsky)
Comment on attachment 298782 [details] [diff] [review]
patch

Looks good.
Attachment #298782 - Flags: superreview?(bzbarsky)
Attachment #298782 - Flags: superreview+
Attachment #298782 - Flags: review?(bzbarsky)
Attachment #298782 - Flags: review+
The "extra compares" shouldn't really hurt at all, since the normal case is that the min-width and max-width don't do any constraining, and in that case we're going from two compares to two compares.  The only case where we do more compares than before is the case where min-width > width (where we short-circuited before).
You have my permission to add those tests (and any other tests for which I own the copyrights) to Mozilla's source tree.
Flags: blocking1.9? → blocking1.9+
Attachment #298782 - Flags: approval1.9? → approval1.9+
(In reply to comment #8)
> This fixes the bug as described and adds Uri's test (which I assume he's ok
> with).
I'm OK with it, of course.
Checked in to trunk, 2008-01-23 17:21 -0800.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
The checkin included a reftest.
Flags: in-testsuite+
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.