295690 - {inc}Incremental reflow bug when 100% width float is removed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[fejesjoco]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217

See the URL, see the attached file. Whenever I go to this page, there's a gap
between the top and the rest of the page. If I move the mouse over some links,
it disappears, sometimes it doesn't. There are similar bugs accross the whole
website.
Some strange things also happen with Firefox 1.0.4 under Debian SID, but not
that big. Some links, the top of the page or some other parts can move around,
just a fix pixels. It's in connection with mouse movement over some links, too.


Reproducible: Always

Steps to Reproduce:

Comment 1

[fejesjoco]

Attachment: screenshot of the bug

Comment 2

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050526
Firefox/1.0+ ID:2005052616
I see this too. The page sometimes renders with a gap, but then changing
fontsize (CTRL++ or CTRL+-) and the gap disappears. Some kind of reflow problem?

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

Comment 4

[Steve England [:stevee]]

Works in Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8)
Gecko/20050509 Firefox/1.0.4, so this is a regression too.

Comment 5

[Martijn Wargers (dead)]

Yes, a regression, works in 2004-08-09 build, fails in 2004-08-10 build:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-08-09+06%3A00%3A00&maxdate=2004-08-10+09%3A00%3A00&cvsroot=%2Fcvsroot
Because of bug 230170?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal testcase for the reflow bug

This requires the Ahem font (see http://www.hixie.ch/resources/fonts/)

The patch for bug 230170 did expose this incremental reflow issue in this
particular circumstance...  but the issue is there no matter what (this
testcase shows it in builds from before bug 230170 landed).  The key is that
the thing following the floats is laid out before the floats are removed; if
that happens, we see the bug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

roc, any idea what's up here offhand?

Comment 8

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

Attachment: fix

Basically, it's not enough to just dirty all lines after the float in this
block when we remove a float. We also need to dirty lines inside those lines.
And in fact we need to dirty lines after the float that belong to any ancestor
blocks that share the same space manager. Maybe there's a better way to do
this, like fire a restyle reflow at the space manager block?

Comment 9

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

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

Wouldn't it be much simpler, more correct (since it would handle the case of
lines that are descendants of ancestors), and much more efficient when multiple
floats are removed to just call nsSpaceManager::IncludeInDamage with an
appropriate region?  (Is an appropriate region known at this point?  Could we
fake one?)

Comment 10

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

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

Oh, right, we don't have a space manager at this point.  (But maybe we should.)

Comment 11

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

Yeah, I actually thought of that :-).

It wouldn't be more correct ... I think this patch handles the ancestors and
cousins properly.

Comment 12

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

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

fix for layout regression

Comment 13

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

checked in

Comment 14

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050629
Firefox/1.0+ ID:2005062903
The "Minimal testcase for the reflow bug" is now fixed for me, but "testcase" is
not.

Comment 15

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

Attachment: additional fix

This fixes the other testcase. If we delete a block child that contains a
float, then we need to dirty all the space manager's lines.

Comment 16

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

Comment on attachment 188551 [details] [diff] [review]
additional fix

>+  nsLineList::iterator line = block->begin_lines();
>+  nsLineList::iterator endLine = block->end_lines();
>+  while (line != endLine) {
>+    if (line->IsBlock() && BlockHasAnyFloats(line->mFirstChild))
>+      return PR_TRUE;
>+    ++line;

I tend to prefer for loops for things like this, but it's ok either way.

Comment 17

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

Comment on attachment 188551 [details] [diff] [review]
additional fix

fixes incremental reflow layout regression

Comment 18

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

checked in