447660 - Reenable float breaking in columns

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

We turned this off in bug 413048. One day when it's safe to turn it on again, we should probably do so.

Comment 1

[Mats Palmgren (inactive)]

We've fixed a ton of bugs in the past six years so I think we should try to enable this
again behind a pref.  I pushed a Try run (together with the fixes in bug 1108104):
https://tbpl.mozilla.org/?tree=Try&rev=b517a952e46d

There are a few assertions that we need to fix before enabling it though:

layout/generic/crashtests/404219-2.html
layout/generic/crashtests/468207-1.html
layout/generic/crashtests/600100.xhtml
layout/generic/crashtests/810726.html
layout/generic/crashtests/840787.html

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - Replace the #define DISABLE_FLOAT_BREAKING_IN_COLUMNS with a pref to enable fragmenting of floats inside columns. Set the pref enabled by default in non-RELEASE builds only.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - Mark an existing next-in-flow of a float that is split due to it being NOT_COMPLETE as NOT being an overflow container.

Fixes this assertion:
overflow container must not have computedBSizeLeftOver: '!( IS_TRUE_OVERFLOW_CONTAINER(this) && computedBSizeLeftOver )', layout\generic\nsBlockFrame.cpp, line 7199

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - When collecting / pushing a float, make sure to also pick up its next-in-flows that are in same block.

This fixes a few bugs when pushing/pulling float continuations between columns.
(see the patch description for details)

https://tbpl.mozilla.org/?tree=Try&rev=e6ec37ed6cca
(other tests were green in an earlier run, so just ref/crashtests here)

Comment 5

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

Comment on attachment 8535960 [details] [diff] [review]
part 3 - When collecting / pushing a float, make sure to also pick up its next-in-flows that are in same block.

Review of attachment 8535960 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBlockFrame.cpp
@@ +7354,5 @@
> +      nsIFrame* nif = e.get()->GetNextInFlow();
> +      MOZ_ASSERT(!nif || (!mFloats.ContainsFrame(nif) && !mFrames.ContainsFrame(nif)));
> +    }
> +  }
> +  

trailing whitespace

Comment 6

[Mats Palmgren (inactive)]

> trailing whitespace

Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c8aa35169bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c248025d37
https://hg.mozilla.org/integration/mozilla-inbound/rev/56d432ad6ed6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c8aa35169bf
https://hg.mozilla.org/mozilla-central/rev/b9c248025d37
https://hg.mozilla.org/mozilla-central/rev/56d432ad6ed6

Comment 8

[Mats Palmgren (inactive)]

Filed bug 1148104 to enable this in RELEASE builds and let it ride the trains.

Comment 9

[fantasai]

I don't really understand why the following change was made in patch #2, since rewrapping a line of text and replacing comments with questions seems unrelated to any of the code changes to fixing this bug.

-  if (NS_FRAME_IS_NOT_COMPLETE(*aStatus)
-      && aFinalSize.BSize(wm) < aReflowState.AvailableBSize()) {
-    // We ran out of height on this page but we're incomplete
-    // Set status to complete except for overflow
+  if (NS_FRAME_IS_NOT_COMPLETE(*aStatus) &&
+      aFinalSize.BSize(wm) < aReflowState.AvailableBSize()) {
+    // We fit in the available space - change status to OVERFLOW_INCOMPLETE.
+    // XXXmats why didn't Reflow report OVERFLOW_INCOMPLETE in the first place?
+    // XXXmats and why exclude the case when our size == AvailableBSize?
     NS_FRAME_SET_OVERFLOW_INCOMPLETE(*aStatus);
   }
Imho the new comment is less helpful than the original: our content doesn't fit, so we're not complete and need a continuation to continue placing our content--but we ran out of computed height, that is why we need to change the reflow status to OVERFLOW_INCOMPLETE to indicate this state.

To answer the first question, Reflow didn't report OVERFLOW_INCOMPLETE rather than NOT_COMPLETE because it didn't know we ran out of computed height: we only know that after we compute the height in this function.

To answer the second question, this seems like an error and likely should be, as you seem to suspect, <= rather than < in the comparison.

p.s. Probably questions should be directed to the person responsible for answering rather than left in the source for her to discover several years later. ;)

Comment 10

[Jet Villegas (inactive)]

Neerja: @fantasai graciously posted a patch for fixing the issue in comment 9 here:
https://public.etherpad-mozilla.org/p/patches-accepted

Can you file a follow-up bug and get it reviewed/landed? Thx!

Comment 11

[Neerja Pancholi[:neerja]]

(In reply to Jet Villegas (:jet) from comment #10)
> Neerja: @fantasai graciously posted a patch for fixing the issue in comment
> 9 here:
> https://public.etherpad-mozilla.org/p/patches-accepted
> 
> Can you file a follow-up bug and get it reviewed/landed? Thx!

Created follow up Bug 1411765 for this.