Closed Bug 295690 Opened 19 years ago Closed 19 years ago

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

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: fejesjoco, Assigned: roc)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [reflow-refactor])

Attachments

(5 files)

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:
Attached image screenshot of the bug
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?
Attached file testcase
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Assignee: general → nobody
Component: General → Layout
Product: Mozilla Application Suite → Core
QA Contact: general → layout
Version: unspecified → 1.0 Branch
Version: 1.0 Branch → Trunk
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.
roc, any idea what's up here offhand?
Summary: strange bugs, maybe involving css, a:hover → {inc}Incremental reflow bug when 100% width float is removed
Whiteboard: [reflow-refactor]
Attached patch fixSplinter Review
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?
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #186562 - Flags: superreview?(dbaron)
Attachment #186562 - Flags: review?(dbaron)
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?)
Attachment #186562 - Flags: superreview?(dbaron)
Attachment #186562 - Flags: superreview-
Attachment #186562 - Flags: review?(dbaron)
Attachment #186562 - Flags: review-
Comment on attachment 186562 [details] [diff] [review]
fix

Oh, right, we don't have a space manager at this point.  (But maybe we should.)
Attachment #186562 - Flags: superreview?(dbaron)
Attachment #186562 - Flags: superreview-
Attachment #186562 - Flags: review?(dbaron)
Attachment #186562 - Flags: review-
Yeah, I actually thought of that :-).

It wouldn't be more correct ... I think this patch handles the ancestors and
cousins properly.
Attachment #186562 - Flags: superreview?(dbaron)
Attachment #186562 - Flags: superreview+
Attachment #186562 - Flags: review?(dbaron)
Attachment #186562 - Flags: review+
Comment on attachment 186562 [details] [diff] [review]
fix

fix for layout regression
Attachment #186562 - Flags: approval1.8b3?
Attachment #186562 - Flags: approval1.8b3? → approval1.8b3+
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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.
Attached patch additional fixSplinter Review
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.
Attachment #188551 - Flags: superreview?(dbaron)
Attachment #188551 - Flags: review?(dbaron)
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.
Attachment #188551 - Flags: superreview?(dbaron)
Attachment #188551 - Flags: superreview+
Attachment #188551 - Flags: review?(dbaron)
Attachment #188551 - Flags: review+
Comment on attachment 188551 [details] [diff] [review]
additional fix

fixes incremental reflow layout regression
Attachment #188551 - Flags: approval1.8b4?
Attachment #188551 - Flags: approval1.8b4? → approval1.8b4+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: