Filing this bug on adding pagination support to our CSS3 Flexbox implementation.

For both columns and printing?

Correct.

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.

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

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.

[sorry, attached the wrong patch. Here's the one I meant to attach.]

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

[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.]

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

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.

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.

Is this bug a prerequisite for Bug 702508? It should be added to the dependency list, then. Thanks!

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.

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?

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

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

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

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

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.

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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