364680 - Column set frames need to compute widths sanely

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Right now, when computing its size a column set frame is basically treated as a non-replaced block container.  As a result it just uses the available width.

However if a block with columns is floated, we end up in nsFrame::ComputeAutoSize, which calls nsFrame::GetMinWidth/GetPrefWidth (since nsColumnSetFrame doesn't implement any of those methods, nor do any of its superclasses).  So we end up with a computed width of 0.

I'll attach a sorta-testcase (sorta because I can't figure out the right magic incantations to get columns to work right even in the non-floated case).

While we're here, we can remove the code to handle unconstrainted computed width in nsColumnSetFrame::ChooseColumnStrategy (probably replace it with an assert).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This testcase is "working". The thing is that specifying a height of 20px means that each column is one line of text, and columns lay out from left to right as many as necessary, so it's hard to tell the difference between that and a regular line layout...

But yeah, the intrinsic width needs to be fixed.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Implement intrinsic widths for column sets. The pref-width is a bit hard to define but what's here is simple and (I think) reasonable.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 255734 [details] [diff] [review]
fix

>+nscoord
>+nsColumnSetFrame::GetPrefWidth(nsIRenderingContext *aRenderingContext) {
>+  const nsStyleColumn* colStyle = GetStyleColumn();
>+  nscoord colGap = 0;
>+  switch (colStyle->mColumnGap.GetUnit()) {
>+    case eStyleUnit_Coord:
>+      colGap = colStyle->mColumnGap.GetCoordValue();
>+      break;
>+    case eStyleUnit_Percent:

The spec doesn't allow percentage column-gap, so we really shouldn't be parsing it -- we should instead change VARIANT_HLP to VARIANT_HL in the CSS parser.

If we do keep supporting it, though, you can handle percentages by increasing the pref width by 1/(1 - total percentage), or making it nscoord_MAX if the total percentage is >= 1.0.

>+  nscoord childPrefWidth = 0;
>+  if (mFrames.FirstChild()) {
>+    childPrefWidth = mFrames.FirstChild()->GetPrefWidth(aRenderingContext);
>+  }

You shouldn't need to call GetPrefWidth on the child if width is specified, so this should be in the else of the if below.

>+  nscoord colWidth = childPrefWidth;
>+  if (colStyle->mColumnWidth.GetUnit() == eStyleUnit_Coord) {
>+    colWidth = PR_MAX(colStyle->mColumnWidth.GetCoordValue(), colWidth);

Why?  If there's a width specified, we will wrap text.  I think this implies that we won't.

>+  }


>+  nscoord prefWidth = colWidth*numColumns + colGap*(numColumns - 1);
>+  // just in case colGap is negative, ensure we return something >= the
>+  // child's pref-width (which is >= our min-width)
>+  return PR_MAX(prefWidth, childPrefWidth); 

The CSS parser should use ParsePositiveVariant for column-gap, so that you can just assert that it's not negative, so that you don't need to do this.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Removes support for % column-gap (and associated testcases). Also updated to the rest of the comments. Also added a couple of reftests.

Comment 6

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 7

[Eli Friedman]

(dbaron, isn't three months a little long to get around to reviewing a small patch like this?)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 258971 [details] [diff] [review]
updated patch

>+nsColumnSetFrame::GetPrefWidth(nsIRenderingContext *aRenderingContext) {
>+  // Our preferred width is the max of our desired column width and the
>+  // child's preferred width, times the number of columns, plus the width of any
>+  // required column gaps

It's not the max anymore -- it's one or the other depending on whether the width is specified.

However, you *do* need to either:
 * max the specified column width (when present) with firstChild.GetMinWidth() in your GetPrefWidth()
 * min the specified column width (when present) with the existing firstChild.GetMinWidth in your GetMinWidth()
so that pref width never yields a result smaller than min-width.  I think the latter is probably correct (unless we use intrinsic sizing during reflow as well to ensure that column widths never go below the contents' min-width).

>+  // XXX what about forced column breaks here?

Could you file a followup bug on these issues?

>+  const nsStyleColumn* colStyle = GetStyleColumn();
>+  nscoord colGap = 0;
>+  switch (colStyle->mColumnGap.GetUnit()) {
>+    case eStyleUnit_Coord:
>+      colGap = colStyle->mColumnGap.GetCoordValue();
>+      break;
>+    case eStyleUnit_Normal:
>+      colGap = GetStyleFont()->mFont.size;
>+      break;
>+    default:
>+      NS_NOTREACHED("Unknown gap type");
>+      break;
>+  }

Could you refactor this and the equivalent chunk in ChooseColumnStrategy into a function?  The ChooseColumnStrategy one also doesn't need the eStyleUnit_Percent bit.

>Index: layout/style/nsCSSParser.cpp
>-    return ParseVariant(aErrorCode, aValue, VARIANT_HLP | VARIANT_NORMAL, nsnull);
>+    return ParseVariant(aErrorCode, aValue, VARIANT_HL | VARIANT_NORMAL, nsnull);

You'll also need to move the "3%" from other_values to invalid_values in layout/style/test/property_database.js.

And could you add, in nsStyleStruct.h, a comment after the mColumnGap line saying "coord, percent" like most other nsStyleCoord values there have?

r+sr=dbaron with those changes

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #8)
> And could you add, in nsStyleStruct.h, a comment after the mColumnGap line
> saying "coord, percent" like most other nsStyleCoord values there have?

Er, now it's just "coord".

Comment 10

[Boris Zbarsky [:bzbarsky]]

Probably also need to fix layout/style/test/test_bug365932.html to not test percentage column gaps.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And you should really have more reftests for the intrinsic width cases -- some for min-width, and tests with and without specified column-widths, with various values of column-gap, and maybe some tests with percentage column-widths (although I'm not sure if the behavior there is sensible yet -- another bug may be needed).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Could you file a followup bug on these issues?

We don't actually support forced column breaks yet, so I'll just leave the min/pref width for that case until we do.

>  * min the specified column width (when present) with the existing
> firstChild.GetMinWidth in your GetMinWidth()

Added that and addressed the other comments.

> maybe some tests with percentage column-widths
> (although I'm not sure if the behavior there is sensible yet -- another bug may
> be needed).

column-width does not support percentages.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Here's the patch updated to comments and with new reftests. I found one issue while putting together the reftests: with column-width:auto, in a min-width situation there will still be column-count columns (assuming my recent message to w3c-css-wg is accepted; currently the spec says we have 1 column when the available width is zero, but column-count columns when available width > 0; I think this discontinuity is stupid). I think this means for column-width:auto the min-width should be column-count*child-min-width. With that change, we still have the guarantee that min-width < pref-width, because when column-width is auto the pref-width is column-count*child-pref-width.

I'll go ahead and check this in.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.