258118 - The layout of sfgate.com regressed around August 26th

Status: RESOLVED FIXED

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

Description

[Stephen Moehle]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040904
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040904

The layout of sfgate.com is no longer correct.  Near the bottom of the page,
there is a gif with the work "MARKETPLACE."  Under that should be a table
consisting of the text "Chronicle Classifieds", "OnSale", "Chronicle Mobile",
"Custom Prints", etc.  In current trunk Mozilla builds, the table is rendered to
the right of where it should be, and if there is text in this right-hand column,
it will be overwritten by the table.

In Linux trunk build 2004-08-25-05, the layout is OK.  In Linux trunk build
2004-08-26-05, the layout is broken.

The first time you ever load the page, or immediately after clearing the cache,
the page will layout OK, but after reloading or revisiting the page, the layout
will be broken as described.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Stephen Moehle]

Attachment: Illustrates the incorrect layout

Comment 2

[Boris Zbarsky [:bzbarsky]]

There're several bugs that were checked in then that could in theory affect this:

Bug 256938 (not too likely), bug 256932, bug 256937 (not too likely), bug
133165, bug 252920, and bug 252771

A minimal testcase would go a long way towards narrowing down which of these is
responsible....

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 4

[Mats Palmgren (inactive)]

Backing out bug 252920 solves this.

Comment 5

[Mats Palmgren (inactive)]

To be precise, this fixes it:

       // Keep mY up to date in case we're propagating reflow damage
       // and also because our final height may depend on it. Only
       // update mY if the line is not empty, because that's what
       // PlaceLine does.
-      if (!line->IsEmpty()) {
+      if (PR_TRUE) {
         aState.mY = line->mBounds.YMost();
       }
       needToRecoverState = PR_TRUE;
     }

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Might this be similar to the "Find a Home or Apartment" regression on
http://channels.netscape.com/ns/homerealestate/default.jsp.

Comment 7

[Bernd]

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

Comment 8

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

Attachment: simpler testcase

The inner table is not needed.

Comment 9

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

Attachment: oops

the real testcase, sorry

Comment 10

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

Attachment: fix

nsBlockFrame::ReflowDirtyLines applies float clearing before the current line
if the previous line was not reflowed and has a float-clearing break type. Now,
if it so happens that we have a line L with a float-clearing break type,
followed by L2 which is empty, followed by L3, and we do a reflow in which only
L3 is dirty, we don't do any float clearing. This used to be covered up because
we'd update aState.mY with L2's YMost which would include the clearing height.
Now that we're not taking into account the YMost of empty lines in
ReflowDirtyLines, we need to ensure that the float clearing required by line L
is applied before we reflow L3. So this patch ensures that the break type is
propagated from any line requesting it to the next line to be reflowed.

It also cleans up the break type identification a little bit.

I'm suspicious about the correctness of CombineBreakType in the cases (NONE,
LEFT) and (NONE, RIGHT)...

Comment 11

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

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

regression fix

Comment 12

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

Attachment: revised

I suppose we should clear floatBreakType when we set mY in the !line->IsEmpty()
non-reflow case, since that will include any necessary clearing.

Comment 13

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

Comment on attachment 158277 [details] [diff] [review]
revised

rubber-stamp r+sr

Comment 14

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

checked in

Comment 15

[Stephen Moehle]

I'll confirm that sfgate.com is fixed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

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

This caused regression bug 261153 and perhaps also bug 260938.