521875 - Collapsed margins do not uncollapse properly in this dynamic case

Status: NEW

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Jesse Ruderman]

Based on layout/reftests/margin-collapsing/block-sibling-1a.html.

Comment 3

[Jesse Ruderman]

This bug gets in refdyn's way fairly often.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  So that first change is just there to force textframe creation.  Then the second marks the line with that textframe as dirty... but not the block line after it.

And then in ReflowDirtyLines, this code:

1934          if (line.next() != end_lines()) {
1935            PRBool maybeWasEmpty = oldY == line.next()->mBounds.y;
1936            PRBool isEmpty = line->CachedIsEmpty();
1937            if (maybeReflowingForFirstTime /*1*/ ||
1938                (isEmpty || maybeWasEmpty) /*2/3/4*/) {
1939              line.next()->MarkPreviousMarginDirty();

doesn't reach line 1939 because maybeWasEmpty is false.  And its false because oldY is 1800 but line.next()->mBounds.y is 2400.

And that happens because oldY includes the margin of the block above.... but not that of the block below.  But the old position of the second line includes both margins.  So we end up maybeWasEmpty false when in fact the line used to be empty.  Since isEmpty is false, and we're not reflowing for the first time, we skip the MarkPreviousMarginDirty call we actually need to make in this case.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Perhaps we should do:

  PRBool maybeWasEmpty = oldY == oldYMost ||
                         oldY == line.next()->mBounds.y;

?  Or would that miss some cases in which oldYMost is bigger than oldY but the line was still empty for margin-collapsing purposes?

Comment 6

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

Yeah, it seems like we'd have a combination of bug 393655 and this bug that could still happen.

The right approach is probably to have a state bit for line boxes that records whether the line was empty last time we reflowed it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

That seems relatively low-pain....