377204 - Clearing multi-page floated elements works on screen but not when printing

Status: NEW

(Core :: Printing: Output, defect)

Description

[Ashley Bischoff (blog at handcoding.com)]

--------
Summary:
--------

It appears that tall floated elements can be cleared properly on screen but not necessarily when printing (or viewing through print-preview).

-------------------
Steps to Reproduce:
-------------------

I created a test cast which has two columns of text followed by a clearing element and then some more text -- something like this:


######           #########
######           #########
######           #########
######           #########
######           #########
######           
######           
######           
######           


##########################
##########################
##########################
##########################


Steps to reproduce:

1. Load the testcase: http://www.handcoding.com/documents/testing/floating-printing/floating-printing.html

2. Confirm that the two columns are floated and that the third block of text is cleared from the floated elements.

3. Run print preview

4. While the third block of text should still be cleared (in the print-preview window), it appears next to the first column of text

------------
More Detail:
------------

The clearing is done via a clearing element just after the two sets of columns which has the properties "clear:both" and "height:0". On screen, this works fine. However, when printing (or viewing through print-preview), that last block of text ends up jutted against the left-most column, as if the clearing hadn't occurred. 

I should also mention that the left-most column has enough text to span two pages.

Scenarios which prevent this bug from occurring:
* If the first column has less than a page's worth of text, the clearing works fine.
* If the clearing element has "clear:both" but not "height:0", the clearing works fine

----------
Build Info
----------

I saw this on both 20070411/Trunk (Minefield/3.0a4pre) and 20070411/Branch (BonEcho/2.0.0.4pre) on OSX. I also confirmed that this occurs on 20070411/Branch (BonEcho/2.0.0.4pre) on WinXP. (I didn't have a chance test 20070411/Branch on WinXP.)

Comment 1

[Ashley Bischoff (blog at handcoding.com)]

Oops -- that last sentence should say that I didn't have a chance test WinXP with 20070411/Trunk.

Comment 2

[fantasai]

Attachment: testcase

Tested on WinXP 20070420 as well. I think it's interesting that the clearing element's content disappears. It doesn't matter what value height is set to. The clearing element only clears the first page if a) its height is set to any fixed value or b) the clearing element is empty.

Comment 3

[fantasai]

Attachment: testcase2

The element also clears properly if it has more than one line (in this case no text disappears) *and* the height is nonzero.

Comment 4

[Eli Friedman]

Hmm, this is kind of difficult to deal with in our current architecture (at least in the zero-height clear case).  I'll think about it.

The fixed-height case is pretty easy to explain; it's caused by the messed up code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.838&mark=1268-1276#1268.)

Comment 5

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

(In reply to comment #4)
> Hmm, this is kind of difficult to deal with in our current architecture (at
> least in the zero-height clear case).  I'll think about it.

I think we could check whether the frames we're clearing-after are complete; if one or more is not complete, just push the clearing frame.

> The fixed-height case is pretty easy to explain; it's caused by the messed up
> code at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.838&mark=1268-1276#1268.)

So that should be fixed by fantasai's work then?

Comment 6

[Eli Friedman]

(In reply to comment #5)
> (In reply to comment #4)
> > Hmm, this is kind of difficult to deal with in our current architecture (at
> > least in the zero-height clear case).  I'll think about it.
> 
> I think we could check whether the frames we're clearing-after are complete; if
> one or more is not complete, just push the clearing frame.

Where would be a good place to check this?

> > The fixed-height case is pretty easy to explain; it's caused by the messed up
> > code at
> > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.838&mark=1268-1276#1268.)
> 
> So that should be fixed by fantasai's work then?
> 

No, it looks like that code is still around.  I'm not sure what you were trying to fix with it, though; the comment is completely wrong.  Is the "aMetrics.height > aReflowState.availableHeight" comparison flipped the wrong way?  (The way it's written, it has the effect of cutting off blocks that don't fit rather than expanding blocks that need to take up more space.)

Comment 7

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

This code is trying to handle the case where the content-rect of the block fits but the padding + border doesn't: the goal is to push the padding + border to the next page. We shouldn't be cutting off block content (i.e. lines) because the block was complete, so the content should fit into the available height (although I could easily believe there's a bug here).

> Where would be a good place to check this?

Maybe the space manager could remember if it has any incomplete floats? Then we could check that where we ask the space manager where the floats end.

Comment 8

[Eli Friedman]

> This code is trying to handle the case where the content-rect of the block fits
> but the padding + border doesn't: the goal is to push the padding + border to
> the next page. We shouldn't be cutting off block content (i.e. lines) because
> the block was complete, so the content should fit into the available height
> (although I could easily believe there's a bug here).

Okay, I understand the issue now; we should be doing the same thing as the other branch, and set aMetrics.height to
PR_MAX(aReflowState.availableHeight, aState.mY + nonCarriedOutVerticalMargin).  The current code leads to the truncation bit not getting set when it should be set.

> > Where would be a good place to check this?
> 
> Maybe the space manager could remember if it has any incomplete floats? Then we
> could check that where we ask the space manager where the floats end.

I was actually thinking the same thing.  I'm not sure where would be appropriate to check the flag, though.

Comment 9

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

nsBlockReflowState::ClearFloats and/or its callers?

Comment 10

[Eli Friedman]

Attachment: Patch part 1

Patch for the fixed-height case.

Comment 11

[Eli Friedman]

Checked in part 1.

Comment 12

[Eli Friedman]

Attachment: Patch part 2 WIP

WIP; does this look sane?

Comment 13

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

+        (aLine->HasClearance() && aState.mY + clearance > aState.mReflowState.availableHeight)) {

Why check this here? I think either we should check this before we reflow the child block, or (better) NS_INLINE_IS_BREAK_BEFORE(frameReflowStatus) should be true in this case. Although I guess it's a misapplication of NS_INLINE_IS_BREAK_BEFORE --- but the child block reflow should saying that none of it fit.

Comment 14

[Eli Friedman]

Attachment: Patch part 2 WIP 2

Does this look better?

Comment 15

[Eli Friedman]

Comment on attachment 272866 [details] [diff] [review]
Patch part 2 WIP 2

This is no good... the nsBlockReflowState change messes up floats in columns.

Comment 16

[fantasai]

Bug 377204 might be related here.

Comment 17

[fantasai]

and by bug 377204 I mean bug 507510...

Comment 18

[fantasai]

Attachment: simplified testcase in multicol

Nevermind, it is definitely a separate bug. Here's a testcase that uses fixed-height floats. The problem is that we don't push zero-height lines even if they have clearance that requires going to the next page.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jwatt, could you have a look please?
For more information, please visit auto_nag documentation.