418766 - {inc}Inconsistent layout in quirks mode (involves margin)

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Jesse Ruderman]

The testcase and reference have the same final DOM, so they should look the same. But in the testcase, there is an extra blank line above the 'b'.

I don't think this bug involves quirk.css rules, since I don't see them in DOM Inspector, and since I can't reproduce in standards mode with a copy of quirks.css in the page.

Comment 1

[Jesse Ruderman]

Attachment: testcase (dynamic)

Comment 2

[Jesse Ruderman]

Attachment: reference (static)

Comment 3

[Jesse Ruderman]

Refdyn found this with a bug 393655 testcase.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Huh.  This actually seems to be a reflow branch regression, given the regression range...

Comment 5

[Daniel Holbert [:dholbert]]

This bug is quirks-mode only because it's reproducibility depends on the zeroEffectiveSpanBox quirk, in nsLineLayout::VerticalAlignFrames.

Code reference:
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#1582


MORE DETAIL:
============

After we call the "boom()" JS function and remove the text, we hit the code below:

1662 if ((emptyContinuation ||
1663      mPresContext->CompatibilityMode() != eCompatibility_FullStandards) &&
...
1671       (0 == spanFramePFD->mMargin.bottom)
...
1688     zeroEffectiveSpanBox = PR_TRUE;
...
1698   psd->mZeroEffectiveSpanBox = zeroEffectiveSpanBox;


So in quirks mode, "psd->mZeroEffectiveSpanBox" is on at this point. Then, in the code below, this prevents us from turning on "applyMinLH", which thereby prevents us from clearing maxY and shrinking the first line.

In Standards mode, on the other hand, zeroEffectiveSpanBox was never turned on, and so we *do* turn on applyMinLH, and so we *do* hit the "minY = maxY = 0" line.

2045     PRBool applyMinLH = !(psd->mZeroEffectiveSpanBox); // (1) above
...
2075     if (applyMinLH) {
...
2106         minY = maxY = 0;

Hope that makes some sense.

Comment 6

[Daniel Holbert [:dholbert]]

Bug 393655 comment 13 and bug 393655 comment 14 both apply on this bug.

In particular, this chunk from bug 393655 comment 14 is also true for this bug's testcase, when we're running nsLineLayout::VerticalAlignFrames after the text removal.

> 1658   // XXXldb This should probably just use nsIFrame::IsSelfEmpty, assuming
> that
> 1659   // it agrees with this code.  (If it doesn't agree, it probably should.)
> 
> This bug's testcase is one instance where IsSelfEmpty does *not* agree with 
> this code.  That is,
>    spanFramePFD->mFrame->IsSelfEmpty() == PR_TRUE
>    zeroEffectiveSpanBox == PR_FALSE

Comment 7

[Daniel Holbert [:dholbert]]

Sorry, I confused myself -- the previous comment is wrong.

This...

>    spanFramePFD->mFrame->IsSelfEmpty() == PR_TRUE
>    zeroEffectiveSpanBox == PR_FALSE

... is only true here in a standards-mode version of the testcase.  And that makes sense, I think, because zeroEffectiveSpanBox is a quirks-mode thing.

Comment 8

[Daniel Holbert [:dholbert]]

Ok -- this is a situation where the "isEmpty || maybeWasEmpty" logic is failing us.

I'm talking about this section of nsBlockFrame.cpp:
http://lxr.mozilla.org/mozilla/source/layout/generic/nsBlockFrame.cpp#1924

1924       if (line.next() != end_lines()) {
1925         PRBool maybeWasEmpty = oldY == line.next()->mBounds.y;
1926         PRBool isEmpty = line->mBounds.height == 0 && line->CachedIsEmpty();
1927         if (maybeReflowingForFirstTime /*1*/ ||
1928             (isEmpty || maybeWasEmpty) /*2/3/4*/) {
1929           line.next()->MarkPreviousMarginDirty();
1930           // since it's marked dirty, nobody will care about |deltaY|
1931         }
1932       }

In the testcase, after we remove the text, maybeWasEmpty and isEmpty are BOTH false.  And it makes sense that they're false, I think.

Clearly maybeWasEmpty should be false, because the line started out with text in it

And isEmpty is false according to its criteria there, because the framedump shows that the line still has a height of 1140, not 0.

  line 0x91e47c0: ... {0,0,0,1140} <
     Inline(span)(0)@0x91e4604 ... {0,0,0,1140} ...<>
  >

I think it's ok that the line has height=1140, because it's got that height in the reference case, too, and we handle it correctly there.

And yet, we should still be marking the margin as dirty, because the line *is* empty.  If I force us to mark the margin as dirty at this point, we get correct behavior. (the 'b' shifts up)

I think we need to tweak the definition of "isEmpty", or add another variable, to handle the situation that we've got here.

Comment 9

[Daniel Holbert [:dholbert]]

roc had some suggestions for fixes in IRC:
 <roc> dholbert: you could remove the height==0 check
 <roc> dunno if that would hurt perf
 <roc> or you could replace it with a width==0 check

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: patch v1

This patch makes us test "width == 0" instead of "height == 0" as part of determining isEmpty. 

If I understand this code correctly, the zero-height check was originally intended as a quick-and-easy indicator of whether CachedIsEmpty() could possibly be true. (i.e. CachedIsEmpty is a bit expensive to call, and we don't want to call it if it's clearly going to return false)

This bug shows that testing for zero height is *NOT* a reliable indicator.  However, testing for zero *width* probably *is* a reliable indicator. (I think? roc, back me up here if you know for sure. :))

In other words, I'm assuming that in all situations where line->CachedIsEmpty() returns true, line->mBounds.width should be 0.

I'm going to run this through the reftest suite for a sanity check.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 306362 [details] [diff] [review]
patch v1

Just verified that patch v1 passes reftests.

Comment 12

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

Comment on attachment 306362 [details] [diff] [review]
patch v1

I agree with your analysis. Nonzero height is because empty spans acquire height from their font metrics. Width should be OK.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 306362 [details] [diff] [review]
patch v1

Requesting approval for including this in B4.

Justification:
 - Fixes layout regression
 - Changes layout engine => would benefit from beta exposure

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306362 [details] [diff] [review]
patch v1

a1.9b4=beltzner

Comment 15

[Daniel Holbert [:dholbert]]

Patch v1 checked in.

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.936; previous revision: 3.935
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/418766-1-ref.html,v
done
Checking in layout/reftests/bugs/418766-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/418766-1-ref.html,v  <--  418766-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/418766-1a.html,v
done
Checking in layout/reftests/bugs/418766-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/418766-1a.html,v  <--  418766-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/418766-1b.html,v
done
Checking in layout/reftests/bugs/418766-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/418766-1b.html,v  <--  418766-1b.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.378; previous revision: 1.377
done

Comment 16

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

Is the width of the line 0 or is it the width of the available space?  (That said, if it's the latter, we just wouldn't have the optimized check, right?)

Comment 17

[Daniel Holbert [:dholbert]]

It's the width of the line's contents, not the available space.

Comment 18

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

So there are cases where we can have 0 width but not be empty, I'd think -- like cases with negative text-indent or negative margin.  Or maybe zero-width inline-blocks or zero-width inlines, if they count as nonempty?  So is this optimization really valid?

Comment 19

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

Yes, because the check is &&, so we always test CachedIsEmpty before setting isEmpty to true.

For the optimization to be safe, we have to be sure that empty lines have zero width. That's always true as far as I know.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #10)
> In other words, I'm assuming that in all situations where line->CachedIsEmpty()
> returns true, line->mBounds.width should be 0.

I think bug 421239 was caused by this bug, because its testcase is one situation in which the above assumption fails.  There, line->mBounds.width is NONzero, and yet the line is empty according to CachedIsEmpty().

See that bug for further discussion.