274249 - Float is placed in the margin area

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Mats Palmgren (inactive)]

Float is placed in the margin area.

Regression occured 2004-11-25-06 -- 2004-11-26-06 (likely bug 209694)

STEPS TO REPRODUCE:
1. Load testcase

ACTUAL RESULTS:
The float is placed at the top, adjacent to the top black border.

EXPECTED RESULTS:
The float should be placed at the bottom, below the margin, and overflowing
the black box.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Mats Palmgren (inactive)]

Probably the same as bug 273946, marking depending for now...

Comment 3

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

Attachment: fix

The fix for bug 273946 did exactly the right thing to position the empty block.
The problem is that it makes us double-count the empty block's top margin: it's
included in the block's position (which the containing block then uses as the
starting point for the next line), but it's also included in the carried-out
bottom margin.

So we need the YMost of the empty block frame to be different from the
Y-coordinate where its parent block will place the next line. That's
straightforward to do for regular reflow but it's hard to do for incremental
reflow; when ReflowDirtyLines skips reflowing a line, it wants to use
line->mBounds.YMost() as the starting point for the next line, and we don't
have anywhere to record that for *this* line, you should use a different value.


My approach is to allow line->mBounds to be different from the block's
position; we back up line->mBounds by the empty block's mTopMargin, but leave
the block frame where it is. This gives us the correct placement of placeholder
frames but also lets the container block put subsequent lines in the right
place.

Note that line->mBounds could already be different from the block position due
to relative positioning.

Comment 4

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

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

>     if (needToRecoverState &&
>         (line->IsDirty() || line->IsPreviousMarginDirty())) {

Now this could just be:

if (needToRecoverState && line->IsDirty()) {


It might be good to add a little more of what you put in the previous comment
into the bit where you declare or set backupContainingBlockAdvance.  I
understood what this patch is doing from the bugzilla comment but not from the
source code comment (which I read first).

r+sr=dbaron

Comment 5

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

In particular, making the point that what you're doing is making the line bounds
different from the block's rect.

Comment 6

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

checked in