Closed Bug 447660 Opened 11 years ago Closed 5 years ago

Reenable float breaking in columns

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: roc, Assigned: mats)

References

Details

Attachments

(3 files)

We turned this off in bug 413048. One day when it's safe to turn it on again, we should probably do so.
Depends on: 492627
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
Depends on: 1108104
Fixes this assertion:
overflow container must not have computedBSizeLeftOver: '!( IS_TRUE_OVERFLOW_CONTAINER(this) && computedBSizeLeftOver )', layout\generic\nsBlockFrame.cpp, line 7199
Attachment #8535956 - Flags: review?(roc)
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)
Attachment #8535960 - Flags: review?(roc)
Attachment #8535955 - Flags: review?(roc) → review+
Attachment #8535956 - Flags: review?(roc) → review+
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
Attachment #8535960 - Flags: review?(roc) → review+
Filed bug 1148104 to enable this in RELEASE builds and let it ride the trains.
Blocks: 1148104
Depends on: 1153695
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. ;)
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!
Flags: needinfo?(npancholi)
Blocks: 1411765
(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.
Flags: needinfo?(npancholi)
You need to log in before you can comment on or make changes to this bug.