851629 - Add float break state recovery that traverses blocks/lines that aren't being reflowed

Status: NEW

(Core :: Layout: Floats, defect)

Description

[Scott Johnson (:jwir3)]

Bug 600100 details a crash that is happening because we aren't reflowing placeholders of floats when layout hasn't changed. We should implement a solution that traverses any floats within blocks/lines that aren't being reflowed in ReflowDirtyLines(), finds the placeholders for those floats, and reflows the floats. 

From bug 600100, comment 18:

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #18)
> So basically we have the problem that this bit of
> nsBlockFrame::ReflowDirtyLines (which is already a pretty significant
> de-optimization) is enough to fix layout of split floats, but isn't
> sufficient for restoring the breaking state for those floats.
>     // If we have a constrained height (i.e., breaking columns/pages),
>     // and the distance to the bottom might have changed, then we need
>     // to reflow any line that might have floats in it, both because the
>     // breakpoints within those floats may have changed and because we
>     // might have to push/pull the floats in their entirety.
>     // FIXME: What about a deltaY or height change that forces us to
>     // push lines?  Why does that work?
>     if (!line->IsDirty() &&
>         aState.mReflowState.availableHeight != NS_UNCONSTRAINEDSIZE &&
>         (deltaY != 0 || aState.mReflowState.mFlags.mVResize) &&
>         (line->IsBlock() || line->HasFloats() || line->HadFloatPushed())) {
>       line->MarkDirty();
>     }
> In other words, even if we have a reflow that isn't changing layout, we
> still need to reflow the floats to restore any continuation state.
> 
> A second fix might be something like adding float break state recovery that
> traverses any blocks or lines that aren't being reflowed in
> ReflowDirtyLines, finds their placeholders, and reflows the floats.  This
> might be preferable to trying to do everything that would be needed to make
> the layout correct, which includes the work that nsBlockFrame::SplitFloat or
> nsBlockReflowState::PushFloatPastBreak do, which can affect the layout of
> later floats.  (In other words, I think the failure to reflow the floats is
> also covering up layout bugs, which reflowing the floats would fix.)  For
> example, suppose that due to a dynamic change inside of a column, we
> reflowed a column that had two floats in it.  The first float, due to its
> size and where the first breakpoint within it was, needed to be pushed past
> the break.  The second float gets pushed past the break only because the
> first one was, and we honor the rule about not placing later floats prior to
> earlier ones (in other words, if the first float wasn't there, the second
> float would fit in the column).  Now suppose there's a dynamic change to a
> line in between the lines containing the placeholders of these floats -- one
> that doesn't change the above situation, but does change the height of that
> line slightly.  Given the current code, it seems like in the reflow to
> handle that dynamic change, we'd move the second float into the first column
> because we didn't reflow the first float at all (and thus call
> PushFloatPastBreak).
> 
> While it might seem admirable to try to fix this without doing the extra
> work, it requires a nontrivial amount of code that probably won't be correct
> on the first pass.  I think it's better to get correct behavior here first
> and worry about optimization later.