Support fragmentation (for printing, multicol) of flex containers (flexbox)

RESOLVED FIXED in mozilla28

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla28
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 5 obsolete attachments)

1.27 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
66.09 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
6.94 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
7.86 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
3.40 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
24.30 KB, patch
Mats Palmgren (vacation - back in August)
: review+
Details | Diff | Splinter Review
1.54 KB, text/html
Details
574 bytes, text/html
Details
(Assignee)

Description

5 years ago
Filing this bug on adding pagination support to our CSS3 Flexbox implementation.
(Assignee)

Updated

5 years ago
Summary: Support pagination of CSS3 Flexbox → Support pagination (printing) of CSS3 Flexbox
(Assignee)

Updated

4 years ago
Depends on: 870555
(Assignee)

Updated

4 years ago
Blocks: 856235

Comment 1

4 years ago
For both columns and printing?
(Assignee)

Comment 2

4 years ago
Correct.
(Assignee)

Updated

4 years ago
Depends on: 885424
(Assignee)

Updated

4 years ago
Depends on: 917032
(Assignee)

Updated

4 years ago
Depends on: 918519
(Assignee)

Updated

4 years ago
Summary: Support pagination (printing) of CSS3 Flexbox → Support fragmentation (for printing, multicol) of CSS Flexbox
(Assignee)

Comment 3

4 years ago
Needless to say, this will be a multi-patch bug. Not all patches are ready yet, but I'm going to attach an initial set of (targeted) patches for review now, and several more over the next day or two.
(Assignee)

Comment 4

4 years ago
Created attachment 807489 [details] [diff] [review]
part 1: Add chunk to nsCSSFrameConstructor::CreateContinuingFrame

This adds a flex-container-specific chunk to nsCSSFrameConstructor::CreateContinuingFrame.

This is needed in order for a continuation to be successfully created, when a flex container returns an incomplete status (which doesn't happen yet, but will start happening as of the next few patches).
Attachment #807489 - Flags: review?(matspal)
(Assignee)

Comment 5

4 years ago
Created attachment 807498 [details] [diff] [review]
[wrong patch; disregard]

This patch fixes ComputeFlexContainerMainSize() so that vertical flex containers (i.e. flex containers whose height is their main size) will be split if they have a fixed height which is larger than the available size.

(This also makes us use GetEffectiveComputedHeight() instead of ComputedHeight() to get the height of each of the continuations resulting from this split. Otherwise, each continuation would incorrectly think it's supposed to get the full computed height.)

This leaves an XXXdholbert comment behind to fix up the auto-height code-path there, where we shrinkwrap our children -- that's a little more complex, so I'm saving it for a later patch.
Attachment #807498 - Flags: review?(matspal)
(Assignee)

Comment 6

4 years ago
Created attachment 807501 [details] [diff] [review]
part 2: Split vertical flex containers with a fixed main-size (height) that's larger than available height

[sorry, attached the wrong patch. Here's the one I meant to attach.]
Attachment #807498 - Attachment is obsolete: true
Attachment #807498 - Flags: review?(matspal)
Attachment #807501 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Attachment #807498 - Attachment description: part 2: Split vertical flex containers with a fixed main-size (height) that's larger than available height → [wrong patch; disregard]
(Assignee)

Comment 7

4 years ago
Created attachment 807506 [details] [diff] [review]
part 3:  Split horizontal flex containers whose cross size (height) is larger than available height

...and this patch fixes the code that computes flex container *cross* sizes, so that horizontal flex containers (whose height is their cross size) will be split if their height is larger than the available size.

(Incidentally, this is the patch that I filed bug 918519 to help with -- thanks to that bug's patches, the GetEffectiveComputedHeight() call here will only get evaluated when we need it.)
Attachment #807506 - Flags: review?(matspal)
(Assignee)

Comment 8

4 years ago
Created attachment 807511 [details] [diff] [review]
part 2 v2: Split vertical flex containers with a fixed main-size (height) that's larger than available height

[actually: I just caught a bug in part 2 -- we need to make sure that the borderpadding subtraction doesn't push "availableHeightForContent" into negative territory, or else we won't end up asking for a big enough aDesiredSize.height in cases with huge border/padding. Fixed in this updated version, with std::max(), and I'll include a reftest for this in my upcoming "empty flex container" reftests patch.]
Attachment #807501 - Attachment is obsolete: true
Attachment #807501 - Flags: review?(matspal)
Attachment #807511 - Flags: review?(matspal)
(Assignee)

Comment 9

4 years ago
Created attachment 807513 [details] [diff] [review]
part 4: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation

This patch fixes the aDesiredSize computation to explicitly only ask for bottom border/padding if we're complete.

(It also makes us check whether that border/padding pushes us over the available height, in which case we *don't* ask for it, and we mark ourselves incomplete to get a continuation for the border/padding. UNLESS we're already asking for 0 height, in which case we just bite the bullet and accept the border/padding, to avoid getting an infinite chain of 0-height continuations none of which can fit the border/padding.)
Attachment #807513 - Flags: review?(matspal)
(Assignee)

Comment 10

4 years ago
With the patches attached so far, we support splitting empty fixed-height flex containers with various combinations of border/padding/margin -- this ends up giving us correctly-sized/positioned continuations. (as determined by comparing against equivalent content with an empty block instead of an empty flex container.)

The next patch will be a reftests patch, asserting that this is true.
(Assignee)

Updated

4 years ago
Attachment #807513 - Attachment description: 811024-onlyGiveBottomBorderPaddingToCompleteFrames.patch → part 4: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation
(Assignee)

Comment 11

4 years ago
Created attachment 807621 [details] [diff] [review]
part 5: reftests with empty flex containers

Here are two sets of reftests: flexbox-empty-1{a-h}.html and flexbox-empty-2{a-d}.html.

Overview:
 - The flexbox-empty-1{a,b,c,d}.html tests each have a series of flex containers overflowing their first column by small amounts, due to a combination of content, margin, border, or padding. The four tests only differ from each other in their "flex-direction" property. (making them horizontal vs. vertical, and reversed-direction or not)

 - The flexbox-empty-1{e,f,g,h}.html tests are the same, but with the flex container being shoved into an unstyled tiny-height block container. This just means the flex containers' second-column continuation frames end up being in an overflow container, instead of being directly in the multicol's second column. The rendering remains the same. (Hence, still compared against flexbox-empty-1-ref.html)
 
 - The flexbox-empty-2{a,b,c,d}.html tests are similar to flexbox-empty-1{a,b,c,d}.html, but now the margin/border/padding values are *themselves* larger than the available height; they exercise the "don't let available height end up below 0" code that I fixed in comment 8 here.

The reference cases are identical to the corresponding testcases, but with "display:flex" and "flex-direction: [whatever]" removed, giving us block containers in place of flex containers.
Attachment #807621 - Flags: review?(matspal)

Comment 12

4 years ago
Is this bug a prerequisite for Bug 702508? It should be added to the dependency list, then. Thanks!
Attachment #807489 - Flags: review?(matspal) → review+
Attachment #807506 - Flags: review?(matspal) → review+
Comment on attachment 807511 [details] [diff] [review]
part 2 v2: Split vertical flex containers with a fixed main-size (height) that's larger than available height

>layout/generic/nsFlexContainerFrame.cpp
>-  if (aReflowState.ComputedHeight() != NS_AUTOHEIGHT) {
>-    return aReflowState.ComputedHeight();
>+  nscoord effectiveComputedHeight = GetEffectiveComputedHeight(aReflowState);
>+  if (effectiveComputedHeight != NS_INTRINSICSIZE) {

I know they are all the same value, but I think this should be
NS_UNCONSTRAINEDSIZE.  I don't think NS_INTRINSICSIZE should be used
in contexts where the box isn't intrinsically sized in the CSS sense -
images, replaced elements and the like.


>+  nscoord availableHeightForContent = aReflowState.availableHeight;
>+  if (availableHeightForContent != NS_UNCONSTRAINEDSIZE &&
>+      !(GetSkipSides() & (1 << NS_SIDE_TOP))) {
>+    availableHeightForContent -= aReflowState.mComputedBorderPadding.top;
>+    // (Don't let that push availableHeightForContent below zero, though):
>+    availableHeightForContent = std::max(availableHeightForContent, 0);

Don't you need to take the bottom side into account as well?
I see that other code use something like this:

  nsMargin bp = aReflowState.mComputedBorderPadding;
  ApplySkipSides(bp, &aReflowState);
  availableHeightForContent =
    std::max(0, availableHeightForContent - bp.TopBottom());

Perhaps we should add a GetAvailableHeightForContent method somewhere
that does that.
Attachment #807511 - Flags: review?(matspal) → review+
Comment on attachment 807513 [details] [diff] [review]
part 4: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation

>+  // Now: If we're complete, add bottom border/padding to desired height
>+  // (unless that pushes us over available height, in which case we become
>+  // incomplete

I think this is wrong.  The Fragmentation spec says that breaks should not
occur between the content and padding.
http://dev.w3.org/csswg/css-break/#possible-breaks

If we're complete but the padding overflows isn't that something that the
parent should deal with?  (by creating an overflow container).

>+    // NOTE: We use can't use containerBorderPadding.bottom for this, because

s/We use can't use/We can't use/

I don't think you should change the reflow status in this situation.

Does any of the tests fail without this patch? If so, what happens?
Attachment #807513 - Flags: review?(matspal) → review-
Attachment #807621 - Flags: review?(matspal) → review+
(Assignee)

Comment 15

4 years ago
(In reply to Florian Bender from comment #12)
> Is this bug a prerequisite for Bug 702508?

Not directly (i.e. fixing bug 702508 doesn't technically require this bug to be fixed), but they both require rearranging/rewriting chunks of the same area of code, so it wouldn't really be feasible to implement them separately in parallel. Hence, I'm finishing this one off first, and then working on bug 702508.
(Assignee)

Comment 16

4 years ago
(In reply to Mats Palmgren (:mats) from comment #13)
> Comment on attachment 807511 [details] [diff] [review]
> >+  nscoord effectiveComputedHeight = GetEffectiveComputedHeight(aReflowState);
> >+  if (effectiveComputedHeight != NS_INTRINSICSIZE) {
> 
> I know they are all the same value, but I think this should be
> NS_UNCONSTRAINEDSIZE.

I'm checking GetEffectiveComputedHeight()'s return value against NS_INTRINSICSIZE because that's what GetEffectiveComputedHeight() *itself* internally checks for and returns.  If you think that's wrong, perhaps we should change GetEffectiveComputedHeight() first?

> >+    availableHeightForContent -= aReflowState.mComputedBorderPadding.top;
> >+    // (Don't let that push availableHeightForContent below zero, though):
> >+    availableHeightForContent = std::max(availableHeightForContent, 0);
> 
> Don't you need to take the bottom side into account as well?

Hmm... I don't think so. "availableHeight" is really "distance until the page-break", and our bottom border/padding doesn't influence that.  (Our *top* border/padding does influence that, though, from the perspective of our contents, because it shifts them down towards the page-break.)
(Assignee)

Comment 17

4 years ago
(In reply to Mats Palmgren (:mats) from comment #14)
> Comment on attachment 807513 [details] [diff] [review]
> >+  // Now: If we're complete, add bottom border/padding to desired height
> >+  // (unless that pushes us over available height, in which case we become
> >+  // incomplete
> 
> I think this is wrong.  The Fragmentation spec says that breaks should not
> occur between the content and padding.
> http://dev.w3.org/csswg/css-break/#possible-breaks

Hmm... I'm not so sure.  For one thing, that spec section starts out "In block-and-inline flow, ..." So it doesn't necessarily apply here, since this isn't block-and-inline flow. :)

Disregarding that, though -- it also says: "When paginating, if there are no possible break points below the top of the page, and not all the content fits, the UA may break anywhere in order to avoid losing content off the edge of the page."  For empty flex containers at least, I think that means the current behavior (breaking between content-box and padding-box) makes sense.  We don't have any *allowed* break points, so we break at the bottom of the content-box because there's no better place to break.  (Certainly we shouldn't break midway through the border/padding; I suppose we also *could* break at any arbitrary point in the middle of the content box if we wanted, but there's no reason to prefer any particular spot over any other, so might as well just break at the bottom of the content-box.)

When we're dealing with a flex container that has children, though, perhaps this means we should prefer pushing (or splitting?) the last child, rather than splitting between the last child and the container's content-edge? (unless there's a gap there, in which case the spec allows it)  Anyway, that's outside the scope of this patch.

> If we're complete but the padding overflows isn't that something that the
> parent should deal with?  (by creating an overflow container).

The parent won't know whether it's our padding vs. content that's overflowing -- it'll just see us requesting a larger height than the availableHeight that it gave us.  It only has two ways to handle that: simply letting us overflow (stomping on its border & potentially on later content), or creating a continuation for us.  We'd prefer the latter, but it'll only make a continuation for us if we return an incomplete reflow status. Hence the status-tweak.

(Also: you mentioned overflow containers -- those are actually kinda orthogonal to what's going on here; they don't solve this "oops, our padding makes us overflow" problem. We'd get an overflow container if we end up taller than our *parent's* frame *and* we return an incomplete status (i.e. we're also taller than the available height, so we ask our parent for a continuation). Importantly, we can only get put into an overflow container if we return an incomplete status; otherwise, we're signalling that we can't split, and we'll simply render with our bottom border/padding sticking out of the parent.)

> s/We use can't use/We can't use/

Thanks, fixed!

> I don't think you should change the reflow status in this situation.

We have to report an incomplete reflow status, in order to get a next-continuation to put our bottom border/padding into. Otherwise, we have to put it on our current continuation (where we know it doesn't fit), and it'll just stick out of the bottom, potentially covering up other stuff, stomping on the multicol's own border (or potentially rendering somewhere off the bottom of a page, when printed).

> Does any of the tests fail without this patch? If so, what happens?

Yes. We fail the last 5 subtests of each flexbox-empty-1* testcase -- those are the ones where there's border and/or padding on the bottom of a flex container, pushing it to be taller than the column that it's in. Without this patch, the flex container ends up rendering as a single frame, too tall for its multicol container, with its bottom border/padding stomping on the multicol's bottom-border.

The rendering looks something like this (just with bottom-border here):
 m    f
 m    f
 m    f  
 m    fb         LEGEND: "m" = left edge of multicol elem
 m mb fb  <-.             "mb"= multicol bottom-border
 m mb mb    |             "f" = flex container (one pixel wide, for simplicity)
 m mb mb    |             "fb"= flex container bottom-border
 m mb mb    |
         flex container bottom border stomps on its multicol's border here.  
         

In contrast, if we replace the flex container with a block container (as in the reference case), we get something like this:

 m    k  |   kb
 m    k  |   kb
 m    k  |
 m                LEGEND: "m" = left edge of multicol elem
 m mb mb mb  mb           "mb"= multicol bottom-border
 m mb mb mb  mb           "k" = block (one pixel wide, for simplicity
 m mb mb mb  mb           "kb"= block bottom-border
 m mb mb mb  mb            |  = column-rule

    (notice that the block's border(/padding) has been put in a continuation,
    leaving a little space at the bottom of the first continuation,
    because the whole border/padding chunk couldn't fit at the end of the
    first continuation.)
(Assignee)

Comment 18

4 years ago
Mats, does Comment 17 address your concerns about Part 4? [If so: is that r+ with the "We use can't use" typo-fix?]

(And are you OK with NS_INTRINSICSIZE in part 3, given Comment 16?)
Flags: needinfo?(matspal)
Comment on attachment 807513 [details] [diff] [review]
part 4: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation

Sorry for the delay.  I wanted to take another look since I misunderstood
the code the first time around, but I haven't had time to do that yet.
I'll get to it the next couple of days.
Attachment #807513 - Flags: review- → review?(matspal)
Comment on attachment 807513 [details] [diff] [review]
part 4: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation

Yeah, this looks reasonable and is in fact how block frames behave too.

Please file a followup bug on adding support for page-break-inside:avoid
unless there is one already.  It should be easy to fix, just add
something like:

      if (ShouldAvoidBreakInside(aReflowState)) {
        *aStatus = NS_INLINE_LINE_BREAK_BEFORE();
        return NS_OK;
      }

before setting an INCOMPLETE status in the patches your have here.
Attachment #807513 - Flags: review?(matspal) → review+
Flags: needinfo?(matspal)
(Assignee)

Updated

4 years ago
Blocks: 935210
(Assignee)

Comment 21

4 years ago
(In reply to Mats Palmgren (:mats) from comment #20)
> Please file a followup bug on adding support for page-break-inside:avoid

Thanks -- filed bug 935210 to cover that.

Also: as I mentioned at the Paris work week, I'm going to be posting updated versions of the patches here and re-requesting review, because it turns out the original patches didn't make us quite behave the same as block frames in cases where we're wrapping an unbreakable child.

(Note: The reason I'm concerned about having the right behavior around unbreakable children is that, in a flex container, *all* children are unbreakable, for the time being (until I post additional patches for fragmenting flex items).)

Specifically, there are two cases that didn't work like blocks, in the patches previously attached here:

 (a) Auto-height flex container, wrapping a fixed-height unbreakable child whose height is larger than the available height. The previous patches here (specifically "Part 4") would have made the container create a continuation for its own bottom border/padding (since the border/padding doesn't fit in the available height); but blocks don't bother with a continuation in that circumstance, so we shouldn't either.

 (b) Fixed-height flex container, wrapping a fixed-height unbreakable child, with both fixed heights being taller than the available height.  With my previous patches here (specifically Part 2 and 3), we'd clamp the flex container's height to the available height, and just let its child stick out; but that doesn't match how blocks behave. They extend to wrap their unbreakable child, even if that makes them stick out of the available height (and if the child's fixed height is taller than the block's fixed height, then the block will simply use up all of its height *trying* to wrap the child, and then create a trivial continuation for its bottom border/padding).

My updated patches address both of these situations, and I've got new reftests to cover these cases too.
(Assignee)

Comment 22

4 years ago
Comment on attachment 807513 [details] [diff] [review]
part 4: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation

>+  if (NS_FRAME_IS_COMPLETE(aStatus)) {
>+    NS_ASSERTION(aDesiredSize.height <= aReflowState.availableHeight,
>+                 "If we're complete, we shouldn't be asking for more than "
>+                 "the available height");

Also, I'm dropping this assertion, since it's invalid in light of cases (a) and (b) in my previous comment.  (In both of those cases, the flex container needs to end up being taller than the available height.)
(Assignee)

Updated

4 years ago
Attachment #807511 - Attachment description: patch 2 v2: Split vertical flex containers with a fixed main-size (height) that's larger than available height → part 2 v2: Split vertical flex containers with a fixed main-size (height) that's larger than available height
Attachment #807511 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #807621 - Attachment description: patch 5: reftests with empty flex containers → part 5: reftests with empty flex containers
(Assignee)

Comment 23

4 years ago
Created attachment 827733 [details] [diff] [review]
part 2 v3: Split vertical flex containers with a fixed main-size (height) that's larger than available height

Here's an updated version of Part 2, now allowing vertical flex containers to create a fragment taller than the available height, if they have a child that's taller than the available height.

(This addresses case "b" from comment 21, for vertical flex containers.)
Attachment #827733 - Flags: review?(matspal)
(Assignee)

Comment 24

4 years ago
Created attachment 827763 [details] [diff] [review]
part 3 v2: Refactor cross-size-computation into its own function & split horizontal flex containers whose cross size (height) is larger than available height

...and here's the updated version of part 3.

This refactors the flex container cross-size computation logic into its own function (very similar to the existing ComputeFlexContainerMainSize function), and makes that function support splitting if the container's specified height is taller than the available height. (while also accounting for case (b) from comment 21)  (This is primarily for horizontally-oriented flex containers, i.e. those whose cross axis is vertical.)
Attachment #807506 - Attachment is obsolete: true
Attachment #827763 - Flags: review?(matspal)
(Assignee)

Comment 25

4 years ago
Created attachment 827767 [details] [diff] [review]
part 4 v2: Don't ask for bottom border/padding space on a flex container if it's going to get a continuation

...and here's an updated version of part 4. The changes here are:
 - Drop the assertion (per comment 22)
 - Add an exception to make *auto-height* flex containers *not* request a continuation for their bottom border/padding, even if they're overflowing the available height. (This addresses case (a) from comment 21.)
Attachment #807513 - Attachment is obsolete: true
Attachment #827767 - Flags: review?(matspal)
(Assignee)

Comment 26

4 years ago
Created attachment 827780 [details] [diff] [review]
part 6: reftests for flex containers that have a single unbreakable child (<img>) taller than available height

Here's a patch with reftests that exercises the cases outlined in comment 21.

The 4 reftests in this patch are all identical except for their "flex-direction" values (to exercise both horizontal and vertical flex containers, with both normal and reverse polarity).

(There are also a couple tweaks in the last two (vertical) test-variants. Firstly, they have "flex: none" to prevent the img elements from taking advantage of the default "flex-shrink: 1" value and shrinking, due to the constrained height. And secondly, the final ("column-reverse") test has "justify-content: flex-end", to make its flex items overflow off of the bottom instead of the top, so that it renders like the other testcases and can share their reference.)
Attachment #827780 - Flags: review?(matspal)
(Assignee)

Comment 27

4 years ago
Created attachment 827787 [details]
reference case from part 6 (use full page zoom for easier viewing), to demonstrate the desired behavior with unbreakable children

To illustrate the situations that these new patches address (from comment 21), here's the reference case from "part 6" (using blocks; no flex containers).  See the <!--...--> comments in the file's source to explain what situation each example is testing.

(I recommend using full-page-zoom to scale this up to be reasonably sized, if you want to take a look. Its content is intentionally small, so that I can fit several examples in the same test and still fit it all in the restricted viewport that we have on mobile devices.)
(Assignee)

Updated

4 years ago
Blocks: 939897
(Assignee)

Updated

4 years ago
Blocks: 939896
Sorry for delay in reviewing this.  I wanted to take the time to
investigate how we and other UAs handle the different cases in
block layout first to form an opinion.

So, regarding comment 21; I think we layout fixed-height blocks
wrong when they overflow a fragmentainer in the case there is
a "non-splittable" child that also overflows the fragmentainer.
That is, the reference testcase you attached (the same cases
in Print Preview are also wrong).

I also think we layout auto-height blocks wrong in the same case,
in fact there should be no difference in the auto-height case
except that it will have a shrink-wrap height.

In both cases should the block create fragments that sums up
to the fixed/shrink-wrapped height.  Secondly, an image or
button or whatever that we today consider non-splittable
must split when it's taller than the fragmentainer.

The rationale for this is simple.  When the user prints a
document the expected result is to see all content that was
visible on screen, with roughly the same layout.  In particular,
an image that is taller than the page height should split and
continue on the next page.  Same as for block, flex or other
containers -- when they don't fit they split and continue on
the next page/column.

That is what IE10 and Chrome(blink) do.

(IE10 does handle overflowing images in column layout
differently than Print Preview though - it doesn't split
them and instead just let's them overflow.)
Created attachment 8335041 [details]
block test (for Print Preview in US Letter size)

Here's a block testcase that I used for testing Print Preview
with a US Letter page size (8.5 x 11 inch).  It's simple:

      <div class="container">
        <button class="child">button</button>
      </div>

The height of the container is larger than the page height,
and the child even taller, thus also overflowing the container.
Our current layout is that the child is clipped and only shows
up on the first page.  The container is also clipped and its
border (if any) shows up *at the top* of second page.
NB. It doesn't matter what fixed height you put on the container
as long as the child overflows you will get the same result.
For example, 11.5in, 14in or 15in all results in the exact same
layout.  This is clearly wrong IMO.

If you set container height to 16.5in (so that the child fits
in it but still overflows the page) then the border moves
away a bit from the top of the page.  But, it's clearly not the
specified 16.5in.  Changing the child height to 1in so that it
doesn't overflow the page finally allows the block to render
correctly as a 16.5in tall block.
I agree with you that flex containers should handle overflow in
the same way as blocks, it's just that block layout is broken in
some cases :-(

Still, I'll go ahead and review the patches as is given that this
is all new code and corresponds to the block layout.  I'd still
prefer patches that doesn't emulate the block layout bugs outlined
above.  (perhaps your previous round of patches actually did that
correctly?)
(Assignee)

Comment 31

4 years ago
(In reply to Mats Palmgren (:mats) from comment #28)
> Sorry for delay in reviewing this.  I wanted to take the time to
> investigate how we and other UAs handle the different cases in
> block layout first to form an opinion.

No worries. I appreciate your careful consideration.

(In reply to Mats Palmgren (:mats) from comment #29)
> Created attachment 8335041 [details]
> block test (for Print Preview in US Letter size)
[...]
> The height of the container is larger than the page height,
> and the child even taller, thus also overflowing the container.
> Our current layout is that the child is clipped and only shows
> up on the first page.  The container is also clipped and its
> border (if any) shows up *at the top* of second page.

Yeah... So, the current behavior makes a bit more sense in a multicol context -- then, you can see where the extra container-height ends up -- it's trying its best to wrap the (taller & overflowing) child. When it runs out of height while wrapping the child, it gives up and creates a zero-height continuation for its bottom border.  You can see this in action at the third example in attachment 827787 [details].

I agree with you that this behavior doesn't make much sense in an actual printed document, though.  If there's other content that the user cares about in the container, it's not much comfort knowing that the container has pushed it off the first page in its attempt to wrap a really-tall child.

(In reply to Mats Palmgren (:mats) from comment #30)
> Still, I'll go ahead and review the patches as is given that this
> is all new code and corresponds to the block layout.  I'd still
> prefer patches that doesn't emulate the block layout bugs outlined
> above.  (perhaps your previous round of patches actually did that
> correctly?)

Hold off for a bit, perhaps -- I'll think about this some more tomorrow, and may obsolete the current round of patches. (My previous round of patches did ignore child height when determining where to fragment the parent -- which I think is what you mean by "did that correctly" -- but IIRC those patches did have at least one other bug that I fixed in the newer versions.
(Assignee)

Updated

4 years ago
Flags: needinfo?(dholbert)
>  So, the current behavior makes a bit more sense in a multicol context --
> then, you can see where the extra container-height ends up

Unless it has overflow:hidden, then you have the identical situation to
page layout.  Granted, that is probably rare and images that are taller
than the column height are also rare, so this is a bit academic.
Images that are taller than a printed page are not that uncommon though.

Still, as a general principal, I think fragmentation in column and page
layout should work exactly the same.

The patches looks fine to me, btw, in case you want to hold off with
further changes until we fix block layout.
(Assignee)

Comment 33

4 years ago
(In reply to Mats Palmgren (:mats) from comment #32)
> The patches looks fine to me, btw, in case you want to hold off with
> further changes until we fix block layout.

Thanks! In that case, I'll take you up on that and just go with the block-consistent behavior, for the time being.

(This bug definitely isn't the last word on flexbox fragmentation, too - followup bug 939897 covers shifting/splitting the children, which will be the more interesting part. :))
Flags: needinfo?(dholbert)
(Assignee)

Updated

4 years ago
Summary: Support fragmentation (for printing, multicol) of CSS Flexbox → Support fragmentation (for printing, multicol) of flex containers (flexbox)
Comment on attachment 827733 [details] [diff] [review]
part 2 v3: Split vertical flex containers with a fixed main-size (height) that's larger than available height

Looks good, r=mats
Attachment #827733 - Flags: review?(matspal) → review+
Attachment #827763 - Flags: review?(matspal) → review+
Attachment #827767 - Flags: review?(matspal) → review+
Attachment #827780 - Flags: review?(matspal) → review+
(Assignee)

Comment 35

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ca87a2898f24
(Assignee)

Comment 36

4 years ago
That last run had timeouts in this mochitest, on B2G emulator.

Here's another try run, with requestLongerTimeout to hopefully avoid that:
 https://tbpl.mozilla.org/?tree=Try&rev=eab2e5fd4b77
(Assignee)

Comment 37

4 years ago
Sorry, disregard comment 36; that was posted on the wrong bug. (Intended for bug 940229.)
(Assignee)

Comment 38

4 years ago
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7b477575ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6c0ced7e9b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2cb514c76e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae68d174249d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e925792be85
https://hg.mozilla.org/integration/mozilla-inbound/rev/6023f2ab4af4
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/4f7b477575ba
https://hg.mozilla.org/mozilla-central/rev/3c6c0ced7e9b
https://hg.mozilla.org/mozilla-central/rev/6b2cb514c76e
https://hg.mozilla.org/mozilla-central/rev/ae68d174249d
https://hg.mozilla.org/mozilla-central/rev/0e925792be85
https://hg.mozilla.org/mozilla-central/rev/6023f2ab4af4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Updated

4 years ago
Depends on: 943509
You need to log in before you can comment on or make changes to this bug.