Last Comment Bug 251162 - (multicol) Implement CSS3 columns
(multicol)
: Implement CSS3 columns
Status: VERIFIED FIXED
: css3
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
: 81382 (view as bug list)
Depends on: 254659
Blocks:
  Show dependency treegraph
 
Reported: 2004-07-13 01:13 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-07-01 07:47 PDT (History)
68 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (80.79 KB, image/png)
2004-07-13 01:16 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
patch (28.42 KB, patch)
2004-07-13 01:26 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
Proposal for generating columns beyond element width (2.69 KB, text/plain)
2004-07-14 09:54 PDT, Craig Saila
no flags Details
Support -moz-column-gap (14.79 KB, patch)
2004-07-14 22:12 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
work in progress (35.93 KB, patch)
2004-07-27 23:52 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
columns in print preview (85.14 KB, image/png)
2004-08-21 14:36 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
RTL (106.55 KB, image/png)
2004-08-21 15:33 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
patch representing current status (143.89 KB, patch)
2004-08-21 16:06 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
update to trunk (120.74 KB, patch)
2004-08-27 22:27 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
updated to trunk again (106.33 KB, patch)
2004-09-15 19:48 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
patch for checkin (115.44 KB, patch)
2004-10-07 22:42 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
Basic columns test (2.37 KB, text/html)
2004-10-08 07:53 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
Slashdot mockup (10.10 KB, text/html)
2004-10-08 07:55 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
RTL testcase (8.89 KB, text/html)
2004-10-08 07:57 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 01:13:28 PDT
I have a very rough prototype, but it already works on simple examples.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 01:16:20 PDT
Created attachment 153017 [details]
screenshot

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.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 01:26:45 PDT
Created attachment 153019 [details] [diff] [review]
patch

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.
Comment 3 Robert Accettura [:raccettura] 2004-07-13 12:23:38 PDT
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/
?
Comment 4 Simon Montagu :smontagu 2004-07-13 12:51:27 PDT
You won't be forgetting to flip the columns when direction is rtl, will you? ;-)
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 12:54:33 PDT
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 :-).
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 13:04:32 PDT
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.
Comment 7 Jesse Ruderman 2004-07-13 13:38:16 PDT
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?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 13:44:25 PDT
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.
Comment 9 fantasai 2004-07-13 14:39:02 PDT
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. :)
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 14:43:41 PDT
Floats will float to the left or right within their column.
Comment 11 Jesse Ruderman 2004-07-13 15:09:34 PDT
If the div scrolls horizontally, pages should be specifying the dimensions of
the scrollable area, not the number of columns.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 15:57:26 PDT
Yeah. I proposed an extension to column-count --- '-moz-unlimited' --- for that
reason.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-13 16:19:18 PDT
*** Bug 81382 has been marked as a duplicate of this bug. ***
Comment 14 fantasai 2004-07-14 01:20:18 PDT
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?
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-14 07:41:32 PDT
(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.
Comment 16 fantasai 2004-07-14 08:58:36 PDT
> 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" ?
Comment 17 Craig Saila 2004-07-14 09:51:16 PDT
(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.


Comment 18 Craig Saila 2004-07-14 09:54:43 PDT
Created attachment 153165 [details]
Proposal for generating columns beyond element width

See comment 17
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-14 11:26:50 PDT
> 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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-14 11:32:04 PDT
> 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.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-14 22:12:31 PDT
Created attachment 153257 [details] [diff] [review]
Support -moz-column-gap

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 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-14 22:14:10 PDT
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 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-07-17 18:02:21 PDT
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.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-17 20:35:39 PDT
Yes, the patch was slightly bogus. I forgot to edit out the incorrect version of
nsComputedDOMStyle.cpp, and I forgot to diff dom/. Thanks
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-18 05:12:27 PDT
checked in
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-07-27 23:52:41 PDT
Created attachment 154541 [details] [diff] [review]
work in progress

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.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-08-21 14:36:49 PDT
Created attachment 156700 [details]
columns in print preview

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).
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-08-21 15:33:37 PDT
Created attachment 156704 [details]
RTL

This one's for smontagu :-)
Comment 29 Simon Montagu :smontagu 2004-08-21 15:39:23 PDT
Comment on attachment 156704 [details]
RTL

Yay :)
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-08-21 16:06:14 PDT
Created attachment 156705 [details] [diff] [review]
patch representing current status

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!
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-08-27 22:27:58 PDT
Created attachment 157237 [details] [diff] [review]
update to trunk
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-15 19:48:28 PDT
Created attachment 159052 [details] [diff] [review]
updated to trunk again

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 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-15 19:53:18 PDT
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.
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-15 19:59:59 PDT
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.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-09-28 17:51:04 PDT
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.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-28 18:29:48 PDT
(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.
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-04 14:34:55 PDT
(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.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-04 15:25:06 PDT
I'll do that.
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-04 17:42:26 PDT
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
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-04 20:48:50 PDT
(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 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-05 11:48:16 PDT
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.
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-05 12:50:06 PDT
(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...
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-05 15:49:12 PDT
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.
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-05 19:56:59 PDT
(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!!!
Comment 45 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-05 23:37:41 PDT
(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?
Comment 46 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-06 06:05:57 PDT
(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 47 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-10-06 15:37:40 PDT
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?
Comment 48 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-06 15:48:44 PDT
(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.
Comment 49 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-07 22:42:25 PDT
Created attachment 161444 [details] [diff] [review]
patch for checkin

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.
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-08 05:19:35 PDT
Landed. Thanks!
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-08 07:53:58 PDT
Created attachment 161483 [details]
Basic columns test
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-08 07:55:26 PDT
Created attachment 161484 [details]
Slashdot mockup
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-08 07:57:08 PDT
Created attachment 161485 [details]
RTL testcase
Comment 54 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-10-08 08:00:33 PDT
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.
Comment 55 Greg K. 2005-02-03 06:56:01 PST
Should -moz-column-span work? It doesn’t appear to.
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-06 15:27:46 PST
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.

Note You need to log in before you can comment on or make changes to this bug.