Description: If content that overflows a fixed-height container is tall enough to cross the page break, the container (and all its ancestors) stretches to fit all page breaks spanned by the overflowing content. The container should end at its prescribed height, and the overflowing content only should break across pages. It should be sure to print nonetheless (and paint at the right z-index).

What we currently seem to do: 1. A block that doesn't fit before the page break returns INCOMPLETE and the remaining content splits off into its new NextInFlow (which is also its NextSibling). 2. The containing block stops reflowing content, breaks the new NIF's NextSibling link and pushes all remaining lines (including the new NIF) as overflow. The frame gets sized to take up whatever computed height is left after its own prev-in-flows (if any). (If all the height has been taken up already, the frame gets zero height.) This continues up to the doc root frame, whose INCOMPLETE status gets noticed by nsSimplePageFrame. 3. nsSimplePageFrame creates a new nsPageFrame+nsContentFrame pair, and the new nsPageFrame splits the doc root frame from the previous nsPageFrame and takes this new Next-in-Flow as its only child. The end result is that the fixed height frame really only takes up its allocated space as far as the frame rects are concerned, but the gap between the short frame and its next-in-flow on the next page is still not available for the frame's siblings' placement. Following content gets pushed to the last page of overflowing content, where the fixed height's frame's [zero-height] last-in-flow is placed.

(Note that this also means the fixed-height block's bottom border is painted at the top of the last page spanned by its overflow instead of at the end of its height.)

So, what we do currently to break content over pages is described in http://wiki.mozilla.org/Gecko:Continuation_Model Taking the example given there, with the root expanded for page breaking, we get (P=nsPageFrame+nsPageContentFrame) Original frame tree Frame tree with A (root) broken into three parts nsSimplePageSeq nsSimplePageSeq | /|\ | / | \ P P1 P2 P3 | | | | A A1 A2 A3 / \ / | | | B C B C1 C2 C3 | /|\ | | | \ | D E F G D E F G1 G2 Now, suppose F has a fixed height, but contains enough content to span three pages. Currently the frame tree (afaict) looks like this: nsSimplePageSeq / | \ \ \ P1 P2 P3 P4 P5 / | | | | A1 A2 A3 A4 A5 / | | | | \ B C1 C2 C3 C4 C5 | | | | | \ \ D E F F2 F3 G1 G2 (And F3 has zero height.) That gives us five pages, with the content after F pushed back several pages. What we really wanted was four pages: the same pagination as before, but with F's content spanning P2, P3, and being the sole content of P4. We don't even want the root to be shown on P4: it's supposed to end and paint its border after G2. The problem is that F can't exist on a page unless all its ancestors exist on that page. (And short circuiting this somehow would probably screw up our paint order.) So what I'm thinking is maybe we should have our frame tree look like nsSimplePageSeq / | \ \ P1 P2 P3 P4 / | \ \ A1 A2 A3 A'4 / | | \ \ B C1 C2 C3 C'4 | | | \ | \ \ D E F1 G1 F'2 G2 F'3 Where X' is a frame that serves only to hold overflow content, i.e. it has zero height, is ignored in margin collapsing, and doesn't count as part of the element's visible "box": borders are painted for the last normal in-flow frame, not the special overflow frames. The overflow frames are still linked up as next-in-flows, but might not be contiguous in a breadth-first traversal. Any thoughts?

> Where X' You mean F', I presume. That might work. How are we going to arrange for reflow of F1 to say a) yes I'm complete but b) I need a continuation F'? It seems like we'll need a new reflow status, NS_FRAME_COMPLETE_EXCEPT_FOR_CHILDREN or something. The parent will need to treat that as complete and carry on reflowing siblings, but it needs to create a continuation for the child and put that in its overflow list, and if it would normally return NS_FRAME_COMPLETE, it needs to return NS_FRAME_COMPLETE_EXCEPT_FOR_CHILDREN instead. This would be hard, but feasible, and maybe no harder than any other solution. Need to think about how it would work for tables, though --- that could be nasty. An alternative idea I had is to return NS_FRAME_COMPLETE but put F1's overflowing children in a special list in nsHTMLReflowState, to be collected by the vertically breaking parent frame (nsSimplePageSequence) and made special children of the next page/column (P3 in this case), much like positioned children, on their own child list. That would also be hard, but maybe less hard.

No, I meant A', C', and F'. They all need to be stealth frames. I thought about your alternative method at first, but then realized getting things to paint in the right order in that case would be... very hard. Note that F'2 has to paint after C2 but before G2. Throw in z-index on a few ancestors and you've got quite a tangle.

Oh, what sort of problems are you anticipating with tables?

(In reply to comment #4) > An alternative idea I had is to return NS_FRAME_COMPLETE but put F1's > overflowing children in a special list in nsHTMLReflowState, to be collected by > the vertically breaking parent frame (nsSimplePageSequence) and made special > children of the next page/column (P3 in this case), much like positioned > children, on their own child list. That would also be hard, but maybe less > hard. I was thinking something similar, actually. I don't think painting should be too hard. Keeping layout up-to-date on dynamic changes seems likely to be harder.

(In reply to comment #7) > (In reply to comment #4) > I was thinking something similar, actually. > > I don't think painting should be too hard. Actually I think fantasai's right; z-ordering and clipping (and whatever other container effects we add down the road, like filters) are going to be very hard with this approach.

(In reply to comment #6) > Oh, what sort of problems are you anticipating with tables? For blocks, the X' frames can behave much like empty blocks. But for tables, what kind of frames should the X' frames be? If we make them table cells, rows, rowgroups and tables, it will be icky because they won't behave like cells, rows, rowgroups and tables. But if we make them some other kind of frame, it will complicate all levels of table logic because we'll have to deal with the possibility that a frame's continuation is not the same type as the frame.

Although we could take an incremental approach where we first fix blocks that way and without fixing tables ... any frame could treat a child returning NS_FRAME_COMPLETE_EXCEPT_FOR_CHILDREN as NS_FRAME_NOT_COMPLETE.

Well, I was thinking of creating a nsOverflowContainer class for all the X' frames, since they don't actually need to do any layout but do need special treatment to be invisible. F' will need to do block layout, though, so it needs to be either an nsBlockFrame or a derivative of it. I like the idea of starting with just blocks though. I'm going to start drawing up an nsOverflowBlockFrame and see where it takes me.

(In reply to comment #8) > Actually I think fantasai's right; z-ordering and clipping (and whatever other > container effects we add down the road, like filters) are going to be very hard > with this approach. We already handle that stuff for out-of-flows with placeholder frames. Why can't we use the same mechanism? Maybe even have actual placeholder frames? (Alternatively, we could have multiple next-in-flows as children of the same parent (on the earlier page/column) and siblingns of each other, but position them in different columns/pages. This would require a concept of giving all the pages non-overlapping positions.)

(In reply to comment #12) > We already handle that stuff for out-of-flows with placeholder frames. Why > can't we use the same mechanism? Maybe even have actual placeholder frames? For float continuations, we have a placeholder for each continuation of the float, yes. We handle the parent completing before its floats by pushing placeholders up to the nearest enclosing ancestor that isn't complete. a) this probably has a bug, come to think of it, when a placeholder floats up out of a relatively positioned block with z-index set on it. So we basically don't handle these hypothetical painting problems, because we mostly don't have to. b) this is also simpler because float positions don't depend on the geometry of the placeholder ancestors. c) placeholders can only go up to the nearest block formatting context ancestor, so it's block frames all the way up. The general case is more complex because we have to push up through anything that can break vertically.

First pass. This more or less works. (Less because it doesn't address tables, or anything except block frames, *at all*, but it passes my simplified testcase and a few others. And please ignore the get/set/append stuff on nsContainerFrame; that needs cleanup.) It's enough to get a really good idea of where I'm going with this, though. And for me to ask some questions.

Comment on attachment 264543 [details] [diff] [review] patch pass 1 Ok, so here's how this works: 1. I added a new state bit to nsIFrame: +// If this bit is set the frame is holding overflow continuation, +// i.e. it is a next-in-flow to hold overflow after the box's height +// has ended. This means the frame should be a) at the top of the +// page and b) invisible: no borders, zero height, ignored in margin +// collapsing, etc. +#define NS_FRAME_IS_OVERFLOW_CONTAINER 0x00010000 I'm guessing that's not ok, since we ran out of reserved state bits in nsIFrame already: #define NS_FRAME_RESERVED 0x000FFFFF I don't know what #define NS_FRAME_IMPL_RESERVED 0xFFF00000 means, though. 2. I also added a new reflow status, which is (currently; this can change) mutually exclusive with NS_FRAME_NOT_COMPLETE. + * NS_FRAME_OVERFLOW_NOT_COMPLETE bit flag means that the frame has + * overflow that is not complete, but its own box is complete. + * (It is mutually exclusive with NS_FRAME_NOT_COMPLETE.) + * The reflower should place and size the frame and continue reflow, + * but make sure overflow NIFs are handled correctly. + * (This happens when content overflows a fixed-height box.) 3. I added two new frame lists to nsContainerFrame. One holds all the overflow container children of a frame. + nsFrameList mOverflowContainers; The other holds the overflow container overflow, which is pulled by the next-in-flow during reflow. This one's implemented as a property of the PresContext, just like regular overflow. +GK_ATOM(overflowOverflowContainersProperty, "overflowOverflowContainersProperty") // nsFrameList* 4. I created a ReflowOverflowContainerChildren method for nsContainerFrame, which gets called from nsBlockFrame::Reflow. This takes care of any overflow containers we already have. + /** + * Reflow overflow container children. They are invisible to normal reflow + * (i.e. don't affect sizing or placement of other children) and inherit + * width and horizontal position from their prev-in-flow. + * + * This method + * 1. Pulls overflow container overflow from the prev-in-flow + * and adds them to our overflow container list before reflowing. + * 2. Reflows all our overflow container kids + * 3. Expands aOverflowRect as necessary to accommodate these children. + * 4. Sets aStatus = NS_FRAME_OVERFLOW_IS_INCOMPLETE if any overflow + * children are incomplete and + * 5. Prepends a list of their next-in-flows to our overflow container + * overflow list, to be reflowed into our own next-in-flow when + * it is reflowed. The caller is responsible for appending + * any new overflow container continuations it makes to this list. + * //XXXfr make this take reflow-nextinflow flags into account + */ 5. I also modified nsBlockFrame's reflow to create overflow containers as necessary. How this works is summarized in nsContainerFrame.h + /** + * Overflow containers are continuation frames that hold overflow. They + * are created when the frame runs out of computed height, but still has + * too much content to fit in the availableHeight. The parent creates a + * continuation as usual, but marks it as NS_FRAME_IS_OVERFLOW_CONTAINER + * and appends it to its overflow container overflow list (by calling + * AppendOverflowContainerOverflow). (The parent also continues reflow + * as if the frame was complete once it ran out of computed height.) The + * parent's next-in-flow is then responsible for calling + * ReflowOverflowContainerChildren to drain and reflow these overflow + * continuations. Overflow containers do not affect other frames' size + * or position during reflow (but do affect their parent's overflow area). + * See nsBlockFrame::Reflow for examples. + */ 6. Any time the parent (including ReflowOverflowContainerChildren) creates an overflow container continuation, it must be sure to pass up either NS_FRAME_NOT_COMPLETE (if it is not complete) or NS_FRAME_OVERFLOW_INCOMPLETE (if it's otherwise complete) so that a next-in-flow will be created to take the overflow. Questions: - Firstly, just to double-check, does this seem like a reasonable direction to be taking? - It's obvious to me that we don't want mOverflowContainers on nsContainerFrame; that's 4 bytes per frame that we almost never need. There are two reasonable ways I know of to deal with this: a) Make it a property of the PresContext, just like overflow is today. Pros: Pretty straightforward. Cons: Painting will require every block frame to 1. dig up a reference to the prescontext 2. perform a lookup in the proptable to see if any overflow containers need to be painted b) Prepend these containers to the mFrames list. Pros: Saves space and cycles. This would have almost no overhead once implemented Cons: Need to make all frames skip over all NS_FRAME_IS_OVERFLOW_CONTAINER children at the front of their mFrames list. It's not straightforward. - If the NS_FRAME_IS_OVERFLOW_CONTAINER state flag is not ok as-is, what should I do as an alternative way of marking frames as overflow containers? - Currently this is only used to set GetSkipSides and suchlike. - If we go with the prepend-these-to-mFrames approach, it will also be necessary there. - Names? "overflow overflow container" is a bit dizzy. We've got two concepts of overflow: stuff that doesn't fit inside the frame's nsRect but belongs to the frame, and stuff that doesn't fit inside the availableHeight and therefore needs to go into another frame. This fix is dealing with the intersection of both.

We can free up a global frame state bit by converting usage of NS_FRAME_IS_BOX to an IsFrameOfType check.

> - Firstly, just to double-check, does this seem like a reasonable > direction to be taking? I think so. > a) Make it a property of the PresContext, just like overflow > is today. > Pros: Pretty straightforward. > Cons: Painting will require every block frame to > 1. dig up a reference to the prescontext > 2. perform a lookup in the proptable to see > if any overflow containers need to be painted I think this is the way to go. A frame can only have overflow container children if it has a prev-in-flow, right? That's rare so we can just check GetPrevInFlow() and do no extra work if that's null. > Names? "overflow overflow container" is a bit dizzy. how about using a term such as "extra frames" to describe the first kind of overflow?

> We can free up a global frame state bit by converting usage of NS_FRAME_IS_BOX > to an IsFrameOfType check. Ok, I'll look into that. > A frame can only have overflow container children if it has a prev-in-flow, > right? That's rare so we can just check GetPrevInFlow() and do no extra work > if that's null. Yep, I just thought of that this morning. :) > how about using a term such as "extra frames" to describe the first kind of > overflow? Or "excess" maybe. Making that consistent globally would involve making a lot of changes to comments and to the nsContainerFrame Get/Set method names. Is that something you'd like me to do?

Actually I was thinking that just the frames with NS_FRAME_IS_OVERFLOW_CONTAINER should have a new name, to reduce the load on the word 'overflow'. Maybe "NS_FRAME_IS_VESTIGE"? or is that too esoteric :-)

Some of the overflows discussed bear some resemblance to the typographical expression "widows".

That is too esoteric. :) I don't think this is really the same as widows. (Also we probably want to implement support for widows controls at some point.) I'll just go with overflow containers and excess overflow containers.

How about "leftover containers"?

So.. renaming the doesn't-fit-in-availableHeight kind of overflow makes sense. But I don't think we should also or instead rename the doesn't-fit-in-rect. Introducing terminology that is inconsistent with the rest of our codebase *and* the CSS spec is imho not a good idea.

I'm not suggesting we change doesn't-fit-in-rect. I am suggesting that for these specific frames which are NS_FRAME_IS_OVERFLOW_CONTAINER, we invent a new name that does not include the word "overflow".

How about: SPANS_PAGE, SPANS_BREAK or SPANS_PAGE_BREAK

But roc, the thing that is special about these frames is that they are holding overflow in the doesn't-fit-in-rect sense. Bill, we have other frames that span breaks, too.

Other frames also have overflowing children. What's really special about these frames is that they aren't associated with any of the "normal" height of their element. That is an entirely new concept that can have a new name, even though it is obviously related to both kind of overflow.

Second pass. General cleanup of the first and fixes overflow behavior on the root element. nsContainerFrame and nsIFrame changes are at a state where I'd feel ready to ask for review, possibly also the page frames. This passes all the tests above except the Interlaced one; there are still some border-related issues with that, and I haven't figured out if they need to be fixed here or is part of a separate bug. (We have some very strange behavior wrt borders and page breaks, generally speaking.) Still haven't tackled tables.

Could the testcases perhaps be converted to printing reftest format (bug 374050), or some sort of automated format such that they can be run with the usual testing commands?

Probably. They're currently in CSS2.1 Test Suite format. Do you want reftests available now, specifically? The Interlaced tests won't pass until bug 381497 is fixed.

Reftests would be fine, assuming the printing reftest support is sufficient. I doubt Mochitest has the right stuff to actually do the tests, but I could be wrong. The point is more to have tests in place so that if this broke the problem would be immediately diagnosed, not lie latent until someone happened to stumble upon it and take the steps to report it. As for which tests, of course more are better, but if some test awaits bug 381497 being fixed, just make a note there and add the test when the fix happens.

So I've been chasing a crasher in the pass 2 code (and fixing leaks and other possible crashers along the way). I think I've found the problem and it's that the code grabs an existing next-in-flow if there's one there already and puts a reference to it in the overflow container lists -- but it doesn't take it out of the normal child list, so there's a dangling pointer left there. We have a RemoveFrame function, but a) it destroys the frame, which I don't want to do because we're still using the frame, just not in this list b) it also deletes all the continuations, which I don't want to do, since they get handled later during reflow c) and it triggers a reflow command, which I don't want to do because we're already taking care of it in this current reflow So I'm writing a StealFrame function that just takes the frame out of the child list. Where should it go? I'm currently putting it on nsContainerFrame.

I think we usually use "Pull". See nsInlineFrame::PullOneFrame and nsBlockFrame::PullFrame.

So, multi-col is great for debugging paginated reflow. Known issues with this patch: - some parts of nsBlockFrame (and possibly other places) are resetting reflow flags. This probably causes overflow truncation in some reflow paths, e.g. floats, but I don't have testcases for those yet - horizontal resize causes columns to disappear in at least one testcase probably another reflow flag problem - presshell is reporting a leak after first set of click1/click2 in the interlaced interactive test. Doesn't seem to be a blockframe because I tried setting a counter there and it always came back to zero.

This is ready for review. There's at least one question marked XXXfr, also, please let me know if the NS_FRAME_OVERFLOW_INCOMPLETE flag should be allowed to coincide with NS_FRAME_NOT_COMPLETE instead of being mutually exclusive with it.

Can you cook up some reftests for this stuff? Eli created a way to do paginated reftests, but I forget how that works...

I'm trying to remember why I started using nsFrameList* in properties instead of just nsIFrame*. I guess it lets you easily change the list in-place without an extra property operation, at the cost of an extra heap object. Is that right? Why did you change overflowProperty to work that way?

I changed the overflowProperty so that I could consolidate its code with the overflowContainersProperty and excessOverflowContainersProperty. It's a list of frames, just like the other two, so I figured it made more sense for them all to share the generic get/set functions. If I understand correctly, the overflowProperty is a temporary object: it doesn't exist once reflow is completed. So we shouldn't be overly-concerned about space there.

It's the cost of dynamically allocating the nsFrameList that I'm most worried about. Is there a reason you can't make everything use nsIFrame* directly instead? nsBlockFrame::GetOverflowOutOfFlows returns an nsFrameList but stores the property as an nsIFrame*. nsBlockFrame::GetOverflowPlaceholders stores an nsFrameList* but I think it could, and probably should, use an nsIFrame* instead. You're adding two more property lookups to nsContainerFrame::Destroy. I'm a wee bit concerned about that, but I'm not sure what to do about it. Index: layout/base/nsPresShell.cpp Can you prune your patch so that non-changes like this are removed? Maybe that's the only one though. Also, could you give me 12 lines or context or so? And use -p to get the function name in the diff hunks. And give me a diff -w because quite a bit of this is just indentation changes... How about splitting out the change from NS_FRAME_IS_BOX to IsFrameOfType to its own patch? That will be easier for everyone. We may even have a bug for you to attach it to. Other cleanup like adding comments to existing code, whitespace changes to align parameters, typo fixes, and the like, would also be nice to have as a separate patch. There are some comment references to NS_FRAME_OVERFLOW_CONTAINER where you mean NS_FRAME_IS_OVERFLOW_CONTAINER. + * too much content to fit in the availableHeight. The parent creates a + * continuation as usual, but marks it as NS_FRAME_IS_OVERFLOW_CONTAINER + * and adds it to its next-in-flow's overflow container list, either by + * adding it directly or by putting it in its own excess overflow containers + * list (to be drained by the next-in-flow when it calls + * ReflowOverflowContainerChildren). Under what circumstances would you want to add it directly?

Could we allocate the nsFrameList from the presshell arena or something? Or would that still be a perf issue?

We could, I guess. I don't know if there's a real perf issue for the nsFrameList or not, but it seems to me the code would be just as simple without it, so why not get rid of it.

I can revert the nsFrameList change for the overflowProperty, but the overflowContainersProperty needs a nsFrameList. During reflow two parts of the code are accessing the list simultaneously: the tracker is walking the next-in-flow's list and adding frames as they are created while the reflow code is potentially removing frames from that list whenever it calls DeleteNextInFlowChild for a newly-complete child frame. Given that overflowContainers are going to be very rare, I don't think the nsFrameList allocation is a problem in this case. > adding two more property lookups to nsContainerFrame::Destroy Also adding said lookups to nsBlockFrame::Reflow... > Under what circumstances would you want to add it directly? The tracker grabs the overflowContainers list from the next-in-flow if it exists, and in that case they are added directly. If the next-in-flow doesn't yet exist, then it has to use the excessOverflowContainers list from the frame itself. The overflowContainers list is not like the overflowProperty: the frames are not necessarily adjacent siblings, and you can't simply prepend or append new frames: if the list already exists, they have to be spliced in.

I still have hopes of moving to a world where all frame lists are nsFrameList and nsFrameList can get the last frame in O(1) time so that we can resolve some longstanding performance bugs... but I guess in this case it really doesn't matter much. Just more code to fix later.

filed bug 386142 for comments, 386141 for NS_FRAME_IS_BOX

Comment on attachment 270133 [details] [diff] [review] patch (-up12w) without overflowProperty changes >+ //XXXfr this should also be set if our computed height has changed >+ // since the last reflow. I don't know how to detect that. Any changes to the computed height will result in a style change reflow, so you don't have to worry about that. > deltaY = line->mBounds.YMost() - oldYMost; > } else { >+ aState.mOverflowTracker.Skip(line->mFirstChild, aState.mReflowStatus); Do you want a line->IsBlock() check here? > ::PlaceFrameView(this); > > // We can skip trying to pull up the next line if there is no next > // in flow or we were told not to or we know it will be futile, i.e., > // -- the next in flow is not changing > // -- and we cannot have added more space for its first line to be > // pulled up into, > // -- it's an incremental reflow of a descendant > // -- and we didn't reflow any floats (so the available space > // didn't change) > // XXXldb We should also check that the first line of the next-in-flow > // isn't dirty. >- if (!aState.mNextInFlow || >- (aState.mReflowState.mFlags.mNextInFlowUntouched && >+ if (aState.mReflowState.mFlags.mNextInFlowUntouched && > !lastLineMovedUp && > !(GetStateBits() & NS_FRAME_IS_DIRTY) && >- !reflowedFloat)) { >- if (aState.mNextInFlow) { >- aState.mReflowStatus |= NS_FRAME_NOT_COMPLETE; >+ !reflowedFloat) { >+ NS_FRAME_SET_INCOMPLETE(aState.mReflowStatus); > } >- } else { >+ else if (aState.mNextInFlow) { > // Pull data from a next-in-flow if there's still room for more > // content here. > while (keepGoing && (nsnull != aState.mNextInFlow)) { > // Grab first line from our next-in-flow > nsBlockFrame* nextInFlow = aState.mNextInFlow; > line_iterator nifLine = nextInFlow->begin_lines(); > nsLineBox *toMove; > PRBool collectOverflowFloats; > if (nifLine != nextInFlow->end_lines()) { > if (HandleOverflowPlaceholdersOnPulledLine(aState, nifLine)) { > // go around again in case the line was deleted > continue; >@@ -2026,25 +2047,25 @@ nsBlockFrame::ReflowDirtyLines(nsBlockRe > if (LineHasClear(line.get())) { > foundAnyClears = PR_TRUE; > } > > // If this is an inline frame then its time to stop > ++line; > aState.AdvanceToNextLine(); > } > } > > if (NS_FRAME_IS_NOT_COMPLETE(aState.mReflowStatus)) { > aState.mReflowStatus |= NS_FRAME_REFLOW_NEXTINFLOW; >- } >+ } //XXXfr shouldn't set this flag when nextinflow has no lines Why? And what exactly are the changes here trying to accomplish? (In what situation do we not need to reflow the next-in-flow if we reflowed ourselves not complete?) > ReflowChild(firstKid, aPresContext, kidSize, kidReflowState, > kidOrigin.x, kidOrigin.y, 0, aStatus); >+ if (NS_FRAME_OVERFLOW_IS_INCOMPLETE(aStatus)) { >+ // Don't pass OVERFLOW_INCOMPLETE through tables until they can actually handle it >+ //XXX should paginate overflow as overflow, but not in this patch (bug 379349) >+ NS_FRAME_SET_INCOMPLETE(aStatus); >+ printf("Set table cell incomplete %p\n", this); >+ } The idea here is that it's a higher priority to preserve the overflow than to preserve the layout? I'd like to see some tests with floats (it looks like you didn't add overflow continuations for them, but everything needs to behave sanely in their presence), some tests with tables (even if they fail, it would be good to make sure they don't crash or anything like that), and some tests dynamically inserting and deleting all sorts of stuff to make sure frame insertion and destruction works correctly. It would also be good to have some printing reftests, both for testing overflow and testing for any unexpected interactions with floats or tables or columns or anything like that. (Especially tables, since they behave significantly differently in printing contexts.) I'm willing to help you write tests if you want me to.

> Any changes to the computed height will result in a style change reflow, so > you don't have to worry about that. Ok, I'll remove that comment. > Do you want a line->IsBlock() check here? IsBlock() would make sense if Skip() cost us anything significant, but Skip() is an inline function for this reason, and the first line is an if test that will fail on all non-blocks anyway. > Why? And what exactly are the changes here trying to accomplish? The changes there are mainly to switch from |= NS_FRAME_NOT_COMPLETE to using the macros. Other than that, the changes should be a nop but should make that if clause a little easier to follow. > In what situation do we not need to reflow the next-in-flow if we reflowed > ourselves not complete? The code here is pulling lines from the next-in-flow. If any lines were pulled, it needs to signal a REFLOW_NEXTINFLOW. If the next-in-flow has no lines to pull up (which now happens when it only contains overflow containers), then there is no need for this code to trigger a reflow. (If the overflow containers need a reflow, that will be triggered elsewhere.) > The idea here is that it's a higher priority to preserve the overflow than to > preserve the layout? The idea here is that I don't want to touch tables until I can take a seriously good look at what needs to be done with them. > I'd like to see some tests with floats ... some tests with tables Yeah, I did make a variant of the Interactive test with floats and a printing test for tables. I'll clean them up a bit and attach them here. > some tests dynamically inserting and deleting all sorts of stuff to make sure > frame insertion and destruction works correctly The "Interactive" test above does trigger that sort of thing. (It is included with the reftests as well.) If you want something more than that, throw me some ideas.

(In reply to comment #61) > > The idea here is that it's a higher priority to preserve the overflow than to > > preserve the layout? > > The idea here is that I don't want to touch tables until I can take a seriously > good look at what needs to be done with them. Well, the alternative would be to suppress the overflow flag in tables. > > some tests dynamically inserting and deleting all sorts of stuff to make sure > > frame insertion and destruction works correctly > > The "Interactive" test above does trigger that sort of thing. (It is included > with the reftests as well.) If you want something more than that, throw me some > ideas. I saw that you have some tests messing around with height; I was asking about stuff using innerHTML or insert/append/removeChild to test frame construction/destruction. (It would also be a good idea to have a test with style changes to 'color' to test that invalidation works; invalidation isn't reftestable, but it would be good to check.)

I'm using background-color instead of color, is that ok?

Comment on attachment 269180 [details] [diff] [review] patch So I've got a float testcase that is asserting a lot. Need to work through that.

This testcase passes.

This patch shouldn't affect this testcase, since I haven't implemented overflow pagination for tables.

This passes all the floating tests as well. I haven't figured out /why/ it passes the "Overflow overflowing Floats" one, though; I'd have expected that to fail.

One more thing: It would be nice to have some printing reftests for tables, to make sure it doesn't change behavior for paginating tables.

+ // If this bit is set the frame a continuation that is holding overflow, "is a" + * This bit is mutually exclusive with NS_FRAME_OVERFLOW_NOT_COMPLETE. OVERFLOW_INCOMPLETE You might want to make it clearer here that there are three possibilities: -- frame is fully complete (NS_FRAME_IS_COMPLETE and !NS_FRAME_OVERFLOW_IS_INCOMPLETE) -- frame is complete but has incomplete overflow (NS_FRAME_IS_COMPLETE and NS_FRAME_OVERFLOW_IS_INCOMPLETE) -- frame is not complete (!NS_FRAME_IS_COMPLETE and !NS_FRAME_OVERFLOW_IS_INCOMPLETE) + overflowContainerBounds, 0x0, Just "0" is fine if (state.mOverflowPlaceholders.NotEmpty()) { NS_ASSERTION(aReflowState.availableHeight != NS_UNCONSTRAINEDSIZE, "Somehow we failed to fit all content, even though we have unlimited space!"); - if (NS_FRAME_IS_COMPLETE(state.mReflowStatus)) { + if (NS_FRAME_IS_FULLY_COMPLETE(state.mReflowStatus)) { Hmm. Can't we get rid of this placeholder-wrangling entirely and say that if there are overflow placeholders, then this block is is OVERFLOW_INCOMPLETE? More later...

> Hmm. Can't we get rid of this placeholder-wrangling entirely and say that if > there are overflow placeholders, then this block is is OVERFLOW_INCOMPLETE? I'm not sure it'll be as simple as that. Normal boxes can overflow pretty much any box except a scrollframe, but floats can't overflow a block formatting context root: they stretch it instead.

Right. I was going to suggest making block formatting context blocks grow to accommodate overflow container children, but that's not right for non-float children. So I guess we're stuck with that for now. + //XXX should give the pagecontentframe a previnflow, too, so we don't + // have to jump through hoops here How hard would that be to fix? + if (!nextFrame) { + nsresult rv = aPresContext->PresShell()->FrameConstructor()-> + CreateContinuingFrame(aPresContext, frame, this, + &nextFrame); + NS_ENSURE_SUCCESS(rv, rv); + frame->SetNextSibling(nextFrame->GetNextSibling()); + nextFrame->SetNextSibling(nsnull); + SetOverflowFrames(aPresContext, nextFrame); + // Root overflow containers will be normal children of + // the pageContentFrame, but that's ok because there + // aren't any other frames we need to isolate them from + // during reflow. + } Can you use nsHTMLContainerFrame::CreateNextInFlow here? Do we need nsContainerFrame::GetOrCreatePropTableFrames? Seems the one caller could just call GetRemovePropTableFrames and SetPropTableFrames.

> "is a" > OVERFLOW_INCOMPLETE > clearer that there are three possibilities > Just "0" is fine > give the pagecontentframe a previnflow > use nsHTMLContainerFrame::CreateNextInFlow here Done. > Do we need nsContainerFrame::GetOrCreatePropTableFrames? Removed.

Could Eli review this first? (I think it will be a while before I get to it.) Also, looks like the patch is missing the reftests. Did you cvs add (or local equivalent), and include -N in the options to cvs diff?

Comment on attachment 272832 [details] [diff] [review] patch Swapping review requests. (Confirmed as ok by dbaron.) The reftests are in the reftest.zip attachment. Let me know if you'd rather have them in the patch itself. Eli, we don't break overflow across pages even now. I'll attach a test case so you can see. Do you still want a reftest for it (that always fails)?

- const nsMargin& borderPadding = aState.BorderPadding(); + nsMargin borderPadding = aState.BorderPadding(); What's the point of this change? nsBlockFrame* block = (nsBlockFrame*)aFrame->GetParent(); + // Remove aFrame from the appropriate list. Random change. //XXXfr Floats can't be overflow incomplete yet if (NS_FRAME_OVERFLOW_IS_INCOMPLETE(aReflowStatus)) NS_FRAME_SET_INCOMPLETE(aReflowStatus); Is this really right? Won't this create continuations for overflowing floats?

> What's the point of this change? > Random change. Nothing. Removed. > Is this really right? Won't this create continuations for overflowing floats? This is preserving the behavior we had before. Before this change, things that are now OVERFLOW_INCOMPLETE used to be NOT_COMPLETE.

+ frame->SetNextSibling(nextFrame->GetNextSibling()); + nextFrame->SetNextSibling(nsnull); CreateNextInFlow does these for you, that's why I suggested calling it. + * For each child, the reflower should either Do you want to say something about the order the children should be processed in? > aWalkOOFFrames is ignored if aSkipOverflowContainerChildren is true. Obsolete comment? Also here: > Causes aWalkOOFFrames to be ignored. + * Don't set aSkipOverflowContainerChildren to PR_FALSE unless you plan + * to walk your own overflow container children. If this is PR_TRUE, then the comments above about what the reflower must do do not apply, right? Better make that clear. + nsOverflowContinuationTracker(nsPresContext* aPresContext, + nsresult Insert(nsPresContext* aPresContext, Rather than taking nsPresContext parameters, just get the prescontext from the nearest handy frame. In nsBlockFrame::ReflowDirtyLines: + aState.mOverflowTracker.Skip(line->mFirstChild, aState.mReflowStatus); You're only skipping the first child on the line, but there could be multiple children on the line. Except that this only matters for block children and in that case there's only one child on the line, so you're safe. I would wrap this in a check of line->IsBlock() just to be clear. In nsContainerFrame::Destroy: + nsFrameList* frameList = GetRemovePropTableFrames(prescontext, PR_TRUE, + nsGkAtoms::overflowContainersProperty); This lookup can be guarded by GetPrevContinuation() != nsnull, right? That should be fast. Maybe there should be a specific function for getting this property that does that check. + frameList = GetRemovePropTableFrames(prescontext, PR_TRUE, + nsGkAtoms::excessOverflowContainersProperty); But this one can't be guarded by anything cheap that I can think of. We could free up another state bit (EXCLUDE_IGNORABLE_WHITESPACE converting to IsFrameOfType) and use that bit to indicate whether we have a non-empty excessOverflowContainersProperty, overflowProperty, or excessOverflowContainersProperty. + if (frameList) { + frameList->DestroyFrames(); + delete frameList; + } Factor this into a little helper function. You can call it from DestroyFrameList too. I like the tracker approach. +inline static void +ReparentFrames(nsPresContext* aPresContext, + nsIFrame* aFrames, + nsIFrame* aNewParent) This handles aFrames containing frames with different parents, but that can't happen can it? If not, you can simplify the code and call nsHTMLContainerFrame::ReparentFrameViewList. +/** + * Reflow overflow container children. They are invisible to normal reflow + * (i.e. don't affect sizing or placement of other children) and inherit + * width and horizontal position from their prev-in-flow. Don't duplicate this comment. Probably just delete the one in nsContainerFrame.cpp. And document aFlags. + if (!overflowContainers) { + // Drain excess from previnflow Shouldn't this be unconditional? Here and elsewhere you seem to be assuming that there can't be excessOverflowContainers and a next-in-flow at the same time. But in the analogous situation for non-overflow-contains we can; there can be an overflowProperty and a next-in-flow after we reflow the frame and before we reflow the next-in-flow. In other words, we don't push frames into the child list of the next-in-flow, it always pulls (drains) from its prev in flow. Is there a reason to do it differently here? Is the reason that it simplifies nsOverflowContinuationTracker to only have to consider one list? + if (excessFrames) { + ReparentFrames(aPresContext, excessFrames->FirstChild(), this); + overflowContainers = excessFrames; + nsresult rv = SetPropTableFrames(aPresContext, overflowContainers, + nsGkAtoms::overflowContainersProperty); + NS_ENSURE_SUCCESS(rv,rv); + } Are we leaking excessFrames here? Maybe we should make Get and RemovePropTableFrames separate functions, and have Remove return an nsAutoPtr<nsFrameList>, and encourage callers to assign that to an nsAutoPtr variable. If they forget, they'll crash immediately instead of compiler error, but they're still likely to catch the bug. + if (!overflowContainers) { return NS_OK; } // nothing to reflow return NS_OK; on its own line (no curlies though) + //XXXfr Do we need to override any shrinkwrap effects here? + // e.g. desiredSize.width = prevRect.width; We probably should ... have you got a testcase for this? + frame->SetNextSibling(nif->GetNextSibling()); + nif->SetNextSibling(nsnull); As before, CreateNextInFlow does these. nsRect rect = frame->GetOverflowRect(); + BuildDisplayListForChild(aBuilder, frame, aDirtyRect, aLists); rect is unused? + return (nsFrameList*) propTable->UnsetProperty(this, aPropID); + return (nsFrameList*) propTable->GetProperty(this, aPropID); static_cast<> + // Removed frame and now list is empty. Delete it. + frameList = GetRemovePropTableFrames(aPresContext, PR_FALSE, + nsGkAtoms::overflowProperty); This is wrong, isn't it? overflowProperty is just an nsIFrame* now. + while (cur && cur->GetPrevInFlow()->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER) { parens around the & expression + if (aOverflowCont->GetStateBits() & NS_FRAME_IS_OVERFLOW_CONTAINER + && mParent != aOverflowCont->GetParent()) ditto

> CreateNextInFlow does these for you, that's why I suggested calling it. Notice that that code is disconnecting the frame, not connecting it. > Do you want to say something about the order the children should be processed > in? The same order they were processed the last time the parent frame was reflowed. Do you think that needs explanation? >> aWalkOOFFrames is ignored if aSkipOverflowContainerChildren is true. > Obsolete comment? Oops, sorry. That's from 154892, guess I forgot to strip it out of the .h. > If this is PR_TRUE, then the comments above about what the reflower must do > do not apply, right? No, they still apply. A frame is related to two sets of overflow containers: those that are its own children, and those that are continuations of its children. The tracker walks through the continuations (the frame's NIF's children) and their prev-in-flows (the frame's children) in parallel. Some of these continuations are continuations of the frame's normal children and some of them are continuations of its overflow container children. The rules for processing both are the same. Typically the frame will delegate processing of its overflow container children to ReflowOverflowContainerChildren, and therefore when invoked by the frame the tracker needs to skip past their continuations. > Rather than taking nsPresContext parameters, just get the prescontext from > the nearest handy frame. aPresContext is readily available to whoever's calling those functions, why not pass it in? I could get it from the frame for Insert, though, since that's rarely called, and sometimes not directly in Reflow(). > You're only skipping the first child on the line, but there could be multiple > children on the line. Except that this only matters for block children and in > that case there's only one child on the line, so you're safe. Right. > I would wrap this in a check of line->IsBlock() just to be clear. If the purpose is clarification, how about a comment? There's no actual need to check IsBlock here: since Skip() is inline and its first operation is a pointer comparison that will fail on all non-blocks, it doesn't even save any processing. > We could free up another state bit Well, let me know soon if you'd rather I did that. :) > I like the tracker approach. Thanks~ > Factor this into a little helper function. Ok, I'll add that to nsFrameList.h. Do you want this function inline? > Don't duplicate this comment. I've generally duplicated the .h function comments into the .cpp file. It helps me understand the code when I'm reading the .cpp, and it also reminds me to update the comment when I make changes to the function. To me, that outweighs the problems with keeping them both in sync. If you feel strongly about this I can remove them, but otherwise I'd prefer to keep the comments. > And document aFlags. aFlags just gets passed through to ReflowChild. I'll add that to the comment. > Is the reason that it simplifies nsOverflowContinuationTracker to only > have to consider one list? Yes. It's not really possible to have two lists unless you want to add code to *splice* them together. With normal overflow, you can simply prepend any new overflow to the old list. The new overflow is always before the old. However new overflow container continuations are not necessarily before all of the old ones. Consider, for example, an fixed-size element A with overflow followed by a fixed-size element B. If B gets content appended, during the next reflow its overflow has to be inserted *after* the overflow from A, not before. In the reverse situation the opposite must happen. Maybe I should enforce this "one list" policy more clearly, though. > Are we leaking excessFrames here? Hm.. if SetPropTableFrames fails then yes. I guess I'll fix that. > We probably should ... have you got a testcase for this? No, because they'd all involve either abspos frames, floats, or tables, none of which get processed here at this point. :) > As before, CreateNextInFlow does these. As before, CreateNextInFlow links the frame into the sibling chain whereas I need to unlink it. > rect is unused? Ahh.. printf code I forgot to remove. #_# > This is wrong, isn't it? overflowProperty is just an nsIFrame* now. > static_cast<> > parens around the & expression > ditto Thanks, fixed. I'll attach another patch with these fixes shortly.

> This handles aFrames containing frames with different parents, but that can't > happen can it? No, I don't think it can. (It did happen in previous some revisions.) > <elif> re: "things that are now OVERFLOW_INCOMPLETE used to be NOT_COMPLETE" > <elif> i thought NS_FRAME_OVERFLOW_IS_INCOMPLETE meant that the frame was > complete outside of overflow? Yes. What I'm saying is that before (i.e. without this patch), we considered frames incomplete (NS_FRAME_NOT_COMPLETE) if they had incomplete children. The patch doesn't change some cases of COMPLETE to OVERFLOW_INCOMPLETE, it changes some cases of NOT_COMPLETE to OVERFLOW_INCOMPLETE.

(In reply to comment #85) > > CreateNextInFlow does these for you, that's why I suggested calling it. > > Notice that that code is disconnecting the frame, not connecting it. Oops right. Then maybe it's not worth calling CreateNextInFlow after all. > > Do you want to say something about the order the children should be > > processed in? > > The same order they were processed the last time the parent frame was > reflowed. Do you think that needs explanation? Yes. > >> aWalkOOFFrames is ignored if aSkipOverflowContainerChildren is true. > > Obsolete comment? > > Oops, sorry. That's from 154892, guess I forgot to strip it out of the .h. > > > If this is PR_TRUE, then the comments above about what the reflower must do > > do not apply, right? > > No, they still apply. A frame is related to two sets of overflow containers: > those that are its own children, and those that are continuations of its > children. The tracker walks through the continuations (the frame's NIF's > children) and their prev-in-flows (the frame's children) in parallel. Some > of these continuations are continuations of the frame's normal children and > some of them are continuations of its overflow container children. The rules > for processing both are the same. Typically the frame will delegate processing > of its overflow container children to ReflowOverflowContainerChildren, and > therefore when invoked by the frame the tracker needs to skip past their > continuations. OK. It would be good to add this explanation to the comments somehow. > > Rather than taking nsPresContext parameters, just get the prescontext from > > the nearest handy frame. > > aPresContext is readily available to whoever's calling those functions, > why not pass it in? I could get it from the frame for Insert, though, since > that's rarely called, and sometimes not directly in Reflow(). Reducing the parameter count in APIs like this is worthwhile IMHO. > > You're only skipping the first child on the line, but there could be > > multiple children on the line. Except that this only matters for > > block children and in that case there's only one child on the line, so > > you're safe. > > Right. > > > I would wrap this in a check of line->IsBlock() just to be clear. > > If the purpose is clarification, how about a comment? > There's no actual need to check IsBlock here: since Skip() is inline and > its first operation is a pointer comparison that will fail on all non-blocks, > it doesn't even save any processing. That latter statement is not quite true since the line could contain multiple frames, the first of which is an inline-block. Also when we support tables the pointer comparison could succeed for a table. So when the line is !IsBlock(), the pointer comparison will fail, but for more complex reasons. Alright then, document that Skip() does nothing for frames that aren't IsBlockOutside, and comment in ReflowDirtyLines that this means calling Skip() on the first child of a !IsBlock() line is a no-op. > > We could free up another state bit > > Well, let me know soon if you'd rather I did that. :) The thing is, we won't know if it's necessary until we've done Tp measurements, which probably won't happen until we check it in ... unless you want to do your own Tp measurements now? Alternatively I suppose we could factor out a small part of the patch that just defines the child list atoms and tries to get the properties in Destroy() (and anywhere else we think might have perf impact) and try checking that in early to see what happens to tinderbox... BTW you haven't added these child lists to GetFirstChild(), is that deliberate? > > Factor this into a little helper function. > > Ok, I'll add that to nsFrameList.h. Do you want this function inline? No. > > Don't duplicate this comment. > > I've generally duplicated the .h function comments into the .cpp file. > It helps me understand the code when I'm reading the .cpp, and it also > reminds me to update the comment when I make changes to the function. > To me, that outweighs the problems with keeping them both in sync. If > you feel strongly about this I can remove them, but otherwise I'd prefer > to keep the comments. C++ sucks I guess :-(. I don't really mind, ask dbaron. > > And document aFlags. > > aFlags just gets passed through to ReflowChild. I'll add that to the comment. Yeah, it still needs a comment though since someone who wants to call the function doesn't know how it gets used. > > Is the reason that it simplifies nsOverflowContinuationTracker to only > > have to consider one list? > > Yes. It's not really possible to have two lists unless you want to add code > to *splice* them together. With normal overflow, you can simply prepend any > new overflow to the old list. The new overflow is always before the old. > However new overflow container continuations are not necessarily before all > of the old ones. Consider, for example, an fixed-size element A with overflow > followed by a fixed-size element B. If B gets content appended, during the > next reflow its overflow has to be inserted *after* the overflow from A, not > before. In the reverse situation the opposite must happen. > > Maybe I should enforce this "one list" policy more clearly, though. You should explain somewhere this "one list" invariant, and add comments where it is being maintained.

(In reply to comment #86) > > <elif> re: "things that are now OVERFLOW_INCOMPLETE used to be NOT_COMPLETE" > > <elif> i thought NS_FRAME_OVERFLOW_IS_INCOMPLETE meant that the frame was > > complete outside of overflow? > > Yes. What I'm saying is that before (i.e. without this patch), we considered > frames incomplete (NS_FRAME_NOT_COMPLETE) if they had incomplete children. The > patch doesn't change some cases of COMPLETE to OVERFLOW_INCOMPLETE, it changes > some cases of NOT_COMPLETE to OVERFLOW_INCOMPLETE. What I'm saying is that claiming that a float is incomplete when it does in fact fit (not considering overflow), we don't want to be claiming that it's incomplete because that will change the behavior. Specifically, we can't put any floats beneath an incomplete float. (I suspect this is why your "Overflow overflowing Floats" testcase appears to pass.)

(No, actually it passes because I didn't put any bottom borders on the float itself.) I'm not saying that the behavior we get by converting OVERFLOW_INCOMPLETE to NOT_COMPLETE is correct. It's not. But it's not a change from what we already have, either, and I simply cannot expand the scope of this patch to fix floats and all their placeholder glory within the next 7 days. > Then maybe it's not worth calling CreateNextInFlow after all. Do you want me to revert that, then? > That latter statement is not quite true since the line could contain multiple > frames, the first of which is an inline-block. ReflowBlockFrame() isn't called on inline blocks, and at this point that's the only code that generates overflow container continuations for the block frame. So Skip is indeed a nop on those, too. It's a nop on any frame that doesn't have an overflow continuation. (Adding more comments now...) > Alright then, document that Skip() does nothing for frames that aren't > IsBlockOutside This isn't a property of Skip(), it's a result of nsBlockFrame not creating overflow continuations for inline blocks... I'll try to make this clear, you can tell me if I succeed. :) > BTW you haven't added these child lists to GetFirstChild(), is that > deliberate? I didn't consider it necessary. Do you think I should?

> > Then maybe it's not worth calling CreateNextInFlow after all. > > Do you want me to revert that, then? It's up to you, do whatever is simpler code. > > BTW you haven't added these child lists to GetFirstChild(), is that > > deliberate? > > I didn't consider it necessary. Do you think I should? It's necessary. For example, if we reparent a frame subtree that contains this stuff, we need to crawl all frames in the subtree to reparent all views and/or widgets. If we miss some child lists then we will get subtle crashes in weird testcases.

Sorry about keeping asking about the float overflow thing; I didn't realize it was broken already...

Sorry that took so long.. I'm running into an assertion with this patch, "bad prev-in-flow ancestor chain" in nsBlockFrame.cpp. It doesn't assert when I set the debugger to break right before the assertion, though, and the frame trees check out ok when I draw them after reflow's over. printf reports that it's trying to check two frames that, judging from the frame tree diagrams, definitely aren't ancestor-descendant. Not sure what to do here.

Can you attach the frame tree from the start of DrainOverflowLines in which that assertion fires? (and mention the frames it fires on)

> comment in ReflowDirtyLines that this means calling Skip() > on the first child of a !IsBlock() line is a no-op. You haven't done this yet... + NS_FRAME_IS_OVERFLOW_CONTAINER & nextFrame->GetStateBits()) parens around the & expr + if (NS_FRAME_IS_NOT_COMPLETE(frameReflowStatus)) { + // If nextFrame used to be an overflow container, make it a normal block + if (!madeContinuation && + NS_FRAME_IS_OVERFLOW_CONTAINER & nextFrame->GetStateBits()) + { Why do we have to do this? If the overflow container already exists, can't we just leave it where it is? Or is this the easiest way to get it out of the overflowContainers/excessOverflowContainers list and transmute it into a regular frame? + // XXXfr what is this comment referring to? // Advance to next line since some of the block fit. That way // only the following lines will be pushed. Should just delete that comment. + PRBool ok = + parent->StealFrame(aState.mPresContext, nextFrame, + NS_FRAME_IS_OVERFLOW_CONTAINER & nextFrame->GetStateBits()); + NS_ASSERTION(ok, "Failed to steal frame\n"); If we fail to steal the frame, we're going to create a corrupt frame tree, right? Why aren't we at least trying to abort with an error? + NS_FRAME_IS_OVERFLOW_CONTAINER & nextFrame->GetStateBits()); Compare to zero, don't pass a random non-zero number as a boolean. Hmm, actually isn't this always PR_FALSE here? Can we share code for deleting a line box between nsBlockFrame::StealFrame and nsBlockFrame::DoRemoveFrame? + if (endLine != beginLine && endLine->HasFloatBreakAfter()) { Why are you doing this? Couldn't the first line be a <br clear="left"> or whatever? If you're worried about an empty block here, you should compare endLine to end_lines(). + if (mState & NS_FRAME_IS_OVERFLOW_CONTAINER) + return (1 << NS_SIDE_TOP) | (1 << NS_SIDE_BOTTOM); Why not skip left and right sides? In nsBlockFrame::IsChild you probably should search overflow container children. But this function is already too complex for debug-only code; I suggest you just delete it and the assertion that calls it. +#define BRS_ISOVERFLOWCONT 0x00000100 ISOVERFLOWCONTAINER, please + if (BRS_ISOVERFLOWCONT) { Don't you mean mFlags & BRS_ISOVERFLOWCONT(AINER)? + if (0 == aIndex) return nsGkAtoms::overflowList; - } else { - return nsnull; + else if (IsFrameOfType(nsIFrame::eCanContainOverflowContainers)) { + if (1 == aIndex) + return nsGkAtoms::overflowContainersList; + else if (2 == aIndex) + return nsGkAtoms::excessOverflowContainersList; Can we have #defines for these list indices? +nsContainerFrame::PaintOverflowContainers Don't use "Paint" here since this gets called for event handling and other things. DisplayOverflowContainers After reading a lot of GetRemovePropTableFrames calls, I think I'd prefer to have separate Get and Remove functions. + // Delete aFrameList and destroy all its frames + static void Destroy(nsFrameList* aFrameList); Why is this a static method? Why not just "void Destroy();"? Removing nsPresContext* parameters from nsOverflowContinuationTracker did not get addressed. Could StealFrame lose the aFromOverflowContainers parameter and just check the state bit on aChild instead? Seems like StealFrame could be simplified with a helper function that grabs a frame list, tries to remove the child, if it succeeds and the list is empty, deletes the list, or if it succeeds and the list is non-empty, puts the list back. I just need to check the logic in nsOverflowContinuationTracker some more and then I'm about done.

I don't see how f (0xb08c9f8c) is being reached in this loop. DrainOverflowLines should be iterating only over prevBlock's overflow lines, this's normal lines, and this's overflow lines... but f is in a normal line in this's next in flow's next in flow. So how are we finding it?

Plus, we're only finding it if I don't set a breakpoint before we find it. > If the overflow container already exists, can't we just leave it where it is? No, because it's no longer an overflow container. As you suggest, we need to put it in the normal frame list.

Please put the nsComboboxControlFrame changes into a different bug. - if (!aState.mNextInFlow || - (aState.mReflowState.mFlags.mNextInFlowUntouched && + if (aState.mReflowState.mFlags.mNextInFlowUntouched && !lastLineMovedUp && !(GetStateBits() & NS_FRAME_IS_DIRTY) && - !reflowedFloat)) { - if (aState.mNextInFlow) { - aState.mReflowStatus |= NS_FRAME_NOT_COMPLETE; + !reflowedFloat) { + NS_FRAME_SET_INCOMPLETE(aState.mReflowStatus); } - } else { + else if (aState.mNextInFlow) { Doesn't this change mean we might use NS_FRAME_SET_INCOMPLETE on something without a next-in-flow? Oh, and one more thing you should probably test: the interaction of the overflow containers with overflow:hidden and overflow:auto. They're an interesting case for two reasons: one, they have views, and two, they're not splittable.

This testcase hits one of my assertions (the "overflow container must have non-empty overflow rect" one), so I'm going to look into that. Leaving the nsCombobox changes in per discussion on IRC. I've fixed the if statement, I'll attach a new patch once I fix this assert.

Replaced the assert with a comment. It was getting triggered by zero-height getting pushed over due to the tall unsplittable frame.

> Replaced the assert with a comment. It was getting triggered by zero-height > getting pushed over due to the tall unsplittable frame. Would you file a bug on that? I'm pretty sure a zero-height block should always be able to fit. (Not that you'd usually be able to tell the difference, but it might matter in some cases.) I just noticed that your patch doesn't deal with overflow above a frame. Probably not worth worrying about, though, because it's rare. (One example: a block with a border has a child block with a negative margin.)

> Replaced the assert with a comment. It was getting triggered by zero-height > getting pushed over due to the tall unsplittable frame. Would you file a bug on that? I'm pretty sure a zero-height block should always be able to fit. (Not that you'd usually be able to tell the difference, but it might matter in some cases.) I just noticed that your patch doesn't deal with overflow above a frame. Probably not worth worrying about, though, because it's rare. (One example: a block with a border has a child block with a negative margin.)

filed bug 389585

Comment on attachment 273820 [details] [diff] [review] patch I'm satisfied that the patch looks correct and that you've done sufficient testing of the patch. I will note that I found the frame list manipulation kind of confusing, so my review there is not as thorough.

Comment on attachment 273820 [details] [diff] [review] patch + // Delete nsFrameList and destroy all its frames "Delete 'this' and ..." + while (cur && (cur->GetPrevInFlow()->GetStateBits() + & NS_FRAME_IS_OVERFLOW_CONTAINER)) + { Put the brace on the same line as the while otherwise good! Let me just say that I think this approach is ugly, but I strongly doubt a less-ugly approach exists without gigantic changes to Gecko. And I'm impressed that you've even gotten this far.

patch with comment and whitespace changes

checked in. We'll see what the performance results look like.

Thanks for checking this in, roc. And thanks very, very much for the review, I'm sure it was just as grueling for you as it was for me. :P I really appreciate all the help. Eli, thanks for taking the time to review this. Your testcase suggestions caught a lot of bugs. :) For the record, this patch was written on behalf of HP.

This bug's patch seems to cause issues with fixed content showing up on the 2nd page of print-preview -- see bug 389767.