521525 - Removing height from block doesn't change layout immediately for percent-height kids

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (dynamic)

Based on layout/base/crashtests/89101-1.html.

Seems to be quirksmode-only.

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Boris Zbarsky [:bzbarsky]]

The difference is that in quirks mode mFlags.mVResize ends up false on the reflow state, so we don't reflow the block inside the fieldset, even though it has a percentage height.

The relevant code here seems to be:

  } else if (mComputedHeight == NS_AUTOHEIGHT) {
    if (eCompatibility_NavQuirks == aPresContext->CompatibilityMode() &&
        mCBReflowState) {
      mFlags.mVResize = mCBReflowState->mFlags.mVResize;
    } else {
      mFlags.mVResize = mFlags.mHResize || NS_SUBTREE_DIRTY(frame); 
    }

In this case, NS_SUBTREE_DIRTY(frame) is true, so in standards mode we get mVResize set.

The frame flags are 0x1021, which does NOT include NS_FRAME_IS_DIRTY for some reason.  It really should in this case.  Looking into that now.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And _that_ is a regression from bug 502288.  I think I should probably include nsChangeHint_NeedDirtyReflow for height/min-height/max-height changes, though it's still ok to skip for width due to the asymmetry between width and height calculations.

David, does that seem reasonable?  I guess the other option is to add the NS_SUBTREE_DIRTY(frame) condition in quirks mode too.  Would you prefer that?  not sure what the tradeoff there is...

We need to fix this on 1.9.2 as well.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase not involving fieldsets

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 7

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

Comment on attachment 405914 [details] [diff] [review]
Proposed fix

r=dbaron

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/59dcd1b1a220

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 405914 [details] [diff] [review]
Proposed fix

Need this on 1.9.2.

Comment 10

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

Comment on attachment 405914 [details] [diff] [review]
Proposed fix

a1.9.2=dbaron

Comment 11

[Boris Zbarsky [:bzbarsky]]

Note that this patch made some cases that didn't use to reflow correctly (e.g. bug 523096) reflow correctly, presumably because mVResize being true triggers a necessary codepath that they didn't use to trigger for just having a dirty descendant.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2462c73609b5