386147 - Don't cut off floats at the bottom of the page

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Eli Friedman]

Attachment: Partial fix

Simple testcase:
<div style="font-size: 1in">
<div style="float:left;">
a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br>a<br></div>
<div style="float:left">
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb

The issue with the partial fix is that it doesn't actually make this testcase render quite right; it seems the continued float ends up with a width of zero for some reason I haven't figured out.

Comment 1

[Eli Friedman]

The rest of the problem is really fun: continuations have a preferred width of zero!  (The reason is that continuations don't have any children until they are reflowed.)

dbaron: any suggestions as to the correct way to fix this?

Comment 2

[Eli Friedman]

Comment on attachment 270141 [details] [diff] [review]
Partial fix

(Since this part of the fix is independent, I might as well get it reviewed separately.)

Comment 3

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

Shouldn't the continuation automatically use the same width as the prev-in-flow?  Though maybe we should make its intrinsic width methods work anyway.

That said, we need to make the first-in-flow's intrinsic width calculation function consider the contents of the next-in-flows once we've already done a reflow.  (Consider incremental reflow inside columns.)

Comment 4

[Eli Friedman]

(In reply to comment #3)
> Shouldn't the continuation automatically use the same width as the
> prev-in-flow?  Though maybe we should make its intrinsic width methods work
> anyway.

Possibly, but maybe not; if we ever support anything like putting even pages into landscape, we wouldn't want that.  It's not too hard anyway; I think we can just make the method forward to the first-in-flow.

> That said, we need to make the first-in-flow's intrinsic width calculation
> function consider the contents of the next-in-flows once we've already done a
> reflow.  (Consider incremental reflow inside columns.)

Right... maybe add nsBlockFrame::GetMin/PrefWidthInternal and call through all the continuations from the first-in-flow?

I'd like to know whether this approach sounds reasonable before I start coding it.

Comment 5

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

That would work -- not sure if there's even a need for an extra method.  You could just get the first-in-flow and use its member variables, and then make the loop loop over in-flows.

You'd also need to fix MarkIntrinsicWidthsDirty to mark them dirty on the first-in-flow.

Comment 6

[Eli Friedman]

Attachment: Second part

Okay, patch using outlined method.  (I'll also attach a diff -w to make reviewing easier.)

Comment 7

[Eli Friedman]

Attachment: Second part (diff -w)

Comment 8

[Eli Friedman]

First part checked in.

Comment 9

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

Comment on attachment 270200 [details] [diff] [review]
Second part

I think you want to traverse the continuation chain, not the in-flow chain.  Currently I don't think there's a difference, but if we implement forced breaks at some point in a way that causes blocks to be continuations but not in-flows, we'd want this to traverse them too.  (And we'd certainly want that for text-indent, so if you don't make that change, you need to undo the GetPrevContinuation() changes you made regarding text-indent.)

> #ifdef IBMBIDI
>-  ResolveBidi();
>+ 
> #endif // IBMBIDI

Remove the ifdefs, too?

r+sr=dbaron with that

Comment 10

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

And thanks for cleaning up after me on this one, and sorry for the delay reviewing it.

Comment 11

[Eli Friedman]

Attachment: Second part final version

Comment 12

[Eli Friedman]

Checked in with reftest.