522390 - Removing height property from overflow:auto div doesn't affect percentage-height children

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Jesse Ruderman]

Based on layout/reftests/percent-overflow-sizing/simpleHeight100.html

When this bug is fixed, this test probably belongs in percent-overflow-sizing ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hrm.  So this is actually a case where the two proposed fixes for bug 521525 are not equivalent, since the mVResize doesn't propagate through the anonymous scrolledframe.  And I don't think we can just make the scrolledframe return false from IsContainingBlock(), since the height and width on its reflow state are hacked to account for the scrollbars (see nsHTMLScrollFrame::ReflowScrolledFrame).

So we need to either fix bug 521525 by going back to using NS_FRAME_IS_DIRTY for size changes or we need to change the mVResize computation to skip over scrolled content too, not just things that are !IsContainingBlock.

I'm trying to think about other cases where this could bite (so where we have wrapper frames).  Table cells work due to the table cell hack in nsHTMLReflowState::InitCBReflowState (which I think we should push into IsContainingBlock).  Fieldsets use % sizing on the wrapper frame, so should be ok.  Any other obvious situations?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 5

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

This is a regression, right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, from bug 502288.

Comment 7

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

Comment on attachment 407229 [details] [diff] [review]
Fix

r=dbaron, although I wonder why you need ClearAncestorIntrinsics; shouldn't NeedReflow|NeedDirtyReflow be sufficient?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Er, yes.  It should.  Will fix that before landing.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: That made the patch different enough it should really get another once-over...

Comment 10

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

Comment on attachment 409167 [details] [diff] [review]
That made the patch different enough it should really get another once-over...

r=dbaron

Comment 11

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/3a5fa7a8fa16

Comment 12

[Boris Zbarsky [:bzbarsky]]

> although I wonder why you need ClearAncestorIntrinsics

Reftest has the answer, methinks: because nsBoxFrame::MarkIntrinsiWidthsDirty dirties the cached _heights_ too, and without calling it up the parent chain height changes on XUL boxes don't work right.  At least I get reftest failures for <html:input> in XUL on height changes unless I have height changes clear ancestor intrinsics.  :(

Pushed http://hg.mozilla.org/mozilla-central/rev/5d1ed737e5be to fix the test orange.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.2 merge

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/301b3fe25335