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

Status: VERIFIED FIXED

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

Description

[Allen Stenger]

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.

Comment 1

[Uri Bernstein]

Attachment: 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.

Comment 2

[Uri Bernstein]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

BTW, I wrote the above comment before seeing Boris's; I think we should fix NS_CSS_MINMAX by making it a function template.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 298782 [details] [diff] [review]
patch

Looks good.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 11

[fantasai]

You have my permission to add those tests (and any other tests for which I own the copyrights) to Mozilla's source tree.

Comment 12

[Uri Bernstein]

(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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk, 2008-01-23 17:21 -0800.

Comment 14

[Jesse Ruderman]

The checkin included a reftest.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.