Closed Bug 582037 Opened 10 years ago Closed 10 years ago

Natural image dimensions are used for wrapping elements even if image is resized

Categories

(Core :: Layout, defect, P1, major)

x86_64
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b4

People

(Reporter: alystair, Assigned: bzbarsky)

References

()

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 ( .NET CLR 3.5.30729; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 ( .NET CLR 3.5.30729; .NET4.0E)

When a wrapped image is being resized, the surrounding wrapper incorrectly uses the natural image width instead of the resized image dimensions.

Reproducible: Always

Steps to Reproduce:
Save yourself time by going to the URL provided for reproduction.. or:
1. create a div with a large image inside it
2. apply 100% height to both div and image
3. apply absolute or fixed positioning to wrapping div
optionally:
4. apply a background color to div to confirm actual size of div is incorrect
5. change the position of the div to right:0

Actual Results:  
When the wrapped image is being resized, the surrounding wrapper incorrectly uses the natural image width instead of the resized image dimensions. This places the resized image in an incorrect starting location

Expected Results:  
The surrounding collapsed tags should have dimensions equal to the internal resized image.

Also affects new v4 builds of FF.

As of July 25th 2010, most popular engines are affected with the following exceptions:
* Webkit renders correctly until window is resized, however if user modifies zoom/font-size (ctrl+/-) the wrapper width is corrected again.
* Opera/9.80 (Macintosh; Intel Mac OS X; U) Presto/2.6.30 Version/10.60 - Renders correctly until window is resized however Windows version is affected (Windows NT 6.1; U)
Component: Layout: Images → Layout
So you're saying that intrinsic width computation for images with percentage widths and heights should use something better than the image's intrinsic size?

It's fixable for widths (with a bit of work -- in our implementation it would require tracking the percentage components in the InlineIntrinsicWidthData and then using coord/(1-percent) for each nonbreakable segment), but I don't even see how it's possible to fix for heights.  What behavior do you expect, exactly?  And would our switching to that behavior make us less compatible with other browsers and break Web sites?
This seems to only affect width calculation, I just did a local test case for wide imagery and seems to work fine. Only width needs to be corrected.

As for the expected result... it would be as I wrote above, the surrounding elements should utilize the resized image dimension instead of the original size.
(In reply to comment #2)
> This seems to only affect width calculation, I just did a local test case for
> wide imagery and seems to work fine.
That is, height measurements are properly calculated across all browsers
David, isn't the issue here just that GetPercentCoord call that nsLayoutUtils::IntrinsicForContainer makes returns false if there's an auto-height ancestor on the containing block chain, which isn't quite the right thing to do for the vieport (which has an auto height in the style context but isn't NS_AUTOHEIGHT during reflow)?  At least that's my initial guess on what's going on here given the confusing testcase and description.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Like so, say (obsolete) — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #460600 - Flags: review?(dbaron)
Attachment #460600 - Attachment is obsolete: true
Attachment #460603 - Flags: review?(dbaron)
Attachment #460600 - Flags: review?(dbaron)
Comment on attachment 460603 [details] [diff] [review]
With the reftest.list change too

r=dbaron
Attachment #460603 - Flags: review?(dbaron) → review+
Comment on attachment 460603 [details] [diff] [review]
With the reftest.list change too

Requesting approval for 2.0.  This fixes a correctness bug in our handling of %-height and replaced elements in shrink-wrap containers.  The risk is low to medium; I'm pretty sure that it's safe to set the sizes up front in the viewport and canvas, but I can't prove it.  If we encounter any issues it would be simple to back this out, though.
Attachment #460603 - Flags: approval2.0?
Priority: -- → P1
Summary: Natural image dimensions are used for wrapping elements even if image is resized → [FIX]Natural image dimensions are used for wrapping elements even if image is resized
Summary: [FIX]Natural image dimensions are used for wrapping elements even if image is resized → [FIXr]Natural image dimensions are used for wrapping elements even if image is resized
QA Contact: layout.images → layout
Summary: [FIXr]Natural image dimensions are used for wrapping elements even if image is resized → Natural image dimensions are used for wrapping elements even if image is resized
Whiteboard: [need approval]
Attachment #460603 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/52a83d6aea20
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Whiteboard: [need landing]
You need to log in before you can comment on or make changes to this bug.