Closed Bug 538194 Opened 10 years ago Closed 4 years ago

non-floated block formatting context only checks top edge for overlap with floats rather than entire height

Categories

(Core :: Layout: Floats, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bugs, Assigned: dbaron)

References

(Depends on 3 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files)

I'll attach riven's testcases shortly, but they are fairly straightforward.
2 left floated divs

[ float  width 40% ]
[ float  width 70% ]
[ regular div ]

The last div expands to 60% of page abutting the 1st div, with the 2nd div laid out underneath it.
Attached file riven's testcase
Summary: non-floated div overlaps earlier floated div. Impacts Webkit and IE8 as well, but not IE7. → non-floated block formatting context overlaps earlier floated div. Impacts Webkit and IE8 as well, but not IE7.
I'm guessing if you switch the order of the floats, it will work correctly?

I think this is just the BFC-pushing variant of the block-variant of bug 25888 (which should perhaps be split into multiple bugs at some point).
Depends on: 25888
Er, the block-pushing variant of bug 25888 *is* block-formatting-context (BFC) related stuff only.  So it's only one level of variants and not two.
Attached file float order swapped.
Swapping does indeed render it correctly in Safari and Firefox, but that's consistent.
IE7 remains fairly consistent with its prior behaviour where the last div abuts the 2nd in both testcases.
I'm going to make this bug cover the "block part" of bug 25888, for which I've had work-in-progress patches for ages, and which I've been looking at again since I had to look at the code for bug 478834.

Note that this works correctly in Chromium these days, which increases the importance of fixing it.
Assignee: nobody → dbaron
Summary: non-floated block formatting context overlaps earlier floated div. Impacts Webkit and IE8 as well, but not IE7. → non-floated block formatting context only checks top edge for overlap with floats rather than entire height
Duplicate of this bug: 500736
... note that I initially posted the patch that will come here in bug 25888 comment 40.
Annoyingly, though, the patch I have fixes all the cases I tested *except* for the two in this bug.
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #9)
> Annoyingly, though, the patch I have fixes all the cases I tested *except*
> for the two in this bug.

Oh, wait, they are fixed; I just misunderstood what they were supposed to look like.

I do need to add a reftest for that case (and probably similar ones), though.
Bug 538194 patch 1 - Refactor some code dealing with fitting block formatting contexts around floats into separate functions.  r?roc
Attachment #8641384 - Flags: review?(roc)
Bug 538194 patch 2 - Redo block reflow below floats when the height of a block that does not intersect floats pushes it into the way of other floats.  r?roc

This adds an additional retry loop in block reflow that we can only
trigger when reflowing a block formatting context (replacedBlock
non-null).  It can retry in two different ways, either with a narrower
width but at the same vertical position (when
ReplacedBlockFitsInAvailSpace is still true) or at a new vertical
position (which is treated as a form of clearance).

Fortunately we don't have to worry about margins collapsing *through*
such a boundary since we're dealing with a new block formatting context.

Note that Chromium passes all of the new bfc-displace-* tests, although
it moves the block formating context down unnecessarily in
bfc-shrink-1.html (which we do neither before nor after the patch),
though agrees with the width we have after the patch (but not before the
patch).
Attachment #8641385 - Flags: review?(roc)
Comment on attachment 8641384 [details]
MozReview Request: Bug 538194 patch 1 - Refactor some code dealing with fitting block formatting contexts around floats into separate functions.  r?roc

https://reviewboard.mozilla.org/r/14479/#review13245

Ship It!

::: layout/generic/nsBlockReflowState.h:96
(Diff revision 1)
> +  bool AdvanceToNextBand(const mozilla::LogicalRect& aFloatAvailableSpace,

Document the return value.
Attachment #8641384 - Flags: review?(roc) → review+
Comment on attachment 8641385 [details]
MozReview Request: Bug 538194 patch 2 - Redo block reflow below floats when the height of a block that does not intersect floats pushes it into the way of other floats.  r?roc

https://reviewboard.mozilla.org/r/14481/#review13247

::: layout/generic/nsBlockFrame.cpp:3337
(Diff revision 1)
> +        aState.mFloatManager->PushState(&floatManagerState);

It's not obvious to me that this PushState is always balanced by a PopState --- in particular, if replacedBlock is true and mayNeedRetry is false.

Can you fix that, and make it obvious that the Push/Pops are balanced?
Attachment #8641385 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Can you fix that, and make it obvious that the Push/Pops are balanced?

They don't need to be balanced per:
https://hg.mozilla.org/mozilla-central/file/32712cd01159/layout/generic/nsFloatManager.h#l241

(And I don't think they're any less balanced with the patch than without it.)
Flags: needinfo?(roc)
Comment on attachment 8641385 [details]
MozReview Request: Bug 538194 patch 2 - Redo block reflow below floats when the height of a block that does not intersect floats pushes it into the way of other floats.  r?roc

https://reviewboard.mozilla.org/r/14481/#review13249

Ship It!
Attachment #8641385 - Flags: review+
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/b17337696896
https://hg.mozilla.org/mozilla-central/rev/80ef9bb2c2e9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 338526
Duplicate of this bug: 1092291
Depends on: 1222150
Depends on: 1222783
Depends on: 1213604
Depends on: 1231064
Depends on: 1236745
You need to log in before you can comment on or make changes to this bug.