Collapsed margins do not uncollapse properly in this dynamic case




8 years ago
4 years ago


(Reporter: Jesse Ruderman, Unassigned)


(Depends on: 1 bug, Blocks: 2 bugs, {testcase})

Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(2 attachments)



8 years ago
Created attachment 405908 [details]
testcase (dynamic)

Comment 1

8 years ago
Created attachment 405909 [details]
reference (static)

Comment 2

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

Comment 3

8 years ago
This bug gets in refdyn's way fairly often.
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 ( != end_lines()) {
1935            PRBool maybeWasEmpty = oldY ==>mBounds.y;
1936            PRBool isEmpty = line->CachedIsEmpty();
1937            if (maybeReflowingForFirstTime /*1*/ ||
1938                (isEmpty || maybeWasEmpty) /*2/3/4*/) {
1939    >MarkPreviousMarginDirty();

doesn't reach line 1939 because maybeWasEmpty is false.  And its false because oldY is 1800 but>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.
Perhaps we should do:

  PRBool maybeWasEmpty = oldY == oldYMost ||
                         oldY ==>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?
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.
Assignee: nobody → roc
That seems relatively low-pain....
Assignee: roc → nobody
Blocks: 549926


4 years ago
Depends on: 50959
You need to log in before you can comment on or make changes to this bug.