405517 - {inc}Extra margin when adding text between P and DIV

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase: there should not be a blank line between 'b' and 'c'

Comment 1

[Jesse Ruderman]

Attachment: reference

Comment 2

[Jesse Ruderman]

I think this is a regression from within the last few weeks.  It causes a lot of failures in refdyn.

Comment 3

[Daniel Holbert [:dholbert]]

Might be from bug 393655?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Might be, yes.  That bug certainly lands inside the one-day regression range for this bug...

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix

Looks like I was wrong in https://bugzilla.mozilla.org/show_bug.cgi?id=393655#c31 -- we do need to keep the original condition (together with the new one) after all.

Haven't looked into it in detail, so I'm not sure yet exactly what was wrong.  But this patch fixes it.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase2 (JS tweak takes 5 sec to load)

Testcase using just divs (no <p>)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: reference2

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 290889 [details]
testcase2  (JS tweak takes 5 sec to load)

(btw: testcase2 takes 5 seconds to trigger the JS tweak.  Meant to reduce that before posting, but forgot to do so.)

Comment 9

[Daniel Holbert [:dholbert]]

Before the JS tweak, our frame tree looks like this:
          line 0x8cda938: {0,0,49920,1140} <
             [div with 600 app-unit margin]
                    "a"
            >
            line 0x8cda960: {0,1140,0,0} <
              Text(1)@0x8cda798[0,1,T]  {0,1740,0,0} <
                " "
              >
            >
            line 0x8cda988: {0,1740,49020,1140} <
              [subsequent div]
            >

Notice that the second line has a y-position of 1140 when it should be 1740.  (Its contents, an empty text frame, *does* have a y-position of 1740, but the line itself is at y=1140.)


The second line actually initially gets the right y-position, but then we subtract the 600 units of margin off in the "slideBy" call at line 4058 in nsBlockFrame::PlaceLine :

4043   if (!aLine->CachedIsEmpty()) {
...
4048   }
4049   else {
4050     // Don't let the previous-bottom-margin value affect the newY
4051     // coordinate (it was applied in ReflowInlineFrames speculatively)
4052     // since the line is empty.
4053     // We already called |ShouldApplyTopMargin|, and if we applied it
4054     // then BRS_APPLYTOPMARGIN is set.
4055     nscoord dy = aState.GetFlag(BRS_APPLYTOPMARGIN)
4056                    ? -aState.mPrevBottomMargin.get() : 0;
4057     newY = aState.mY + dy;
4058     aLine->SlideBy(dy); // XXXldb Do we really want to do this?
4059   }
4060 

URL: http://mxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#4058

As a result, that empty line's y-position does *not* equal the next line's y-position.  So the "oldY == line.next()->mBounds.y" check added in bug 393655 doesn't catch this, and it doesn't turn on "maybeWasEmpty".

Simply removing the slideBy call at 4058 fixes this bug, and I'm guessing from the XXXldb comment that we may not need that line.  I'm not sure if this change would break other things, though -- I'll try running it through reftests to see what happens.

Comment 10

[Daniel Holbert [:dholbert]]

Sorry -- the indentation of the first line in the frame tree was wrong in my last comment.  All three lines should be at the same indentation level.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v2

This patch just removes this line, as suggested in commment 9:
  4058     aLine->SlideBy(dy); // XXXldb Do we really want to do this?

Justification:
  - Enforces the invariant of "emptyLine.mBounds.mY == emptyLine.next.mBounds.mY" (which the patch to bug 393655 depends on)
  - Doesn't break margin-collapsing, which is the point of the clause it's in -- the other lines in that clause are in charge of margin-collapsing, but the removed line doesn't have any useful purpose towards that end, AFAICT.

This patch passes reftests, also.

(If anyone knows a reason why this line is needed, let me know -- if it is, we can just use attachment 290429 [details] [diff] [review] as a patch instead.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: reftest

reftest based on testcase2/reference2.

Fails pre 'fix v2', passes after.

Comment 13

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

Comment on attachment 290904 [details] [diff] [review]
fix v2

I think this is fine. The empty line could have floats but their position shouldn't be related to the position of the line box.

Comment 14

[Daniel Holbert [:dholbert]]

fix v2 checked in:

/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.898; previous revision: 3.897

Comment 15

[Jesse Ruderman]

Reftest checked in.