44242 - [MARGIN-C]computation of collapsed top margins is not correct

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

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

Computation of collapsed top margins is not correct in in the case where there
is an empty block as the first child of another block.  We need to pull out the
bottom margin of the empty block (in addition to the top margin of its next
sibling and that sibling's children, or siblings if the sibling is empty).  The
fix I'm going to check in will make this bug a little more obvious since there i
no longer special case handling of empty Ps that prevents it from showing up for
P elements.

The problem lies in nsBlockReflowContext::ComputeCollapsedTopMargin, where we
need to do some fancier stuff.  Probably nsBlockFrame::GetTopBlockChild should
be changed to do something else, since it was written only for the use of the
former function.

Comment 1

[Hixie (not reading bugmail)]

IE5 gets this right (believe it or not). However, I wouldn't hold release for
this if we can't get it fixed in time.

Comment 2

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

Giving margin collapsing bugs that I have not fixed to buster.

Comment 3

[buster]

P3 bugs are getting moved to "future" milestone.  They will not be addressed for
NS6 RTM.

Comment 4

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 5

[Chris Waterson]

Taking.

Comment 6

[Mats Palmgren (inactive)]

I have a relatively simple patch that fixes this bug (and bug 146115, bug 105346
too). There are two key points in this patch:
1. Add a new nscoord field to nsCollapsingMargin that keeps track of how much of
   the margin have been applied so far.
2. Pass the margin down to children so they can collapse correctly.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch A, rev 1.

Comment 8

[Mats Palmgren (inactive)]

Patch A uses an extra nsCollapsingMargin in nsHTMLReflowMetrics to carry the
margin down which is a bit wasteful. I have a more optimized patch that reuses
'mCarriedOutBottomMargin' for this instead (since it isn't used on the "top
edge"), if you are interested.

Comment 9

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

Comment on attachment 93094 [details] [diff] [review]
Patch A, rev 1.

>Index: layout/base/public/nsHTMLReflowMetrics.h
>+    nscoord mRunning;

I'm not yet convinced of the need for this new |mRunning|.  See comments below.

>@@ -143,6 +158,10 @@
>   // Carried out bottom margin values. This is the collapsed
>   // (generational) bottom margin value.
>   nsCollapsingMargin mCarriedOutBottomMargin;
>+  
>+  // Carried in top margin values. This is the collapsed
>+  // (generational) top margin value.
>+  nsCollapsingMargin mCarriedInTopMargin;
>   
>   // For frames that have content that overflow their content area
>   // (NS_FRAME_OUTSIDE_CHILDREN) this rectangle represents the total area

This seems to be in the wrong place.  nsHTMLReflowState is for input into the
reflow process, whereas nsHTMLReflowMetrics is for output from it.  This
belongs in nsHTMLReflowState.

Also, the copied comment doesn't seem to make sense -- what does "generational"
mean?

>Index: layout/html/base/src/nsBlockReflowContext.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockReflowContext.cpp,v
>retrieving revision 1.91
>diff -u -r1.91 nsBlockReflowContext.cpp
>--- layout/html/base/src/nsBlockReflowContext.cpp	1 Jul 2002 17:43:01 -0000	1.91
>+++ layout/html/base/src/nsBlockReflowContext.cpp	28 Jul 2002 17:09:45 -0000
>@@ -445,6 +445,7 @@
>     }
>   }
>   mTopMargin = aPrevBottomMargin.get();
>+  aPrevBottomMargin.Sync();
> 
>   // Compute x/y coordinate where reflow will begin. Use the rules
>   // from 10.3.3 to determine what to apply. At this point in the
>@@ -541,6 +542,10 @@
>   nscoord ty = y - mOuterReflowState.mComputedBorderPadding.top;
>   mOuterReflowState.mSpaceManager->Translate(tx, ty);
> 
>+  PRBool propagateMargin = (0 == aReflowState.mComputedBorderPadding.top &&
>+                            !(state & NS_BLOCK_MARGIN_ROOT))
>+                           ? PR_TRUE : PR_FALSE;
>+
>   // See if this is the child's initial reflow and we are supposed to
>   // compute our maximum width
>   if (mComputeMaximumWidth && (eReflowReason_Initial == aReason)) {
>@@ -549,6 +554,11 @@
> 
>     aReflowState.availableWidth = NS_UNCONSTRAINEDSIZE;
>     aReflowState.mComputedWidth = NS_UNCONSTRAINEDSIZE;
>+
>+    if (propagateMargin) {
>+      mMetrics.mCarriedInTopMargin = aPrevBottomMargin;
>+    }
>+
>     rv = aFrame->Reflow(mPresContext, mMetrics, aReflowState,
>                         aFrameReflowStatus);
> 
>@@ -565,6 +575,10 @@
>     aReason = eReflowReason_Resize;
>   }
> 
>+  if (propagateMargin) {
>+    mMetrics.mCarriedInTopMargin = aPrevBottomMargin;
>+  }
>+
>   rv = aFrame->Reflow(mPresContext, mMetrics, aReflowState,
>                       aFrameReflowStatus);
>   mOuterReflowState.mSpaceManager->Translate(-tx, -ty);
>@@ -706,19 +720,19 @@
>   // but probably should.
>   if ((0 == mMetrics.height) && (0 == mMetrics.mOverflowArea.height)) 
>   {
>-    // Collapse the bottom margin with the top margin that was already
>-    // applied.
>-    aBottomMarginResult.Include(mTopMargin);
>+    if (mTopMargin) {
> #ifdef NOISY_VERTICAL_MARGINS
>-    printf("  ");
>-    nsFrame::ListTag(stdout, mOuterReflowState.frame);
>-    printf(": ");
>-    nsFrame::ListTag(stdout, mFrame);
>-    printf(" -- collapsing top & bottom margin together; y=%d spaceY=%d\n",
>-           y, mSpace.y);
>+      printf("  ");
>+      nsFrame::ListTag(stdout, mOuterReflowState.frame);
>+      printf(": ");
>+      nsFrame::ListTag(stdout, mFrame);
>+      printf(" -- collapsing top & bottom margin together; y=%d spaceY=%d\n",
>+             y, mSpace.y);
> #endif
>-
>-    y = mSpace.y;
>+      // Exclude mTopMargin from the running top margin
>+      aBottomMarginResult.Undo(mTopMargin);
>+      y = mSpace.y;
>+    }

So I'm not convinced you need this Undo here.  [MOSTLY IGNORE THE REST OF THIS
PARAGRAPH, UNLESS YOU WANT TO FOLLOW MY TRAIN OF THOUGHT.]  Can't you get the
same effect by having the parent remembering the Y distance that it applied and
undoing that movement, in its own block reflow state, if the block is empty? 
After all, we already do something very similar for lines (see
nsBlockFrame::PlaceLine) -- this is only slightly more complicated because we
have to remember the old value since an empty block child could change the
margin.  It seems like it would make more sense to have an extra variable in
one place rather than burdening all the collapsing margins with that knowledge.

An even simpler possibility might be to modify nsBlockFrame::ReflowBlockFrame
so that it sets |applyTopMargin| to false if the block in question is empty (as
determined by |IsEmpty|, which I believe is what we should be using to
determine adjacency).  Then the margin that went in the top could just come out
the bottom again.  (After all, the CSS spec doesn't say where the block should
be placed, so we can decide.)  That approach would also mean that we wouldn't
have to change nsBlockReflowState::RecoverMarginAbove, since the carried out
bottom margin would be the correct thing to apply below the block.

>Index: layout/html/base/src/nsBlockReflowState.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockReflowState.cpp,v
>retrieving revision 3.456
>diff -u -r3.456 nsBlockReflowState.cpp
>--- layout/html/base/src/nsBlockReflowState.cpp	17 Jul 2002 01:48:55 -0000	3.456
>+++ layout/html/base/src/nsBlockReflowState.cpp	28 Jul 2002 17:09:46 -0000
>@@ -445,18 +445,8 @@
> void
> nsBlockReflowState::ReconstructMarginAbove(nsLineList::iterator aLine)
> {
>-  mPrevBottomMargin.Zero();

Why?  This seems wrong.  We don't want to apply a bottom margin from one line
to a line a ways down the page.

(Yet this function may need some changes to match the other ones.)

>   nsBlockFrame *block = mBlock;
> 
>-  const nsStyleText* styleText = NS_STATIC_CAST(const nsStyleText*,
>-      block->mStyleContext->GetStyleData(eStyleStruct_Text));
>-  PRBool isPre =
>-      ((NS_STYLE_WHITESPACE_PRE == styleText->mWhiteSpace) ||
>-       (NS_STYLE_WHITESPACE_MOZ_PRE_WRAP == styleText->mWhiteSpace));
>-
>-  nsCompatibility mode;
>-  mPresContext->GetCompatibilityMode(&mode);
>-
>   nsLineList::iterator firstLine = block->begin_lines();
>   for (;;) {
>     --aLine;
>@@ -464,16 +454,15 @@
>       mPrevBottomMargin = aLine->GetCarriedOutBottomMargin();
>       break;
>     }
>-    PRBool isEmpty;
>-    aLine->IsEmpty(mode, isPre, &isEmpty);
>-    if (! isEmpty) {
>+    if (aLine->GetHeight() > 0) {

I disagree with the changes from IsEmpty to GetHeight().  Do you want a block
with perfectly matching negative margins to trigger this code (when we're doing
an incremental reflow on a line just below it)?  If so, then we'd need to make
it trigger collapsing margins in the normal reflow path too, which seems like
it would be really hard.  (That's one of the reasons I prefer IsEmpty to height
checks in general.)

>+      mPrevBottomMargin.Zero();

Adding this Zero() doesn't make any sense to me.  Do note that this function is
walking backwards over the line list.

>       break;
>     }
>     if (aLine == firstLine) {
>       // If the top margin was carried out (and thus already applied),
>       // set it to zero.  Either way, we're done.
>-      if ((0 == mReflowState.mComputedBorderPadding.top) &&
>-          !(block->mState & NS_BLOCK_MARGIN_ROOT)) {
>+      if ((0 != mReflowState.mComputedBorderPadding.top) ||
>+          (block->mState & NS_BLOCK_MARGIN_ROOT)) {
>         mPrevBottomMargin.Zero();
>       }
>       break;

This change also doesn't make any sense to me.	(Did you read the comment at
the beginning of ReconstructMarginAbove?)

Comment 10

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

http://www.hixie.ch/tests/adhoc/css/box/block/margin-collapse/001.html

Comment 11

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

Attachment: Patch B, v. 1 (diff -u)

Comment 12

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

Attachment: Patch B, v. 1 (diff -uw)

Comment 13

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

Taking bug.

The patch I just attached seems to work, and the changes I made make sense to me
(or at least did when I wrote them).  It still needs a lot more testing, and
review...

Comment 14

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

Attachment: patch B, v. 2

Comment 15

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

*** Bug 80669 has been marked as a duplicate of this bug. ***

Comment 16

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

Attachment: patch B, v. 3

Comment 17

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

Here are the regression test results from the patch in attachment
106508 [details] [diff] [review].  A bunch of the regression tests (maybe 25 over the course of
the run) seem to hang, so I'm assuming I didn't get data on those tests,
but I managed to get the page cycler to continue past them by hitting
"Back" in viewer.

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/4956.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/5688.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/5859-a.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/8683.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/44264-1.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/44264-2.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/126213-1.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug18664.html
  FIXED.  Top margin of P or UL is now applied.  It was not before.
  (Note that it might be a desired quirk for it not to be applied.
  However, that issue should be fixed in bug 33784, not by having
  another bug cause in all modes a small part of the quirk that we might
  want in quirks mode only.)

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/7892.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-3.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-4.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-6.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-7.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-8.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-9.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/43882-11.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/53690.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/block/bugs/56563_1.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug53690-2.html
http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug56563.html
  FIXED.  Margin that was too big is now the correct size.

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/viewer_tests/test4.html
  Couldn't see the difference, but there probably is one.

http://lxr.mozilla.org/seamonkey/source/layout/html/tests/table/bugs/bug137388-2.html
  Incremental loading problem.  (A bug I'd mark as [reflow-refactor].)

Comment 18

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

A description of changes in the patch in attachment 106508 [details] [diff] [review]:

 * Adds a helper function nsBlockFrame::IsLineEmpty to do an |IsEmpty|
   call on a line, saving the caller from computing |isPre| and
   |compatMode|.

 * Fixes the logic in nsBlockFrame::IsEmpty so that a block is not
   considered empty when it has a nonzero height from style.  This is
   the correct notion of layout emptiness that is used for collapsing
   margins, and, I think, other things.

 * Makes nsBlockFrame::IsEmpty not consider top/bottom margins, since
   they're collapsed.

 * Changes nsBlockFrame::ShouldApplyTopMargin to use |nsIFrame::IsEmpty|
   rather than assuming that all blocks are non-empty.  (This is where
   we determine whether a block's top margin has been pulled out and
   applied to a parent or whether it needs to be applied, and must match
   ComputeCollapsedTopMargin.  Before it didn't match, and neither were
   correct.  Now they should match and should be correct.  This is
   somewhat ugly, and should probably be simplified in the future...)

 * Greatly simplifies nsBlockFrame::GetTopBlockChild to use
   |nsIFrame::IsEmpty|.  (It was originally written well before IsEmpty
   existed.)

 * Changes nsBlockFrame::PlaceLine to use IsEmpty to determine whether
   margins should collapse through a frame, rather than whether its
   height ended up being zero.  (This allows for consistent calculations
   in some edge cases.)

 * Fixes nsBlockReflowContext::ComputeCollapsedTopMargin to pull out all
   the margins that need to be pulled out, including those from later
   children (grandchildren, etc.) if all prior children are empty.

 * Fixes nsBlockReflowContext::ReflowBlock to apply the top margin only
   if told to do so.  (Did this not matter in the past because it was
   always zero when told not to do so?  It seems like a pretty obvious
   and major error.)

 * Fixes nsInlineFrame::IsEmpty not to consider top/bottom
   margin/border/padding as something that makes a frame non-empty,
   since if there's no width they don't show up.  Also |#if 0|'s out a
   bit of code that I think should be needed if we fix other bugs but
   causes incremental reflow problems (bug 172816, I think) now.

Comment 19

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

Comment on attachment 106508 [details] [diff] [review]
patch B, v. 3

r=roc+moz

I'm as confident about this as I am about any significant changes to block+line
:-).

Comment 20

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

Attachment: patch B, v. 4

This has one additional minor change to avoid applying a carried out margin to
an *inline* line, which is now needed since we can now move a bottom margin out
through an empty block to outside its parent.

Comment 21

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

Fix checked in, 2002-11-28 11:29 PST.