263586 - {inc}[MARGIN-C]block vertical margins broken on incremental reflow

Status: RESOLVED FIXED

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

Description

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

There were major regressions in handling of block vertical margins on
incremental reflow introduced by the changes in nsBlockFrame.h rev. 3.192 and
nsBlockFrame.cpp rev 3.665.

These cause, e.g., large amounts of vertical space when shift-Reloading
http://www.nytimes.com/ or http://www.imdb.com/title/tt0047034/

Comment 1

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

Attachment: near-minimal testcase

Looks like the deltaY computation is bogus

Comment 2

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

Attachment: fix

The immediate problem is that during incremental reflow, deltaY was being
computed after reflowing a line that had just been created by splitting an
existing line. That line's old bounds was (0,0,0,0) --- not where the line
really 'was' before reflow --- so the deltaY computation was bogus. That deltaY
got applied to the next line which was not dirty.

However, whenever we reflow a line for the first time, we should mark the next
line with MarkPreviousMarginDirty, which will force the line to be reflowed,
ensuring that the bogus deltaY value is not used. That wasn't happening in this
case because the calculation to determine that the line was being reflowed for
the first time --- 'oldY == 0 && deltaY != mBounds.y' was returning false! I
don't really understand how that expression was supposed to be doing the right
thing; it was old code. So I replaced it with a conservative approximation: say
that a line *may* be being reflowed for the first time if its initial bounds
are (0,0,0,0). This fixes the bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

+        line->mBounds.x == 0 && line->mBounds.y == 0 &&
+        line->mBounds.width == 0 && line->mBounds.y == 0;

Shouldn't that last one be height?

Comment 4

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

Comment on attachment 161603 [details] [diff] [review]
fix

r+sr with bz's comment, although I don't really understand what's going on
here.  (Did this have something to do with moving the deltaY assignment out of
the else?)

Comment 5

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

Thanks bz.

> Did this have something to do with moving the deltaY assignment out of the else?

Yes, but I'm not sure how this worked before, to be honest.

Comment 6

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

checked in.