I have a very rough prototype, but it already works on simple examples.

Tada! The markup is like this: <div style="-moz-column-count:3; width:600px; height:200px; border:1px solid green;"> If you remove the width constraint, the DIV gets the width of the window (of course). Watching the columns reflows dynamically as you resize the window is marvellous to behold.

Here's the code. Currently it crashes a lot. I'd like to track down the major crashers and fix them, then see about landing this. I would love to have experimental columns support, even with only the most basic features enabled, in 1.8a3.

Holy smokes that's sweet. Just for my knowledge, are we ultimately looking to implment as per these specs: http://www.w3.org/TR/2001/WD-css3-multicol-20010118/ ?

You won't be forgetting to flip the columns when direction is rtl, will you? ;-)

Yes, I'm following that spec and ultimately we should be able to implement most or all of it. Right now, though, I'm just trying to get the basics right. I am modifying it in one way ... that spec assumes "column balancing", where the browser must try to keep all columns the same height or as near as possible, while the width of the column set is fixed and the columns grow in height to fit the content. We can and will support that, but I also want to support a mode like you see in the screenshot, where the height of the columns is fixed, we flow text into them filling one column before we start the next, and if there's extra content we overflow to the right. This is much easier to implement and also seems like it will give a really nice layout --- see www.iht.com for an example done with Javascript. Simon: good point. I'll get to it. I'm sure you won't let me forget :-).

BTW column balancing is not only tricky, it's also guaranteed to be quite a bit slower to lay out. Another reason why we probably want to avoid it unless the author really needs it.

How will scrolling work with columns? If there's a lot of text, will I have to scroll up to the top after reading all of the first column?

That depends on what markup you use; various kinds of layouts are posssible. The iht.com layout, which I think makes a lot of sense, avoids the long scrolling problem. It looks like this: 1*** 5*** 9*** 13** 17** 2*** 6*** 10** 14** 18** 3*** 7*** 11** 15** 19** 4*** 8*** 12** 16** 20** <============> which is basically what my screenshot is. Which isn't quite allowed by the CSS3 draft, but I hope I can get the CSS WG to provide a way to turn off column balancing to get this layout.

How are you handling floats? If I stick a floating image in the middle of the text for example. (btw, thanks for cc'ing me on this, roc. :)

Floats will float to the left or right within their column.

If the div scrolls horizontally, pages should be specifying the dimensions of the scrollable area, not the number of columns.

Yeah. I proposed an extension to column-count --- '-moz-unlimited' --- for that reason.

*** Bug 81382 has been marked as a duplicate of this bug. ***

Does 'auto' not do what you want? I'm thinking that if the column height is limited, then 'auto' should behave as 'unlimited'. What would -moz-unlimited do if the column height is /not/ limited? wrt floats -- what if the float is wider than the column?

(In reply to comment #14) > Does 'auto' not do what you want? I'm thinking that if the column height is > limited, then 'auto' should behave as 'unlimited'. What would -moz-unlimited > do if the column height is /not/ limited? My understanding was that 'column-count:auto' would just compute the column count by dividing the available width by the column width. In that case, column balancing would still be in effect and if there's too much content for the element height, the element overflows vertically. Authors will definitely want to be able to distinguish between "an unspecified number of balanced columns that overflow vertically and not horizontally" from "an unspecified number of unbalanced columns that overflow horizontally and not vertically". So I think we definitely need a new property or a new property value. > wrt floats -- what if the float is wider than the column? Then it overflows and overlaps the next column. I suppose with some major hacking we might be able to get subsequent columns to flow around any overlap.

> Then it overflows and overlaps the next column. That sounds bad. Even if it's floated right? What happens with "column-count: -moz-unlimited; height: auto" ?

(In reply to comment #15) > My understanding was that 'column-count:auto' would just compute the column > count by dividing the available width by the column width. In that case, column Yes, that's how I read it to (and by "column width" I'm presuming you're including the column rule and column gap). > balancing would still be in effect and if there's too much content for the > element height, the element overflows vertically. The draft seems to suggest height is not applied, in essence with the application of "column-count", height/min-height/max-height all revert to auto, which I agree is strange, and could cause layout problems. I've posted a proposal to www-style, and will post it here as an attachement.

See comment 17

> The draft seems to suggest height is not applied, in essence with the > application of "column-count", height/min-height/max-height all revert to auto, No ... non-auto heights stay the same, but columns can overflow the element vertically.

> Authors will definitely want to be able to distinguish between "an unspecified > number of balanced columns that overflow vertically and not horizontally" from > "an unspecified number of unbalanced columns that overflow horizontally and not > vertically". So I think we definitely need a new property or a new property > value. Hmm, well, I take it back. Maybe we could just say that if the computed height is 'auto', then balancing happens (the first case), otherwise balancing does not happen (the second case). I think both "auto-height with no balancing" and "non-auto height with balancing" don't make sense.

You need column-gap if you're going to do anything that doesn't look embarrassing on screen. So here's a standalone patch that adds -moz-column-gap support to the style system on trunk.

Comment on attachment 153257 [details] [diff] [review] Support -moz-column-gap This support stands alone. I'd like to get this stuff checked in piece by piece to reduce regressions and make the reviews manageable...

Comment on attachment 153257 [details] [diff] [review] Support -moz-column-gap This patch contains nsComputedDOMStyle.cpp changes twice, the second of which looks right, and doesn't contain nsIDOMCSS2Properties.idl changes at all. Given those, r+sr=dbaron.

Yes, the patch was slightly bogus. I forgot to edit out the incorrect version of nsComputedDOMStyle.cpp, and I forgot to diff dom/. Thanks

checked in

This patch adds a whole lot more work. Major developments: -- Builds on some other bug fixes I did on the trunk to fix a few bad bugs. This patch does not crash *too* easily. -- Now I'm trying to handle different reflow types correctly, including incremental reflows. -- I have an implementation of column balancing. There are a couple of hacks, as can be seen from the XXX comments, and performance really sucks right now, but it basically works. It's really cool to go into the editor and watch it rebalance as you add and remove text! I'm doing column balancing based on what I said in comment 20. If you give the block a computed height, we don't do column balancing. If you don't give the block a computed height, we do column balancing. We need to think about what the acceptance criteria should be for landing this stuff.

I know I've been quiet but I've been doing a lot of work on this. I got printing working (technically, the significance is that an nsColumnFrame respects the available height constraint and can manage its own next in flows) and fixed a lot of bugs in incremental reflow and floats in columns. Here's a screenshot of print preview of a columnated DIV containing a float in the second paragraph (and justified text too).

This one's for smontagu :-)

Comment on attachment 156704 [details] RTL Yay :)

I believe things are in pretty good shape. This patch includes everything relevant in my tree. It's a checkpoint, not for review yet; some parts of it were already reviewed by David in another bug and need to be checked in on their own, and I plan to break the rest up into independent bug fixes and check them in piecemeal before the real columns patch lands. However, I hope we can get it all landed as soon as possible!

OK, I've updated it to trunk again after checking in parts of it. The patch has the following parts: -- I add mNextInFlowUntouched to nsHTMLReflowState which keeps track of the situation where the next in flow is not changing. This allows nsBlockFrame::ReflowDirtyLines to avoid the pitfall where we always try to pull one line from the next in flow, only to find that it doesn't fit and push it back, thus achieving nothing but requiring the next in flow to be reflowed, which defeats incremental reflow of just one column. The optimization is: if the next in flow is not changed, and the last line in the current column did not move up and was not dirty, then do not try to pull a line from the next in flow --- we already know it won't fit. NOTE: I wonder if this could be incorrect due to margin changes and margins collapsing through an empty non-dirty line. But it'll do for now. -- In nsBlockFrame::Reflow, move CheckInvalidateSizeChange to after absolute children are accounted for. This is just a bug fix for blocks. -- In nsBlockFrame::ReflowDirtyLines, simplify the code by making line->IsPreviousMarginDirty force line to be marked dirty. Then we don't have to worry about maintaining deltaY in this case. -- In nsBlockFrame::ReflowDirtyLines, check to see if a non-dirty line is going to overflow our height constraint; if it is, mark it dirty, so it gets reflowed and possibly pushed to the overflow lines. Currently non-dirty overflowing lines just get happily slid below the bottom of the column... -- Modify the line-pulling code in nsBlockFrame::ReflowDirtyLines, and the frame-pulling code in nsBlockFrame::PullFrame, and the frame deletion code in DoRemoveFrame, to search the next-in-flow's overflow line list as well as its normal line list. This makes it safe to leave lines on blocks' overflow line lists between reflows, which we need when column balancing. -- When nsBlockFrame::ReflowDirtyLines pulls a line from the block's next in flow, reparent any floats in the line to the current block. (Currently we don't and bad things can happen.) This is unfortunately complex because of the need to handle the cases where the line came from the next-in-flow's normal line list or from the overflow line list separately. -- nsBlockFrame::PullFrameFrom needs to reparent any floats that it drags over from the block's next in flow. -- Various other small fixes that I hope are reasonably obvious. Note that NS_FRAME_REFLOW_NEXTINFLOW is mainly just set in nsBlockFrame::Reflow when we discover that we have overflow lines; all the code paths that go through PushLines will eventually set NS_FRAME_REFLOW_NEXTINFLOW there. -- Don't push lines when we do a trial reflow of a line just to determine its maximum width. This just confuses the heck out of everything. Allow the line to be placed beyond the height constraint, because we're going to reflow it again for real, and we'll just push the line then if necessary. -- In nsBlockFrame::ReflowFloat, don't unconditionally destroy the float's next in flows; allow them to persist unless the float is completed. -- nsCSSFrameConstructor::ConstructBlock is modified to check the column style and construct an nsColumnSetFrame to wrap the block if necessary. -- Finally the real work is the core column code in nsColumnSetFrame. I've renamed this from nsColumnFrame to get a more accurate name. I hope that the code is reasonably easy to understand. The balancing algorithm is in nsColumnSetFrame::Reflow. I hope I've written the code so its operation is pretty obvious; the big picture is that we basically do a binary search for the best height, with some optimizations to speed up common cases. The algorithm for laying out columns with a particular height constraint is encapsulated in a subroutine in nsColumnSetFrame::ReflowChildren. nsColumnSetFrames can have continuations (for nested columns and columns in paginated layouts); column sets manage continuations and their columns in a very similar way to the way blocks manage their continuations and their lines, although mercifully it's a lot simpler for columns. Note that nsColumnSetFrame delegates GetContentInsertionFrame to its first child, so inserted and removed frames are all delegated to its block children. Note that currently image frames and especially table frames don't break in galley contexts, not even if they have a height constraint --- so they don't break in columns. We probably should at least allow table frames to break across columns but for now I'm glad we're not going to exercise that scary code. That's about it!

Comment on attachment 159052 [details] [diff] [review] updated to trunk again Okay, right now I think the best way to make progress is to land this beast.

One other thing I should mention is that I've mostly tested with a fixed column-count. Fixing column-width and allowing column-count to change dynamically probably has issues, but they should be easy to fix.

Some comments, up to the point in the last comment: nsStyleStruct.cpp: Maybe add "XXX", since this doesn't seem ideal. nsHTMLReflowState.h: Change mUnused from 12 to 11. nsBlockFrame.cpp: - nsIFrame* nextInFlow = GetNextInFlow(); - if (nextInFlow && NS_FRAME_IS_NOT_COMPLETE(state.mReflowStatus)) { + if (NS_FRAME_IS_NOT_COMPLETE(state.mReflowStatus)) { This chunk of code could use a comment -- and perhaps even one explaining why you don't want the check that you're removing here. Were the *PreviousMargin* changes in nsBlockFrame::ReflowDirtyLines part of another patch? If not, I need to come back to them.

(In reply to comment #35) > nsStyleStruct.cpp: > Maybe add "XXX", since this doesn't seem ideal. Sure > nsHTMLReflowState.h: > Change mUnused from 12 to 11. Right. Why do we even have it? > nsBlockFrame.cpp: > > - nsIFrame* nextInFlow = GetNextInFlow(); > - if (nextInFlow && NS_FRAME_IS_NOT_COMPLETE(state.mReflowStatus)) { > + if (NS_FRAME_IS_NOT_COMPLETE(state.mReflowStatus)) { > > This chunk of code could use a comment -- and perhaps even one explaining > why you don't want the check that you're removing here. // If we pushed anything to our next in flow then we'll need to make sure it // gets reflowed Actually there can't be overflow placeholders if there are no overflow lines. So I think I'll change it to if (NS_FRAME_IS_NOT_COMPLETE(state.mReflowStatus)) { if (GetOverflowLines()) { state.mReflowStatus |= NS_FRAME_REFLOW_NEXTINFLOW; } else { NS_ASSERTION(!GetOverflowPlaceholders(), "Cannot have overflow placeholders without overflow lines"); } } > Were the *PreviousMargin* changes in nsBlockFrame::ReflowDirtyLines > part of another patch? If not, I need to come back to them. No. They overlap with another patch that you've seen and that I checked in: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsBlockFrame.cpp&branch=&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&rev1=3.660&rev2=3.661 but they're consistent with it. Both patches convert an IsPreviousMarginDirty() into an IsDirty(). This patch adds an additional clause: + } else if (line->mBounds.YMost() + deltaY + > aState.mReflowState.availableHeight) { + // Lines that aren't dirty but get slid past our height constraint must + // be reflowed. + line->MarkDirty(); to reflow lines that might otherwise be slid beyond the available height. It also pulls the following "if (!line->IsDirty())" into the else clause for a minor efficiency gain.

(In reply to comment #36) > (In reply to comment #35) > > nsHTMLReflowState.h: > > Change mUnused from 12 to 11. > > Right. Why do we even have it? Getting rid of it is fine too.

I'll do that.

nsBlockFrame.cpp: > Actually there can't be overflow placeholders if there are no overflow > lines. So I think I'll change it to Sounds fine with me. Although I don't exactly understand how that corresponds to the code. (I thought that was the way the code worked, and I assumed it led to a bunch of bugs, but the bugs I expected aren't present -- in particular, we can carry floats to the next page and have later text not carried to that next page, which surprised me.) There's no need for the |canSkipPull| variable -- just put the expression inside the if. I'd prefer the arguments to ReparentFrame to be in the order old,new. That's consistent with ReparentFrameView and probably other things. + } else { + nsLineList* lineList = RemoveOverflowLines(); + line = lineList->erase(line); + if (line != lineList->end()) { + SetOverflowLines(lineList); + } + } This (in particular, the (line != lineList->end()) test) seems wrong. It says that if the frame you're removing is the only frame in the *last* line of the overflow lines, then forget about *all* the overflow lines I'm up to pretty much the last few changes in nsBlockFrame.cpp

(In reply to comment #39) > nsBlockFrame.cpp: > > > Actually there can't be overflow placeholders if there are no overflow > > lines. So I think I'll change it to > > Sounds fine with me. Although I don't exactly understand how that > corresponds to the code. (I thought that was the way the code worked, > and I assumed it led to a bunch of bugs, but the bugs I expected aren't > present -- in particular, we can carry floats to the next page and have > later text not carried to that next page, which surprised me.) I think below-current-line floats can do that. I don't know exactly what causes a float to become a below-current-line float because I haven't looked deeply at float layout and placement yet. Actually you're right, I was too hasty. I was thinking of the code that follows immediately in nsBlockFrame::Reflow, which puts any overflow placeholders onto a new line at the start of the overflow line list. So I should not only change the code to if (NS_FRAME_IS_NOT_COMPLETE(state.mReflowStatus)) { if (GetOverflowLines()) { state.mReflowStatus |= NS_FRAME_REFLOW_NEXTINFLOW; } else { NS_ASSERTION(!GetOverflowPlaceholders(), "Cannot have overflow placeholders without overflow lines"); } } but also move it to after the following 'if' block, which moves the placeholders to the overflow line list. Then I can remove the assertion since it's obvious GetOverflowPlaceholders will return nothing. > There's no need for the |canSkipPull| variable -- just put the expression > inside the if. OK > I'd prefer the arguments to ReparentFrame to be in the order old,new. > That's consistent with ReparentFrameView and probably other things. Sure > + } else { > + nsLineList* lineList = RemoveOverflowLines(); > + line = lineList->erase(line); > + if (line != lineList->end()) { > + SetOverflowLines(lineList); > + } > + } > > This (in particular, the (line != lineList->end()) test) seems wrong. It > says that if the frame you're removing is the only frame in the > *last* line of the overflow lines, then forget about *all* the > overflow lines Crikey! Yes, it should be "if (!lineList->empty())" > I'm up to pretty much the last few changes in nsBlockFrame.cpp Great! Thanks so much.

Comment on attachment 159052 [details] [diff] [review] updated to trunk again >Index: layout/html/base/src/nsHTMLReflowState.cpp >+ nsIFrame* parentNext = aParent->GetNextInFlow(); Double-space is odd. >@@ -271,12 +284,13 @@ nsHTMLReflowState::nsHTMLReflowState(nsP > availableHeight = aAvailableSpace.height; > > rendContext = aParentReflowState.rendContext; > mSpaceManager = aParentReflowState.mSpaceManager; > mLineLayout = aParentReflowState.mLineLayout; > mFlags.mIsTopOfPage = aParentReflowState.mFlags.mIsTopOfPage; >+ mFlags.mNextInFlowUntouched = PR_FALSE; Should this one be like the third and fourth? >Index: layout/html/style/src/nsCSSFrameConstructor.h >- nsIFrame* aNewFrame, >+ nsIFrame** aNewFrame, Comment that it's inout, since that's odd.

(In reply to comment #41) > >@@ -271,12 +284,13 @@ nsHTMLReflowState::nsHTMLReflowState(nsP > > availableHeight = aAvailableSpace.height; > > > > rendContext = aParentReflowState.rendContext; > > mSpaceManager = aParentReflowState.mSpaceManager; > > mLineLayout = aParentReflowState.mLineLayout; > > mFlags.mIsTopOfPage = aParentReflowState.mFlags.mIsTopOfPage; > >+ mFlags.mNextInFlowUntouched = PR_FALSE; > > Should this one be like the third and fourth? Good catch. Thanks...

Index: layout/html/base/src/nsColumnSetFrame.cpp + * The Initial Developer of the Original Code is + * Netscape Communications Corporation. + * Portions created by the Initial Developer are Copyright (C) 1998 + * the Initial Developer. All Rights Reserved. This seems wrong. + * + * Contributor(s): + * roc@ocallahan.org How about a full name? +#include "nsCSSRendering.h" +#include "nsHTMLAtoms.h" +#include "nsIServiceManager.h" At least these seem extraneous. + virtual nsIFrame* GetContentInsertionFrame() { + return GetFirstChild(nsnull)->GetContentInsertionFrame(); + } I prefer out-of-line, since it's never going to be called inline. +nsresult +NS_NewColumnSetFrame(nsIPresShell* aPresShell, nsIFrame** aNewFrame, PRUint32 aStateFlags ) Extra space at end. +{ + NS_PRECONDITION(aNewFrame, "null OUT ptr"); + if (nsnull == aNewFrame) { + return NS_ERROR_NULL_POINTER; + } The null-check here should just be an assertion, no runtime check in release builds. +NS_IMETHODIMP +nsColumnSetFrame::SetInitialChildList(nsPresContext* aPresContext, + nsIAtom* aListName, + nsIFrame* aChildList) +{ + NS_ASSERTION(!aListName, "Only normal children supported"); How about "only default child list supported" + NS_ASSERTION(aChildList && !aChildList->GetNextSibling(), + "must set to a singleton initial child list"); Singleton is the wrong word, I think. How about "must have one child". + // Queue up the frames for the content frame + return nsHTMLContainerFrame::SetInitialChildList(aPresContext, nsnull, aChildList); +} Regarding the reflow code, which I didn't read too closely: Did you test what happens when you add margin, padding, or border (probably best to test each side separately) to both the element with the columns and to the ::-moz-column-content pseudo? If not, it's worth doing. (The testing on the element itself hopefully shouldn't be an issue, but testing on the pseudo might show some problems.) +nsColumnSetFrame::ReflowConfig +nsColumnSetFrame::ChooseColumnStrategy(const nsHTMLReflowState& aReflowState) +{ + const nsStyleColumn* colStyle = GetStyleColumn(); + nscoord availWidth = aReflowState.availableWidth; + if (aReflowState.mComputedWidth != NS_INTRINSICSIZE) { + availWidth = aReflowState.mComputedWidth; + } At least according to the comments in nsHTMLReflowState.h, this seems wrong, since mComputedWidth doesn't include border/padding but availableWidth does. + nscoord colHeight = GetAvailableContentHeight(aReflowState); + if (aReflowState.mComputedHeight != NS_INTRINSICSIZE) { + colHeight = aReflowState.mComputedHeight; + } + nscoord colWidth = NS_INTRINSICSIZE; + if (colStyle->mColumnWidth.GetUnit() == eStyleUnit_Coord) { + colWidth = colStyle->mColumnWidth.GetCoordValue(); This doesn't enforce the spec's statement that the column width can't be bigger than the available width. + } else if (colStyle->mColumnCount > 0 && availWidth != NS_INTRINSICSIZE) { + nscoord widthMinusGaps = availWidth - colGap*(colStyle->mColumnCount - 1); + colWidth = widthMinusGaps/colStyle->mColumnCount; + } This if-else makes me notice that this doesn't match the spec for what happens when both 'column-count' and 'column-width' are specified. Given that we don't implement 'column-width-policy' (and thus 'column-space-distribution'), what we should do according to the spec (as I read it) is, when both are specified, use the lower number of columns that we'd get from the two of them. So this makes me notice you're using 0 and -1 as magic values for column count. First of all, the spec doesn't say that they're invalid. It probably should, and we should send that comment to the WG. (And likewise for column-width not being negative -- which our parsing code does allow.) Second, the parser doesn't enforce that they won't come from a stylesheet -- for two reasons. One, ParsePositiveVariant should really be called ParseNonNegativeVariant, and second, it's not set up to handle integers -- only floats and percents. + if (colWidth < 1) { + colWidth = 1; + } + + PRInt32 numColumns = colStyle->mColumnCount; You could use this variable earlier to clean up a bunch of uses of colStyle->mColumnCount. + if (aReflowState.mComputedHeight == NS_INTRINSICSIZE) { It's worth noting that this stuff isn't in the spec. I just have to review ReflowChildren, DrainOverflowColumns, and Reflow.

(In reply to comment #43) > Index: layout/html/base/src/nsColumnSetFrame.cpp > + * The Initial Developer of the Original Code is > + * Netscape Communications Corporation. > + * Portions created by the Initial Developer are Copyright (C) 1998 > + * the Initial Developer. All Rights Reserved. > > This seems wrong. I started by copying and pasting the code of some other frame implementation ... I forget which one though. I think legally that means Netscape was the original developer. > + * > + * Contributor(s): > + * roc@ocallahan.org > > How about a full name? OK :-) > +#include "nsCSSRendering.h" > +#include "nsHTMLAtoms.h" > +#include "nsIServiceManager.h" > > At least these seem extraneous. OK > + virtual nsIFrame* GetContentInsertionFrame() { > + return GetFirstChild(nsnull)->GetContentInsertionFrame(); > + } > > I prefer out-of-line, since it's never going to be called inline. I prefer inline since it's less code ... but I'll yield. > +nsresult > +NS_NewColumnSetFrame(nsIPresShell* aPresShell, nsIFrame** aNewFrame, PRUint32 > aStateFlags ) OK > Extra space at end. > > +{ > + NS_PRECONDITION(aNewFrame, "null OUT ptr"); > + if (nsnull == aNewFrame) { > + return NS_ERROR_NULL_POINTER; > + } > > The null-check here should just be an assertion, no runtime check in release > builds. OK > +NS_IMETHODIMP > +nsColumnSetFrame::SetInitialChildList(nsPresContext* aPresContext, > + nsIAtom* aListName, > + nsIFrame* aChildList) > +{ > + NS_ASSERTION(!aListName, "Only normal children supported"); > > How about "only default child list supported" OK > + NS_ASSERTION(aChildList && !aChildList->GetNextSibling(), > + "must set to a singleton initial child list"); > > Singleton is the wrong word, I think. How about "must have one child". Sure > Regarding the reflow code, which I didn't read too closely: > > Did you test what happens when you add margin, padding, or border > (probably best to test each side separately) to both the element with > the columns and to the ::-moz-column-content pseudo? If not, it's worth > doing. (The testing on the element itself hopefully shouldn't be an > issue, but testing on the pseudo might show some problems.) No. That's a good idea, I guess I should. But how about I just remove the pseudo for now and we add it back in later as a separate patch? I don't even have a use for it right now. > +nsColumnSetFrame::ReflowConfig > +nsColumnSetFrame::ChooseColumnStrategy(const nsHTMLReflowState& aReflowState) > +{ > + const nsStyleColumn* colStyle = GetStyleColumn(); > + nscoord availWidth = aReflowState.availableWidth; > + if (aReflowState.mComputedWidth != NS_INTRINSICSIZE) { > + availWidth = aReflowState.mComputedWidth; > + } > > At least according to the comments in nsHTMLReflowState.h, this seems > wrong, since mComputedWidth doesn't include border/padding but > availableWidth does. Right. I'll change the name to availContentWidth and subtract the border/padding from aReflowState.availableWidth. > + nscoord colWidth = NS_INTRINSICSIZE; > + if (colStyle->mColumnWidth.GetUnit() == eStyleUnit_Coord) { > + colWidth = colStyle->mColumnWidth.GetCoordValue(); > > This doesn't enforce the spec's statement that the column width can't > be bigger than the available width. Right, OK, I'll need to retool this code a bit. > + } else if (colStyle->mColumnCount > 0 && availWidth != NS_INTRINSICSIZE) { > + nscoord widthMinusGaps = availWidth - colGap*(colStyle->mColumnCount - 1); > + colWidth = widthMinusGaps/colStyle->mColumnCount; > + } > > This if-else makes me notice that this doesn't match the spec for what > happens when both 'column-count' and 'column-width' are specified. > Given that we don't implement 'column-width-policy' (and thus > 'column-space-distribution'), what we should do according to the spec > (as I read it) is, when both are specified, use the lower number of > columns that we'd get from the two of them. My reading of the 'column-count' and 'column-width' properties is that we first reduce the number of columns until we reach 1 or the columns fit in the available width. If that number of columns fits, then we assume column-width-policy:flexible and divide surplus width among the colums. If even 1 column doesn't fit, then we reduce its width to the available width. > So this makes me notice you're using 0 and -1 as magic values for > column count. First of all, the spec doesn't say that they're invalid. > It probably should, and we should send that comment to the WG. (And > likewise for column-width not being negative -- which our parsing code > does allow.) It does say "The minimum actual value of 'column-count' is 1." So I think we can at least adjust the style value up to 1 before we start using it. > Second, the parser doesn't enforce that they won't come from a > stylesheet -- for two reasons. One, ParsePositiveVariant should really > be called ParseNonNegativeVariant, and second, it's not set up to handle > integers -- only floats and percents. What do you think we should do about that? > + if (colWidth < 1) { > + colWidth = 1; > + } > + > + PRInt32 numColumns = colStyle->mColumnCount; > > You could use this variable earlier to clean up a bunch of uses of > colStyle->mColumnCount. Sure > + if (aReflowState.mComputedHeight == NS_INTRINSICSIZE) { > > It's worth noting that this stuff isn't in the spec. OK. > I just have to review ReflowChildren, DrainOverflowColumns, and Reflow. Great!!!

(In reply to comment #44) > > + virtual nsIFrame* GetContentInsertionFrame() { > > + return GetFirstChild(nsnull)->GetContentInsertionFrame(); > > + } > > > > I prefer out-of-line, since it's never going to be called inline. > > I prefer inline since it's less code ... but I'll yield. Is it really less code? That's bizarre... > > Did you test what happens when you add margin, padding, or border > > (probably best to test each side separately) to both the element with > > the columns and to the ::-moz-column-content pseudo? If not, it's worth > > doing. (The testing on the element itself hopefully shouldn't be an > > issue, but testing on the pseudo might show some problems.) > > No. That's a good idea, I guess I should. But how about I just remove the pseudo > for now and we add it back in later as a separate patch? I don't even have a use > for it right now. Leaving it now is fine as long as you remember to test that at some point. It may show some bugs (like one of the ones I pointed out). > My reading of the 'column-count' and 'column-width' properties is that we first > reduce the number of columns until we reach 1 or the columns fit in the > available width. If that number of columns fits, then we assume > column-width-policy:flexible and divide surplus width among the colums. If even > 1 column doesn't fit, then we reduce its width to the available width. Right. But given, in a 1000px wide container, 'column-width: 800px; column-count: 2', this makes it seem like you'll end up with two columns instead of 1. > > So this makes me notice you're using 0 and -1 as magic values for > > column count. First of all, the spec doesn't say that they're invalid. > > It probably should, and we should send that comment to the WG. (And > > likewise for column-width not being negative -- which our parsing code > > does allow.) > > It does say "The minimum actual value of 'column-count' is 1." So I think we > can at least adjust the style value up to 1 before we start using it. But it should probably be a parsing error rather than something that's corrected later on. > > Second, the parser doesn't enforce that they won't come from a > > stylesheet -- for two reasons. One, ParsePositiveVariant should really > > be called ParseNonNegativeVariant, and second, it's not set up to handle > > integers -- only floats and percents. > > What do you think we should do about that? Make ParsePositiveVariant handle integers -- perhaps even handle them as truly positive rather than nonnegative -- or otherwise just check for 0?

(In reply to comment #45) > (In reply to comment #44) > > > I prefer out-of-line, since it's never going to be called inline. > > > > I prefer inline since it's less code ... but I'll yield. > > Is it really less code? That's bizarre... I mean it's less code to write because you don't have to repeat the prototype. > > No. That's a good idea, I guess I should. But how about I just remove the > > pseudo for now and we add it back in later as a separate patch? I don't even > > have a use for it right now. > > Leaving it now is fine as long as you remember to test that at some point. It > may show some bugs (like one of the ones I pointed out). OK. It shouldn't be too hard to fix any problems that show up, anyway. > Right. But given, in a 1000px wide container, 'column-width: 800px; > column-count: 2', this makes it seem like you'll end up with two columns > instead of 1. Yes, the current code is incorrect, I didn't mean to imply otherwise. I'll fix it. > > It does say "The minimum actual value of 'column-count' is 1." So I think > > we can at least adjust the style value up to 1 before we start using it. > > But it should probably be a parsing error rather than something that's > corrected later on. OK. I'll have a go at hacking that into the style system. > > > Second, the parser doesn't enforce that they won't come from a > > > stylesheet -- for two reasons. One, ParsePositiveVariant should really > > > be called ParseNonNegativeVariant, and second, it's not set up to handle > > > integers -- only floats and percents. > > > > What do you think we should do about that? > > Make ParsePositiveVariant handle integers -- perhaps even handle them as truly > positive rather than nonnegative -- or otherwise just check for 0? Alright, I'll try modifying the style system.

Comment on attachment 159052 [details] [diff] [review] updated to trunk again I'm not going to get through Reflow and ReflowChildren in any reasonable amount of time, and I need to move on, so r+sr=dbaron with the comments above. One thought, though: Is there a mechanism for preventing columns from shrinking below the "max-element-width" of their content? That might be a good thing -- at least when there's more than one column (so that it could potentially reduce the number of columns). Or do things currently just overflow?

(In reply to comment #47) > (From update of attachment 159052 [details] [diff] [review]) > I'm not going to get through Reflow and ReflowChildren in any reasonable > amount of time, and I need to move on, so r+sr=dbaron with the comments above. Thanks!!! > One thought, though: Is there a mechanism for preventing columns from > shrinking below the "max-element-width" of their content? That might be a > good thing -- at least when there's more than one column (so that it could > potentially reduce the number of columns). Or do things currently just > overflow? They overflow. That's a good idea and could be useful because I forsee overflow as a problem. Any such mechanism would violate the draft, I suppose. Maybe we should implement it as a -moz- extension.

This is updated to trunk and incorporates all the fixes for the review comments. I modified the style system to convert ParsePositiveVariant to ParseBoundedBelowNPILVariant and use it appropriately. Also, I've tested the ::-moz-column-content pseudo and it mostly works. It did expose a bug in block reflow which I think boils down to this: if the last line of a block has a bottom margin, and the block has borders or padding, and we reflow the block with a constrained height, then we may decide that the line fits (because its YMost is < aState.mBottomEdge (which is aState.availableHeight - borderpadding)) but when computing the final size for the block, applying the carried-out margin pushes the block height beyond aState.mBottomEdge, so the block ends up returning a desired height greater than the available height [which should enver happen and confuses the column balancer]. We really need to detect this situation before it happens and declare that the last line does not fit. I don't want to try to fix this before checking this beast in, so I'll check this patch in as-is tomorrow.

Landed. Thanks!

This work is experimental and stresses Gecko in exciting new ways, so I anticipate we'll encounter a lot of bugs as we start playing with more complex examples. Please do not spam this bug with such comments; file new bugs against me and mention "columns" in the summary.

Should -moz-column-span work? It doesn’t appear to.

No. The 'column-span' property is underspecified, difficult to implement, and IMHO will never be implemented by anyone. Hopefully we'll change the spec to provide some more reasonable way to do spanning. My personal preference is to have the content of a column flow around floats in previous columns that are overflowing horizontally to the right.