Splitting out from bug 132035: This is about implementing CSS2.1's page-break-inside property. Initial patch is attachment 558222 [details] [diff] [review] by Chris Yuen (kizzx2) from bug 132035.

First set of comments is the easy ones: You have a bunch of stray changes that need to be reverted. - + aLine->SetOverflowAreas(overflowAreas); ... - PR_FALSE /*XXX isTopOfPage*/); + PR_FALSE /*eXX Page*/) ... ... + + const nsStyleDisplay * disp = rowFrame->GetStyleDisplay(); + CreateContinuingRowFrame(*aPresContext, *rowFrame, (nsIFrame**)&contRow);

@fantasai Sure, I think I have noticed them. Should I update the reverted patch now in order to continue the process, or I can do them later in batch?

Sorry, got distracted before I could finish... Yeah, feel free to do them in batch. :) Another change that seems unintended: - if (NS_FRAME_IS_NOT_COMPLETE(reflowStatus)) - floatMargin.bottom = 0; Substantive comments: // Continue the block frame now if it didn't completely fit in // the available space. if (!NS_FRAME_IS_FULLY_COMPLETE(frameReflowStatus)) { + + // WSG + // + // If child needs to page-break-inside avoid but didn't fit, + // move it to a new line. This will effectively move it to + // the next page + // + // XXX The pagination check is probably not needed + if((display->mBreakInside == NS_STYLE_PAGE_BREAK_AVOID || + frameReflowStatus & NS_FRAME_REQUEST_NEXT_PAGE) && + startingY > 0 && + aState.mPresContext->IsPaginated()) { + aLine->SetChildCount(aLine->GetChildCount() - 1); + + nsLineBox* line = aState.NewLineBox(frame, 1, PR_TRUE); + NS_ENSURE_TRUE(line, NS_ERROR_OUT_OF_MEMORY); + mLines.after_insert(aLine, line); + + PushLines(aState, aLine.prev()); + + NS_FRAME_SET_INCOMPLETE(aState.mReflowStatus); + *aKeepReflowGoing = PR_FALSE; + + break; + } So, we definitely shouldn't be checking for IsPaginated since we still want to honor page-break:avoid inside a multi-column element. Wrt creating a new line box... I don't understand why you're doing that, it seems wrong. Probably we should be hooking into the code that already handles blocks that are too large to fit. I suggest doing that here: // None of the block fits. Determine the correct reflow status. if (aLine == mLines.front() && !GetPrevInFlow()) { // If it's our very first line then we need to be pushed to // our parents next-in-flow. Therefore, return break-before // status for our reflow status. aState.mReflowStatus = NS_INLINE_LINE_BREAK_BEFORE(); } Where it's checking aLine == mLines.front(), we could check alternatively for page-break-inside: avoid and mIsTopOfPage on ourself. Our parent should then handle pushing us to the next page. That should simplify your code a lot: no need for a new reflow status, etc. To handle table cells and rows, you'll want the check to walk up to the parent frame. If it's not a table cell, then nevermind; but if it is, check for page-break-inside: avoid on it and its parent row, and treat it as if it were set on ourself. Our code seems to be able to split between rows correctly if it detects there's no breakpoint within the cell block. An interesting question would be rowspans; perhaps if the cell is a rowspan, we don't check the row's page-break-inside property. That way if there's a break between the two rows, the rowspanning cell can split there. (We're not good at splitting between rows when there's a rowspan, but that's a separate issue.) For other table elements: the equivalent code in nsTableRowGroupFrame is here: // We can't push children, so let our parent reflow us again with more space aDesiredSize.height = rowRect.YMost(); aStatus = NS_FRAME_COMPLETE; break; Looks like it relies on nsTableFrame detecting that it's too big... so if we need to pass the "we don't fit" message up, we'll need to either teach nsTableFrame about BREAK_BEFORE statuses, or pick an aDesiredSize.height that is greater than the available height. Let me know if that makes sense, or if there's something here I didn't consider. :) Also, you forgot to attach your style system patch -- you should attach that and flag dbaron for review. (I'm not really familiar with the style system.)

So let's do this one by one. I think we can get the nsBlockFrame right first, because table is a totally different beast. > Wrt creating a new line box... I don't understand why you're doing that, it seems wrong. Probably we should be hooking into the code that already handles blocks that are too large to fit. I suggest doing that here: > ... > Where it's checking aLine == mLines.front(), we could check alternatively for page-break-inside: avoid and mIsTopOfPage on ourself. Our parent should then handle pushing us to the next page. That should simplify your code a lot: no need for a new reflow status, etc. 1. I have tried numerious combinations of stuffs inside nsBlockFrame::ReflowBlockFrame and the "create new line" is the only one that came out working. In fact when I first started I was doing it at the place you suggested (from the obvious comments), here is one of my failed attempts: // Attempt the same treatment as "none of our block fits" if ((display->mBreakInside == NS_STYLE_PAGE_BREAK_AVOID || frameReflowStatus & NS_FRAME_REQUEST_NEXT_PAGE) && startingY > 0) { // The next line does not make any difference PushLines(aState, aLine.prev()); aState.mReflowStatus = NS_INLINE_LINE_BREAK_BEFORE(); } // None of the block fits. Determine the correct reflow status. else if (aLine == mLines.front() && !GetPrevInFlow()) { // If it's our very first line then we need to be pushed to // our parents next-in-flow. Therefore, return break-before // status for our reflow status. aState.mReflowStatus = NS_INLINE_LINE_BREAK_BEFORE(); } else { // Push the line that didn't fit and any lines that follow it // to our next-in-flow. PushLines(aState, aLine.prev()); NS_FRAME_SET_INCOMPLETE(aState.mReflowStatus); } The above code does not work. The result is that the "splitting block" will not create a continuation (see attached screenshot1). After numerous attempts I came up with the "new line" approach. I suspect the reason it works is because the same element gets reflowed twice at page boundaries. Any ideas?

The key difference between your code and my suggestion is instead of the parent doing the check for NS_STYLE_PAGE_BREAK_AVOID, have the frame do its own check for NS_STYLE_PAGE_BREAK_AVOID. Like this: // None of the block fits. Determine the correct reflow status. else if (!GetPrevInFlow() && (aLine == mLines.front() || (!aState.mFlags.mIsTopOfPage && GetStyleDisplay()->mBreakInside == NS_STYLE_PAGE_BREAK_AVOID) { // If it's our very first line *or* we're not at the top of the page // and we have page-break-inside: avoid, we then we need to be pushed to // our parents next-in-flow. Therefore, return break-before // status for our reflow status. aState.mReflowStatus = NS_INLINE_LINE_BREAK_BEFORE(); } The parent should then treat this exactly like a block that doesn't fit, and you don't need a special reflow status for page-break-inside: avoid cases.

Maybe I am missing something obvious, I just did a simple debugger run with `divs.html` from my testsuite. Using vanilla trunk, doing Print Preview does not reach the "none of the block fits" area at all, so for the case of "divs.html" adding code there changes nothing. Maybe you meant using the "none of the block fits" code above? I did similar experiments but to no avail. I think the crux of the problem is that it's nsHTMLContainerFrame that will create continuation for the contained frame. In order to prevent the continuation from being created, the straight-forward approach (which I did) is to insert some conditions right above the continuation creation code and block it. I also haven't had much success doing `GetStyleDisplay()` instead of `frame->GetStyleDisplay()` because it's usually nsHTMLContainerFrame that detects the breakage, and `GetStyleDisplay()` on nsHTMLContainerFrame is always default. I think it gets quite complicated talking about about, so: Here is an trunk-ready patch, it adds the page-break-inside avoid to CSS Display so experiments can be easily made. (I think the extra reflow status could be eliminated if we can revamp the table part (which is rather ugly). As you can see from the patch, the extra status is only propagated from tables.)

No, I meant exactly what I wrote. But I forgot that that code doesn't execute for lines of inline content, only for blocks. :) It works on this testcase, for example. I'm still trying to understand the lines-of-inlines code; it's taking me awhile, sorry... :)

Alright, found the part that handles inlines, it's the part that starts with // See if the line fit. If it doesn't we need to push it. Our first // line will always fit. should look like this: if (mLines.front() != aLine && newY > aState.mBottomEdge && aState.mBottomEdge != NS_UNCONSTRAINEDSIZE) { NS_ASSERTION((aState.mCurrentLine == aLine), "oops"); if (!GetPrevInFlow() && GetStyleDisplay()->mBreakInside == NS_STYLE_PAGE_BREAK_AVOID) { // If all our content doesn't fit, start on the next page aState.mReflowStatus = NS_INLINE_LINE_BREAK_BEFORE(); } else { // Push this line and all of its children and anything else that // follows to our next-in-flow PushTruncatedLine(aState, aLine, *aKeepReflowGoing); } return PR_TRUE; } This bit and the bit in comment 6 should take care of the layout parts for block frames. Do you understand how this is different from your approach? In your approach the responsibility for handling page-break-inside: avoid is in the parent of the affected frame. In this approach, the frame itself does this check and communicates to the parent that it's unbreakable.

Comment on attachment 559077 [details] [diff] [review] Add page-break-inside to CSS system It looks like most of the important pieces are here. However, the most important thing you're missing is addition of tests to layout/style/test/property_database.js : all you need to do there is remove the relevant 'backend_only: true,' line. Then you should run the mochitests in layout/style/ and fix the ones that fail. At the very least, you're going to need to implement some code in nsComputedDOMStyle.cpp. >diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp >+ const nsCSSValue * breakInsideValue = aRuleData->ValueForPageBreakInside(); >+ if(eCSSUnit_Enumerated == breakInsideValue->GetUnit()) >+ display->mBreakInside = breakInsideValue->GetIntValue(); This is wrong; it should look a lot more like the code for other properties (i.e., handle inherit and initial, and likely do so by using SetDiscrete). >diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp > if (mBreakType != aOther.mBreakType > || mBreakBefore != aOther.mBreakBefore > || mBreakAfter != aOther.mBreakAfter >+ || mBreakInside != aOther.mBreakInside Fix indent to match surrounding code. >diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h >--- a/layout/style/nsStyleStruct.h >+++ b/layout/style/nsStyleStruct.h >@@ -1516,16 +1516,17 @@ struct nsStyleDisplay { > PRUint8 mBreakType; // [reset] see nsStyleConsts.h NS_STYLE_CLEAR_* > PRPackedBool mBreakBefore; // [reset] > PRPackedBool mBreakAfter; // [reset] > PRUint8 mOverflowX; // [reset] see nsStyleConsts.h > PRUint8 mOverflowY; // [reset] see nsStyleConsts.h > PRUint8 mResize; // [reset] see nsStyleConsts.h > PRUint8 mClipFlags; // [reset] see nsStyleConsts.h > PRUint8 mOrient; // [reset] see nsStyleConsts.h >+ PRUint8 mBreakInside; add a comment to match the surrounding code, and also point to which constants in nsStyleConsts.h are relevant, since it's not obvious since there aren't any BREAK_INSIDE constants. You're close to done here, I think (though I didn't look particularly carefully for other missing pieces; if the tests fail after doing the things I mentioned above, try looking at another recent patch to add a CSS property via the hg history of one of the files you're touching). Also, when you upload a new patch, it's best if that patch contains a From: header and commit message. See: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F if you're using mq.

> This bit and the bit in comment 6 should take care of the layout parts for > block frames... Yes, I tried it and it worked, though it still seem to be not doing anything in `columns.html` from my test suite. I suspect I just need to do another NS_INLINE_LINE_BREAK_BEFORE() somewhere. I will do more experiments when I can get to it :P Just wondering, though, what's the thing that's fundamentally wrong about the "creating new line approach"? You know I also felt it was wrong, but it worked surprisingly well for all the cases (including columns, except tables) on the first try. It has nothing to do with the extra status (it's only needed for tables and admittedly the table part of my patch was rushed).

Well, we have an invariant that line boxes aren't empty. It looks like you're creating an empty line box and then shifting the frame from the old line box to a new one. Which doesn't really make any sense... There are basically two ways to handle responsibility for page-break-inside: avoid. a) The parent is responsible for detecting it and, when necessary, denying a request to split and instead pushing the element to the next page. This is your approach. b) The element itself is responsible for detecting it and, when necessary, returning a "can't fit, push to next page" request to the parent. This is my approach. I'm not actually sure which is better. Both can work. But I'm pretty sure creating empty line boxes is not a good idea. :) The number of line boxes shouldn't change because we have a page-break-inside: avoid.

Any update on this ? It's a pity Firefox is the only one currently not supporting this although a working patch seems to exist. :-(

The interaction between break-inside and column balancing for height still leads to breaking in some cases where it shouldn't, but I'll file a separate bug about that. Seems to work fine other than that. https://tbpl.mozilla.org/?tree=Try&rev=af08d926cd94 Try builds for testing: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-af08d926cd94/

Comment on attachment 662579 [details] [diff] [review] part 2, layout + tests Your tests should have <title>s and <link>s to the relevant sections of the CSS2.1 and CSS3 Fragmentation specs and go into w3c-css. Please add a test to make sure inline-tables behave sanely and don't crash. If we can't split them, we need to make sure they truncate and don't leave the frame tree in some weird indeterminate state. I haven't reviewed the individual tests yet. The patch overall seems alright, but I think I need to look it over again before I really understand everything that's going on. >- } >- else { // frame is complete but its overflow is not complete >+ } else { >+ // frame is complete but its overflow is not complete >- } >- else { // frame is fully complete >+ } else { >+ // frame is fully complete Stray changes here. > void > nsBlockFrame::PushTruncatedLine(nsBlockReflowState& aState, > line_iterator aLine, >- bool& aKeepReflowGoing) ... >+ /** >+ * Push aLine (and any after it), since it cannot be placed on this >+ * page/column. Set aKeepReflowGoing to false and set >+ * flag aState.mReflowStatus as incomplete. >+ */ If we only need PushTruncatedLine and not PushLines anywhere, maybe we should follow through and merge the two? (That would be a separate patch, though.) >+ if (aPresContext->IsPaginated() && !NS_FRAME_IS_FULLY_COMPLETE(aStatus) && >+ IsBreakInsideAvoid(aReflowState)) { >+ aStatus = NS_INLINE_LINE_BREAK_BEFORE(); >+ aDesiredSize.height = aReflowState.availableHeight + 1; This line deserves a comment. Why are we setting aDesiredSize.height to aReflowState.availableHeight + 1? >+ if (!aRowForcedPageBreak && !NS_FRAME_IS_FULLY_COMPLETE(aStatus) && >+ IsBreakInsideAvoid(aReflowState)) { >+ aStatus = NS_INLINE_LINE_BREAK_BEFORE(); >+ aDesiredSize.height = aReflowState.availableHeight + 1; Same here > nsTableFrame::InvalidateFrame(rowFrame, oldRowRect, >- oldRowVisualOverflow, >- false); >+ oldRowVisualOverflow, false); Stray change. > if (!rowIsOnPage) { > NS_ASSERTION(!contRow, "We should not have created a continuation if none of this row fits"); >+ if (!aRowForcedPageBreak && IsBreakInsideAvoid(aReflowState)) { >+ aStatus = NS_INLINE_LINE_BREAK_BEFORE(); >+ aDesiredSize.height = aReflowState.availableHeight + 1; And here.

In the block-page-break-inside-avoid series, you're missing tests for: * Putting everything in an avoid, and wrapping 2 and 3 into a nested avoid. * Putting everything in an avoid, and adding a forced break between 1 and 2. * Nesting an explicitly 'auto' <div> inside an 'avoid' <div>, to make sure it has no effect. You're also missing tests for floats with 'page-break-inside: avoid'. I don't understand the following tests: block-page-break-inside-avoid-1.html rowgroup-page-break-inside-avoid-6.html The test will pass if either page-break-inside or page-break-before takes effect. Why are you putting them together--what ways are you expecting this combination to fail that wouldn't be caught by the individual tests? column-balancing-break-inside-avoid-1.html What is the purpose of .short p { page-break-inside:auto; } ? Whether this <p> is treated as 'auto' or as 'avoid' makes no difference in the result of this test, because the <p> is at the top of the column. Also what's the point of <div class="colset"> one two three four five </div> ? Also, I'd suggest having <div class="colset"> <p>one two</p> <p>three four five</p> </div> to make sure "three" doesn't get pushed a column over because its <p> doesn't fit. table-page-break-inside-avoid-2.html The page-break-inside: avoid has no effect under the circumstances. What's the intention of this test? Also, why is it using a completely different styling than the other tests? (It makes the series harder to understand if there are arbitrary differences in styling.) Wrt row-page-break-inside-avoid series: Do we have a control test that shows us breaking within a row? Wrt rowgroup-page-break-inside-avoid series: I would suggest either zeroing out the border-spacing so the reference matches the block tests, or going full out and testing the handling of margins/borders/padding on the table. Also, there appears to be no difference between #3 and #4. Did you have something in mind you wanted to test, but forgot? rowgroup-page-break-inside-avoid-7.html There's a stray .bb { page-break-before:always; } here. In general, since the presence of green in your tests isn't indicating success, (in contrast with red for failure), please use a different color, like blue or orange. I haven't cross-checked... did you convert all of Chris's tests to equivalent reftests?

(As a general comment on NS_INLINE_LINE_BREAK_BEFORE(), I think once we handle breaks between elements through reflow statuses, we'll probably want some way for nsReflowStatus to distinguish between passing up forced breaks vs. induced breaks. But that requires some refactoring of nsReflowStatus first. Just something to think about going forward.)

(In reply to fantasai from comment #16) > Your tests should have <title>s and <link>s to the relevant sections of the > CSS2.1 and CSS3 Fragmentation specs and go into w3c-css. These tests evolved while writing the code; every time I made a mistake that lead to infinite reflows or whatever I added a test for that. I also tried to cover each branch in the code with a test. The purpose of these tests is to catch regressions in Gecko and I think it would be a mistake to go back and change them so that they can be spec compliance tests for W3C. For testing spec compliance, I used the tests at: http://test.csswg.org/suites/css2.1/20110323/xhtml1/chapter-13.xht (page-break-inside-000 .. page-break-inside-006) They pass. I'll be happy to write reference files for those tests if you like. (you need to tell me where to put them and what file names to use - I read layout/reftests/w3c-css/README but I can't figure it out) > Please add a test to make sure inline-tables behave sanely and don't crash. Added inline-table tests: table-page-break-inside-avoid-3.html table-page-break-inside-avoid-4.html > Stray changes here. Removed those. > >+ aStatus = NS_INLINE_LINE_BREAK_BEFORE(); > >+ aDesiredSize.height = aReflowState.availableHeight + 1; > > This line deserves a comment. Why are we setting aDesiredSize.height to > aReflowState.availableHeight + 1? I removed those and added NS_INLINE_IS_BREAK_BEFORE checks in the corresponding container class instead.

(In reply to fantasai from comment #17) > In the block-page-break-inside-avoid series, you're missing tests for: > * Putting everything in an avoid, and wrapping 2 and 3 into a nested avoid. Added new tests: block-page-break-inside-avoid-7.html block-page-break-inside-avoid-8.html > * Putting everything in an avoid, and adding a forced break between 1 and > 2. block-page-break-inside-avoid-9.html > * Nesting an explicitly 'auto' <div> inside an 'avoid' <div>, to make sure > it has no effect. block-page-break-inside-avoid-10.html > You're also missing tests for floats with 'page-break-inside: avoid'. float-page-break-inside-avoid-1.html float-page-break-inside-avoid-2.html float-page-break-inside-avoid-3.html float-page-break-inside-avoid-4.html float-page-break-inside-avoid-5.html float-page-break-inside-avoid-6.html > I don't understand the following tests: > > block-page-break-inside-avoid-1.html > rowgroup-page-break-inside-avoid-6.html > The test will pass if either page-break-inside or page-break-before > takes effect. The intention is to test page-break-inside:avoid on a container at the top of the page. Yes, it should render exactly as if page-break-inside:avoid were not specified, that's what the test verifies. > column-balancing-break-inside-avoid-1.html > What is the purpose of > .short p { page-break-inside:auto; } > ? Whether this <p> is treated as 'auto' or as 'avoid' makes no difference > in the result of this test, because the <p> is at the top of the column. Yes, this test verifies that it has no effect. > Also what's the point of > <div class="colset"> > one two three four five > </div> Removed. > Also, I'd suggest having > <div class="colset"> > <p>one two</p> > <p>three four five</p> > </div> > to make sure "three" doesn't get pushed a column over because its <p> > doesn't fit. Added. > table-page-break-inside-avoid-2.html > The page-break-inside: avoid has no effect under the circumstances. > What's the intention of this test? To verify page-break-inside:avoid has no effect at the top of the page. > Also, why is it using a completely different styling than the other > tests? OK, I made it similar to the others in style. > (It makes the series harder to understand if there are arbitrary > differences in styling.) (OTOH, variation in regression tests is a good thing since their purpose is to catch *unexpected* errors.) > Wrt row-page-break-inside-avoid series: > Do we have a control test that shows us breaking within a row? Can you be more specific please? (both tests you're referring to would split the row without the page-break-inside:avoid) > Wrt rowgroup-page-break-inside-avoid series: > I would suggest either zeroing out the border-spacing so the reference > matches the block tests, ... I don't think it matters, leaving them as is. > Also, there appears to be no difference between #3 and #4. Did you have > something in mind you wanted to test, but forgot? Oops. Fixed. > rowgroup-page-break-inside-avoid-7.html > There's a stray .bb { page-break-before:always; } here. Removed. > In general, since the presence of green in your tests isn't indicating > success, ... Changed to blue. > I haven't cross-checked... did you convert all of Chris's tests to > equivalent reftests? I've checked that the reftests covers all his cases (I've added a couple that were missing). I think a few of his tests are invalid - the ones testing breaks in transformed boxes.

Added code to deal with floats (which were handled poorly in the first version). Improved the table code a bit. Added lots of new tests as requested.

Green on Try: https://tbpl.mozilla.org/?tree=Try&rev=ce8061d12a55 Builds for testing: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-ce8061d12a55/

(In reply to Mats Palmgren [:mats] from comment #19) > The purpose of these tests is to catch regressions in > Gecko and I think it would be a mistake to go back and change > them so that they can be spec compliance tests for W3C. Why do you think that? They're very good compliance tests afaict. > I'll be happy to write reference files for those tests if you like. > (you need to tell me where to put them and what file names to use - They'd have to be checked into the W3C repo, since that's where changes are made to those tests, so, maybe attach a zipfile and I'll check them in? > I read layout/reftests/w3c-css/README but I can't figure it out) Ok, let's talk off-list about that. Sounds like I need to fix the documentation somehow. :) > table-page-break-inside-avoid-3.html I think you need to get rid of +<div style= "page-break-after: always"></div> for the page-break-inside: avoid to be tested. > I removed those and added NS_INLINE_IS_BREAK_BEFORE checks in the > corresponding container class instead. - } - // If we have a next-in-flow, then we're not complete - // XXXldb This used to be done only for the incremental reflow codepath. - if (GetNextInFlow()) { - aStatus = NS_FRAME_NOT_COMPLETE; - } + // If we have a next-in-flow, then we're not complete + // XXXldb This used to be done only for the incremental reflow codepath. + if (GetNextInFlow()) { + NS_FRAME_SET_INCOMPLETE(aStatus); + } + } Afaict, ReflowChildren doesn't check for a next-in-flow and therefore won't send a correct status code if there is one. So this change seems wrong... why do you think it's correct? > > * Putting everything in an avoid, and wrapping 2 and 3 into a nested avoid. > Added new tests: > block-page-break-inside-avoid-7.html > block-page-break-inside-avoid-8.html 8 seems like the more rigorous test, maybe make 7 the same as 8 except without the 'overflow: hidden'? Or at least add variants without 'overflow: hidden". > > Do we have a control test that shows us breaking within a row? > Can you be more specific please? Yes, I'm wondering if we have a test somewhere (without page-break-inside: avoid) that tests that we can split a row. :) We have a bunch of tests here proving cases where we shouldn't split a row, but I don't recall us having one that tests where we should.

float-page-break-inside-avoid-5.html This test seems to depend on the default <html> margins. Can you write it so that it doesn't, i.e. make the second float sized so that it won't fit even if those margins are zeroed out? There are no tests that show what happens to content around the float that is not floated when the float is pushed to the next page. Please add some.

> - // If we have a next-in-flow, then we're not complete > - // XXXldb This used to be done only for the incremental reflow codepath. > - if (GetNextInFlow()) { > - aStatus = NS_FRAME_NOT_COMPLETE; > - } > Afaict, ReflowChildren doesn't check for a next-in-flow and therefore won't > send a correct status code if there is one. So this change seems wrong... > why do you think it's correct? The block-page-break-inside-avoid-11.html test fails with the above code intact. Here's what happens: The first frame dump is at the very first CreateContinuingFrame call, this is when we reflow the first page and see that the "2" block doesn't fit, so we create a continuation for it, push that, create continuations for the cell and row (all 'incomplete'). The next frame dump is when we create the row-group continuation (that causes the bug later), it's the table frame (naturally) that does this because the row-group reflow resulted in 'incomplete' status (since its children are). This is all correct. The next frame dump is when we reflow the first row-group again (now on the *second* page) and we find that it fits! (status is 'complete'). But it still has that lingering next-in-flow from the reflow on the first page so we change it to 'incomplete' (this is the bug). Because of the 'incomplete' status on the second page, we again create a bunch of continuations - the table, the div block, etc all the way out to creating a new page frame. The last frame dump is when we reflow the empty row-group on the *third* page. ==== The fix is to remove the above code so that when the row-group reports a 'complete' status the table frame (nsContainerFrame::ReflowChild really) deletes the row-group's next-in-flow, and when it reports 'incomplete' status the table frame create a continuation for it. AFAIU, this is how all reflow works and I don't see any reason why row-group needs to have this special code. We should fix ReflowChildren if it fails to set completion status correctly, but I have no evidence of that.

(In reply to fantasai from comment #23) > I think you need to get rid of > +<div style= "page-break-after: always"></div> > for the page-break-inside: avoid to be tested. The purpose of this test is to verify that 'page-break-after:always' on one sibling and 'page-break-inside:avoid' on the next works as expected, i.e. doesn't result in two page breaks, crashes, asserts etc. > > block-page-break-inside-avoid-8.html > > 8 seems like the more rigorous test, maybe make 7 the same as 8 except > without the 'overflow: hidden'? Or at least add variants without 'overflow: > hidden". Added same tests but without 'overflow:hidden': block-page-break-inside-avoid-12.html block-page-break-inside-avoid-13.html > Yes, I'm wondering if we have a test somewhere (without page-break-inside: > avoid) that tests that we can split a row. :) We have a bunch of tests here > proving cases where we shouldn't split a row, but I don't recall us having > one that tests where we should. I couldn't find such a test so I added: layout/reftests/table-overflow/table-row-pagination.html (In reply to fantasai from comment #24) > float-page-break-inside-avoid-5.html > > This test seems to depend on the default <html> margins. > Can you write it so that it doesn't, i.e. make the second > float sized so that it won't fit even if those margins > are zeroed out? Fixed. > There are no tests that show what happens to content around the float > that is not floated when the float is pushed to the next page. > Please add some. Added tests: float-page-break-inside-avoid-7.html float-page-break-inside-avoid-8.html float-page-break-inside-avoid-9.html I've moved all page-break-inside tests (except one that use reftest-wait) to layout/reftests/w3c-css/submitted/ and added a prefix to make the file names globally unique per the README.

Comment on attachment 662576 [details] [diff] [review] part 1, style system r=dbaron

Comment on attachment 662576 [details] [diff] [review] part 1, style system Actually, two comments: >Bug 685012 - Implement page-break-inside:avoid. part 1/2, r=dbaron Commit message should mention "in the style system" rather than "part 1/2" >diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h >+ uint8_t mBreakInside; // [reset] NS_STYLE_PAGE_BREAK_AUTO/ALWAYS/AVOID remove the "ALWAYS/" since that's not part of this property

(In reply to Mats Palmgren [:mats] from comment #25) > > The fix is to remove the above code so that when the row-group reports a > 'complete' status the table frame (nsContainerFrame::ReflowChild really) > deletes the row-group's next-in-flow, and when it reports 'incomplete' > status the table frame create a continuation for it. AFAIU, this is how > all reflow works and I don't see any reason why row-group needs to have > this special code. We should fix ReflowChildren if it fails to set > completion status correctly, but I have no evidence of that. Ah, ok. That makes sense. The case where I think ReflowChildren is not setting the status correctly is if a second reflow pass is triggered with an availableHeight greater than or equal to the last time. There's no pull-up code in ReflowChildren that I can see. If we run out of rows in this continuation, then we think we're complete, and the next-in-flow will get destroyed with all its children. If I'm correct, this is likely to put the frame tree in a very weird state. I think it's less weird than if we fix that testcase the way you're proposing; at least in that case, the frame tree is not inconsistent, it just has too many continuations. Does that make sense? Or am I misinterpreting something?

> There's no pull-up code in ReflowChildren that I can see. I agree, it should do that - filed bug 804888. We have no test that triggers this though, so I kind of doubt it can occur currently - given that we don't support fragmenting tables in multi-column layout. Anyway, I'll do a workaround for block-page-break-inside-avoid-11.html here and punt to bug 804888. (I would appreciate a test for it if you can think of one.)

Nits fixed.

I worked around the troublesome code like so: - if (GetNextInFlow()) { - aStatus = NS_FRAME_NOT_COMPLETE; + if (GetNextInFlow() && GetNextInFlow()->GetFirstPrincipalChild()) { + NS_FRAME_SET_INCOMPLETE(aStatus); In other words, if this row-group frame has an empty next-in-flow and is COMPLETE, we leave it like that and let the table destroy the nif. NS_FRAME_SET_INCOMPLETE preserves break status, unlike assignment. https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=813c0e160ae9

Comment on attachment 674525 [details] [diff] [review] part 2, layout + tests, v4 Can't think of a way to trigger it either, but your change looks good now. I'm not sure I'll have time to finish a full review of the float-related changes this week; also I'm not familiar with it, so flagging roc for sr. I'd like him to look over the patch in any case, since I'm not as experienced and it is a complicated set of changes.

Comment on attachment 674525 [details] [diff] [review] part 2, layout + tests, v4 Review of attachment 674525 [details] [diff] [review]: ----------------------------------------------------------------- This looks good! ::: layout/generic/nsFrame.h @@ +410,5 @@ > > + /** > + * @return true if we should avoid a page/column break in this frame. > + */ > + bool IsBreakInsideAvoid(const nsHTMLReflowState& aReflowState) const { ShouldAvoidBreakInside sounds less clumsy.

fantasai, do you have further comments on the patch?

(renamed IsBreakInsideAvoid to ShouldAvoidBreakInside)

No, as long as roc's good with the float-related changes, I think it's fine to check in. I'll try to take another review pass through the tests tomorrow, but that shouldn't hold you up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf63c3f34e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3dd343ee5e

https://hg.mozilla.org/mozilla-central/rev/dbf63c3f34e9 https://hg.mozilla.org/mozilla-central/rev/ea3dd343ee5e

Documentation updated: https://developer.mozilla.org/en-US/docs/CSS/page-break-inside and https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers

These 3 CSS 2.1 test suite tests which previous Firefox versions failed now pass in Firefox 19: http://test.csswg.org/suites/css2.1/20110323/html4/page-break-inside-001.htm http://test.csswg.org/suites/css2.1/20110323/html4/page-break-inside-004.htm http://test.csswg.org/suites/css2.1/20110323/html4/page-break-inside-006.htm Also Firefox 19 passes this test: Testcase on page-break-inside: avoid; http://www.gtalbot.org/BugzillaSection/Bug132035_Page_Break_Inside_Avoid.html Gérard

This bug is not resolved, try this test code and you will see page breaks inside divs : ************************************* [CODE] <html> <head> <style> div { height: 600px; background-color: red; border: solid 2px; margin-bottom: 10px; page-break-inside: avoid; } </style> </head> <body> <div>Hello</div> <div>Hello</div> <div>Hello</div> <div>Hello</div> <div>Hello</div> <div>Hello</div> </body> </html> [CODE]

(In reply to hus.buy from comment #42) > try this test code and you will see page breaks > inside divs hus.buy, I think your feedback is correct. I created a slightly modified test from your code. I removed background color as this is definitely not welcomed, not needed if the document gets printed. I may remove (or reintroduce) border declaration for each div: the border presence helps view each div layout. http://www.gtalbot.org/BugzillaSection/Bug685012-page-break-inside-avoid.html The test assumes paper size in paper setup is (preferably) US Letter (216mm wide by 279mm tall; 8½inches by 11inches) or A4 (210mm wide by 297mm tall). Page orientation should be Portrait. Firefox 21 fails this test. Chrome 27.0.1453.110 passes this test. I use Linux, kernel version 3.8.0-25-generic, KDE 4.10.4, i686 (32bits). There are other related bug reports with regards to page-break-inside: bug 775622 . Gérard

That issue should go in a separate bug report so we can track it appropriately.

Bug 883676 Implement page-break-inside:avoid for block has been created.

Thanks a lot Gérard. I have just seen that this bug is assigned to you for chrome too. I noticed there is a bug for Chrome too when div elements are in a "table". Try the code below, and you will see : - at pages 3,9 and 15: some unexpected space between first and second div; - at pages 4,10 and 16: some unexpected space between second and third div; - at pages 5,11 and 17: only 2 divs are visible instead of 3; There is a cyclic behavour every 6 pages. Thank you. ********************************************************** <html> <head> <style> table { width: 100%; } div { height: 270px; border: solid 2px; margin-bottom: 10px; page-break-inside: avoid; } </style> </head> <body> <table><tbody> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> <tr><td><div>Hello</div></td></tr> </tbody></table> </body> </html>

(In reply to hus.buy from comment #46) > Try the code below, and you will see : > - at pages 3,9 and 15: some unexpected space between first and second div; > - at pages 4,10 and 16: some unexpected space between second and third > div; > - at pages 5,11 and 17: only 2 divs are visible instead of 3; > > There is a cyclic behavour every 6 pages. hus.buy, here is a slightly modified version of your code-example: http://www.gtalbot.org/BrowserBugsSection/MozillaBugs/unexpected-space-page-break-inside.html Now, I've tried *_several modified versions_* of your code-example and I just never was able to reproduce in Firefox 29 any of the issues you mentioned. But I do see that Chrome 35 has issues (produces 19 pages: 5 pages with only 2 rectangular blocks, 1 page with only 1 rectangular block; 2 pages with inconsistent vertical gap) with that test. So, Firefox *_passes_* this test. Chrome fails this test. For this test, I have been using Firefox 29 (buildId == 20140428193813) and Chrome 35.0.1916.114 under Linux 3.13.0-29-generic x86_64 Qt 4.8.6 KDE 4.13.1 Kubuntu 14.04 LTS (codename: trusty) and my print setting is paper size in paper setup is (preferably) US Letter (216mm wide by 279mm tall; 8½inches by 11inches) or A4 (210mm wide by 297mm tall), page orientation is Portrait, page scale is 100%. Notes ----- - I replaced "div" with "rectangular block" because if we, one day, submit this test to CSS 2.1 test suite, we want ordinary people (not familiar with vocabulary/terminology of webpage development) to be able to take that test and be able to figure out easily if its a PASS or FAIL. - This kind of test is not ideal and not within normal circumstances: having to print 17 pages just for a paged test is not something we want all paged tests to be. At most, at maximum: 10 pages. Within such 10 pages limit, Chrome 35 still has a bug: its page 6 (in print preview) has only 2 divs. - Can anyone please tell me if Internet Explorer 11 (update version 11.0.7) passes this test? Gérard

Re: IE 11 version 11.0.15. If I am understanding the test correctly, it IE does pass

Looks like many of the reftests that we added here... (1) ...live in a w3c-css/submitted/ subdirectory (2) ...use "reftest-print" But is "reftest-print" something that the w3c reftest harness actually understands? (I don't think so?) (And if the w3c harness doesn't understand "reftest-print", should these tests move somewhere else perhaps, like e.g. layout/reftests/css-break?) example: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/css21/pagination/moz-css21-block-page-break-inside-avoid-1.html?force=1

I originally had them in layout/reftests/pagination/ (see attachment 664064 [details] [diff] [review]) but fantasai asked me to move them under w3c-css. (comment 19 to comment 26 contains some discussion of that). Feel free to move them if they (w3c) don't want them there. IIRC, we specifically added @page rules to all these tests so that they should render correctly (in Print Preview) in other browsers, assuming they support @page sizes. fantasai or dbaron might know more about the W3C reftest harness.

Ah, gotcha -- so the "@page" rule is meant to mimic "reftest-print"? That's handy. (My motivation in asking is that kzentner has a "contain:style" reftest which needs "reftest-print", and his patch is currently placing it in a w3c-css subdirectory, and I was pretty sure it needed to move elsewhere until I discovered your reftest-print tests. I'll recommend that he use @page rules like the ones you used, then. Thanks!)

Yeah, the 'size' in the @page rules in these tests match what we use internally for "reftest-print". (Our internal value was hard coded back then, but perhaps we honor a specified @page size in "reftest-print" tests these days?) So viewing them manually in Print Preview in other browsers should produce a similar layout. Actually, I just tried that in Chrome's Print Preview and it seems it honor the 'size' but not the 'margin', so you have to adjust the margin manually to 0.5 inch on all sides (fortunately they have simple UI for that). After that the tests seem to work fine. I would recommend he follows the same path, unless someone from W3C says otherwise.

(In reply to Daniel Holbert [:dholbert] from comment #49) > But is "reftest-print" something that the w3c reftest harness actually > understands? (I don't think so?) No, the w3c reftest harness will not "understand" "reftest-print". The W3C reftest harness "understands" the paged flag: " paged Only valid for paged media " http://testthewebforward.org/docs/css-metadata.html#requirement-flags If you write in the <head> section of a test: <meta content="paged" name="flags"> then this message " This test is only valid for paged media such as print. " is displayed above the test itself. Eg http://test.csswg.org/harness/test/css21_dev/single/abspos-paged-001/ Best is to include in the test itself a short message specifying, emphasizing that the test is for printing or print preview only: "PREREQUISITE: Switch to print preview or a paged media view of the page." is the most frequent message used. Eg http://test.csswg.org/harness/test/css21_dev/single/at-import-008/ http://test.csswg.org/harness/test/css-writing-modes-3_dev/single/first-page-vrl-002/

(In reply to Daniel Holbert [:dholbert] from comment #49) [snipped] > (And if the w3c harness doesn't understand "reftest-print", should these > tests move somewhere else perhaps, like e.g. layout/reftests/css-break?) > > example: > http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/ > submitted/css21/pagination/moz-css21-block-page-break-inside-avoid-1. > html?force=1 It appears that David Baron submitted the test moz-css21-block-page-break-inside-avoid-1 and it has the <meta content="paged" name="flags"> in the <head> section: [Shepherd] http://test.csswg.org/shepherd/testcase/moz-css21-block-page-break-inside-avoid-1/ [Test harness] http://test.csswg.org/harness/test/css21_dev/single/moz-css21-block-page-break-inside-avoid-1/

(In reply to Mats Palmgren (:mats) from comment #50) > IIRC, we specifically added @page rules to all these > tests so that they should render correctly (in Print Preview) in other > browsers, > assuming they support @page sizes. I always assumed that print tests in W3C CSS Test suites were manual tests. (In reply to Daniel Holbert [:dholbert] from comment #51) > Ah, gotcha -- so the "@page" rule is meant to mimic "reftest-print"? I think you should propose and explain this idea of "reftest-print" (whatever it means, implies) in Public CSS Test suite mailing list. We have and use "reftest-wait" Controlling When Comparison Occurs http://testthewebforward.org/docs/reftests.html#controlling-when-comparison-occurs I believe that an at-rule page is *not* meant to mimic "reftest-print" in W3C Test harness.

This "reftest-print" idea is most likely exclusively meant for comparing a test in print media with a reference file also in print media. The reference file itself has to be rendered in print preview: http://test.csswg.org/source/vendor-imports/mozilla/mozilla-central-reftests/css21/pagination/moz-css21-block-page-break-inside-avoid-ref.html I think the use of @page rule is really part of that test.

Yes, these are really meant to be automated, just like other reftests. The difference is that we load them (test and reference) in Print Preview (or something that closely mimics it) and then take a screenshot of that. Then compare the screenshots as usual for reftests. We could add the meta tag to the reftest-print tests if that helps. A patch would be welcome.