nemo Reporter
	Description • 15 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 2 • 15 years ago

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).

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 3 • 15 years ago

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.

	nemo Reporter
	Comment 4 • 15 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 5 • 9 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 7 • 9 years ago

... note that I initially posted the patch that will come here in bug 25888 comment 40.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 8 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29254bda8126

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 9 • 9 years ago

Annoyingly, though, the patch I have fixes all the cases I tested *except* for the two in this bug.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 10 • 9 years ago

(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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69cff4fd70ad

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 12 • 9 years ago

Bug 538194 patch 1 - Refactor some code dealing with fitting block formatting contexts around floats into separate functions.  r?roc

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 13 • 9 years ago

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).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 9 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 9 years ago

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?

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Assignee
	Comment 16 • 9 years ago

(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.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 9 years ago

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!

	Pulsebot
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b17337696896
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ef9bb2c2e9

	Carsten Book [:Tomcat]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/b17337696896
https://hg.mozilla.org/mozilla-central/rev/80ef9bb2c2e9

	Kohei Yoshino [:kohei]
	Comment 22 • 9 years ago

Added the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/css-float-bugs-have-been-fixed/

	Jean-Yves Perrier [:teoli]
	Comment 23 • 9 years ago

Added a brief note there: https://developer.mozilla.org/en-US/Firefox/Releases/42#CSS