385823 - Images with pct height in tables are handled differently in FF 2.0 vs trunk

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert]]

The images (with height=100%) in these two webpages are rendered differently in FF 2.0 vs trunk:

http://people.mozilla.com/~dholbert/tests/364124/without_height/img.html
http://people.mozilla.com/~dholbert/tests/364124/with_height/img.html

I'll make better testcases for them and post them tonight or tomorrow.

Note: These testcases were originally discovered while exploring bug 364124, but they're caused by something different from the underlying problem in that bug.  (Hence, filing this bug)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: image (for use in testcases)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1: table has auto-height

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: table has specified height

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: (capture) testcase 1, firefox 2

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: (capture) testcase 1, trunk

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: (capture) testcase 2, firefox 2

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: (capture) testcase 2, trunk

Comment 8

[Daniel Holbert [:dholbert]]

If we want image-in-table cases to match the behavior of table-in-table cases, then Trunk is doing the right thing on the first (auto-height) testcase, whereas FF 2 is doing the right thing on the second (specified height) testcase.

The general philosophy with table-in-table cases, where the inner table has pct height, is this:
  If an ancestor (cell/row/rowgroup/table) of the inner table has height style data, then we should honor the inner table's pct height.  Otherwise, we do not honor the pct height.

I'm not sure if that's the Right Thing To Do when the inner element is an image, but it's what we'd need to do match for image height to work like table height in this respect.

Comment 9

[Mikko Rantalainen]

Also note that both testcases show reflow problems in older versions. After loading testcase 1 or testcase 2 press CTRL-+ followed by CTRL-= (or force reflow by some other means). The missing text appears in Firefox 1.5/linux (Ubuntu 6.06). Firefox 2.0.0.4/win32 (official) renders both the test cases differently and definitely incorrectly (the text is never displayed and the image is clipped horizontally to the length of the text).

For the test case 2, I'd expect (as a page author) the table cell size (250px) to be honored and it's content filled with the text line at the bottom and image sized to fill the remaining space (100% meaning all the available space instead of 100% of the specified height). Such behavior is probably very hard to implement and against normal CSS behavior. Perhaps a reasonable compromise would be to set height of the image to 100% of specified table height (250px) and then the table cell would be enlarged vertically to fit the text line. In any case, the second test case should not render the image over the text. Clearly the text should start one line below the image.

Comment 10

[Daniel Holbert [:dholbert]]

See also bug 225548, Testcase #2 (attachment 141977 [details])

(It's similar to testcase #2 on this bug, but without any text below the 100%-high image.)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 1: table has auto-height (fixed)

Fixing the missing </title> in testcase 1.  (worked in FF, but prevented IE from rendering it)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: table has specified height (fixed)

Fixing the missing </title> in testcase 2.  (worked in FF, but prevented IE
from rendering it)

Comment 13

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

Transferring blocking1.9+ from bug 225548 for the regression in bug 225548 comment 19.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: first-pass patch

Here's a first-pass and admittedly bad patch, but it nonetheless fixes the bug and demonstrates what's wrong.  

The issue is related to the call to NotifyPercentHeight at nsFrame.cpp:3142.  (see the patch for context, or 
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#3141 )

We only call NotifyPercentHeight if both of the following hold:
  a) the frame has a percent height
  b) aReflowState.ComputedHeight() is either 0 or NS_UNCONSTRAINEDSIZE

For image frames, requirement "b" isn't satisfied -- ComputedHeight() is set to the *true* height of the original image at that point.

The reflowState's ComputedHeight is set in nsHTMLReflowState::InitConstraints, and it's set based on frame->ComputeSize().  But nsImageFrame::ComputeSize ignores its percentage-height and returns the image's true size instead.  In contrast, the default nsFrame.cpp version of ComputeSize returns NS_UNCONSTRAINEDSIZE in cases where we've got percent-heights.

So, frames that use the default nsFrame.cpp version of ComputeSize will get their deserved special-height reflow, but nsImageFrames won't because they have a custom version of ComputeSize.

Fixes:
------

Tentative fix 1:
   I initially tried fixing this by modifying nsImageFrame::ComputeSize to fall back on nsFrame::ComputeSize for images with percent heights (so that reflowState->ComputedHeight() ends up as NS_UNCONSTRAINEDSIZE).  This is bad, however, because nsImageFrame::Reflow depends on the reflowState's computed size for its metrics (see lines 802 and 806-807), and the image ends up being millions of pixels tall.  In other words, nsImageFrame::Reflow seems to *expect* the reflowState's computed height to be an actual number -- not NS_UNCONSTRAINEDSIZE.

Tentative fix 2:  (The patch I'm posting now)
  Just add a special case for imageFrames in nsFrame::DidReflow so that we'll call NotifyPercentHeight for %-height images, regardless of their computedHeight.  This works, but it's probably a bad use of a special case.


Possible fix 3:
  We could remove the checks on aReflowState->ComputedHeight() altogether from DidReflow(), if those aren't needed.  I'm not sure why they're there -- don't we always need a special-height reflow when we have %-height content inside a cell, regardless if we've somehow got a ComputedHeight?

Possible fix 4:
  We could rework how nsImageFrame::ComputeSize and nsImageFrame::Reflow work, so that we can have aReflowState->ComputedHeight() be NS_UNCONSTRAINEDSIZE without breaking the imageFrame's reflow.

Comment 15

[Daniel Holbert [:dholbert]]

Note: Even with the special-height reflow, there's still one problem -- the table's width doesn't grow to accomidate the enlarged image on testcase 2 (attachment 277812 [details]).  

IE/Opera/Konqueror all show this same sort of behavior, and Firefox 2.0 does too under certain conditions (see bug 385825), so maybe it's not a big issue.

Also, to fix this problem, it looks like we'd need to perform width adjustments during the special height reflow, and I'm not sure if that'd be dangerous.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: patch v2 (remove ComputedHeight checks from DidReflow)

This patch implements "Possible Fix 3" from my previous comment (removing the ComputedHeight() checks from DidReflow), and it seems to work well.  As far as I can tell, it won't cause any extraneous reflows or anything like that.

One consequence that might initially appear to be bad is that we'll now call NotifyPercentHeight in DidReflow for percent-height frames after a special-height reflow. (Before this patch, we wouldn't make this call, because the special-height reflow would assign a computedHeight to the percent-height frame's reflowState, and so the old ComputedHeight checks would make us skip over the NotifyPercentHeight call.)

However, this new call is basically a no-op in this case, because NotifyPercentHeight does nothing if the cell's reflowState has a computedHeight.  (and it _must_ have a computedHeight after a special-height reflow, afaik.)  See http://shorl.com/keprodruhahydy for details in nsTableCellFrame, which has the only implementation of NotifyPercentHeight.


This patch fixes testcase 2, the situation where the table has a specified height.  It also passes layout reftests.


As for testcase 1 (100%-high image in auto-height table), I think trunk is already doing the right thing, and Firefox 2 was wrong. Here's why I think Trunk is right here:

  * Trunk deals with the internal 100%-high image in the same way that it deals with an internal 100%-high table (letting them both auto-layout) whereas Firefox 2 does not (it makes the internal image fill the whole outer table, whereas it lets the internal table be auto-height.)  For details, compare these examples:
   http://people.mozilla.com/~dholbert/tests/364124/without_height/table.html
   http://people.mozilla.com/~dholbert/tests/364124/without_height/img.html

  * Trunk matches Opera and Konqueror. (IE does something completely different for testcase 1 -- it just doesn't show the image at all)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

dbaron would be a much better reviewer for this

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 279796 [details] [diff] [review]
patch v2 (remove ComputedHeight checks from DidReflow)

OK, switching reviewer to dbaron.

Comment 19

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

This didn't make any sense to me the first two times I looked at it, but now it makes perfect sense, so I think this is fine.  I think I was confused before about what code you were changing.  The computed height test became wrong once we started filling in computed heights for images all the time rather than waiting until reflow.  And I don't think it will make the quirk any broader than it should be since nsTableCellFrame::NotifyPercentHeight still checks the computed height on the cell, which is what we should be checking.

The one thing I'm worried about is that this might lead us to call NotifyPercentHeight during a special height reflow, which I don't think we should be doing.  Perhaps you should also change nsTableCellFrame::Reflow to set itself as the percent height observer if it's not currently a special-height reflow?

Sorry for taking so long to get to this.

Comment 20

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

Comment on attachment 279796 [details] [diff] [review]
patch v2 (remove ComputedHeight checks from DidReflow)

Marking review- since it increases the chances you'll see the above comment.

Comment 21

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

(In reply to comment #19)
> should be doing.  Perhaps you should also change nsTableCellFrame::Reflow to
> set itself as the percent height observer if it's not currently a
> special-height reflow?

Oops, I meant to *not* set itself as the percent height observer if it *is* currently a special-height reflow?

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: patch v3 (added: tableCell doesn't set self as % height observer during special-height reflow)

(In reply to comment #21)
> Oops, I meant to *not* set itself as the percent height observer if it *is*
> currently a special-height reflow?

I think this addresses that -- dbaron, how does this look?

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: reftests (as patch)

Attaching reftests for this bug.  Note that reftest 385823-2b.html fails right now, as marked in the changes to reftest.list.  This is exemplified in attachment 277812 [details] -- with this bug's patch applied, the right chunk of the scaled image is cut off.

This should be fixed at some point, but I'd say it's not as huge of an issue right now because:
 - It's partly broken in branch (see bug 385825)
 - It's broken in IE7 :)

(I'm planning on filing a separate bug for this issue.)

Comment 24

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

Comment on attachment 285025 [details] [diff] [review]
patch v3 (added: tableCell doesn't set self as % height observer during special-height reflow)

r+sr=dbaron

(But was the "psuedo" spelling fix part of a different patch?  I remember seeing that elsewhere, not that it really matters...)

Comment 25

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

Comment on attachment 285028 [details] [diff] [review]
reftests (as patch)

r+sr=dbaron.  It's interesting that you're testing some other quirks-mode stuff here too -- in particular, cells clipping their overflow.

Comment 27

[Martijn Wargers (dead)]

I guess this would also fix http://www.google.com/translate_t
where the textarea height is currently not ash high as in branch builds.

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: patch v3a (removed "s/psuedo/pseudo/" comment change -- already been fixed)

(In reply to comment #24)
> (From update of attachment 285025 [details] [diff] [review])
> r+sr=dbaron
> 
> (But was the "psuedo" spelling fix part of a different patch?  I remember
> seeing that elsewhere, not that it really matters...)
> 

Removed those "pseudo" spelling fix chunks from this version of the patch.

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 287589 [details] [diff] [review]
patch v3a (removed "s/psuedo/pseudo/" comment change -- already been fixed)

Requesting approvalM9, because Meebo (w/ alexa ranking 650) looks very broken without this patch.  See screenshots on Bug 390833.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 287589 [details] [diff] [review]
patch v3a (removed "s/psuedo/pseudo/" comment change -- already been fixed)

a=beltzner for M9

Comment 31

[Daniel Holbert [:dholbert]]

Checked in patch v3a:

Checking in generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.763; previous revision: 3.762
done
Checking in tables/nsTableCellFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableCellFrame.cpp,v  <--  nsTableCellFrame.cpp
new revision: 3.406; previous revision: 3.405
done

Comment 32

[Daniel Holbert [:dholbert]]

Reftests checked in as well.

Comment 33

[Carsten Book [:Tomcat]]

verified fixed using  Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a9pre) Gecko/2007110616 Minefield/3.0a9pre and also on Intel mac with the testcases from this bug

-> Verified

Comment 34

[Carsten Book [:Tomcat]]

also verified for the Firefox 3 Beta 1 Release Candidate 2 on all platforms with the testcases and the google translate url from comment #27 using:

Windows:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110811 Firefox/3.0b1

Linux (Fedora F7):
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b1) Gecko/2007110810 Firefox/3.0b1

Mac 10.4 Intel:
 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b1)
Gecko/2007110810 Firefox/3.0b1