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

RESOLVED FIXED

Status

()

P2
minor
RESOLVED FIXED
9 years ago
3 months ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
Created attachment 405611 [details]
testcase (dynamic)

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

Seems to be quirksmode-only.
(Reporter)

Comment 1

9 years ago
Created attachment 405612 [details]
reference (static)
(Assignee)

Comment 2

9 years ago
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.
(Assignee)

Comment 3

9 years ago
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.
Assignee: nobody → bzbarsky
Blocks: 502288
(Assignee)

Comment 4

9 years ago
Created attachment 405626 [details]
Testcase not involving fieldsets
(Assignee)

Updated

9 years ago
Assignee: bzbarsky → nobody
Component: Layout: Form Controls → Layout: Misc Code
Flags: blocking1.9.2?
OS: Mac OS X → All
QA Contact: layout.form-controls → layout.misc-code
Hardware: x86 → All
(Assignee)

Updated

9 years ago
Summary: Removing height from fieldset doesn't change layout immediately → Removing height from block doesn't change layout immediately for percent-height kids
(Assignee)

Updated

9 years ago
Blocks: 521539
(Assignee)

Comment 5

9 years ago
Created attachment 405914 [details] [diff] [review]
Proposed fix
Attachment #405914 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Duplicate of this bug: 521539
(Assignee)

Comment 8

9 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/59dcd1b1a220
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Comment 9

9 years ago
Comment on attachment 405914 [details] [diff] [review]
Proposed fix

Need this on 1.9.2.
Attachment #405914 - Flags: approval1.9.2?
Assignee: nobody → bzbarsky
(Assignee)

Updated

9 years ago
Blocks: 523096
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.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2

Updated

3 months ago
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.