Closed Bug 364680 Opened 13 years ago Closed 13 years ago

Column set frames need to compute widths sanely

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: bzbarsky, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

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).
Flags: blocking1.9?
Keywords: regression
Attached file Testcase
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.
Attached patch fix (obsolete) — Splinter Review
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #255734 - Flags: superreview?(dbaron)
Attachment #255734 - Flags: review?(dbaron)
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.
Attachment #255734 - Flags: superreview?(dbaron)
Attachment #255734 - Flags: superreview-
Attachment #255734 - Flags: review?(dbaron)
Attachment #255734 - Flags: review-
Attached patch updated patch (obsolete) — Splinter Review
Removes support for % column-gap (and associated testcases). Also updated to the rest of the comments. Also added a couple of reftests.
Attachment #255734 - Attachment is obsolete: true
Attachment #258971 - Flags: superreview?(dbaron)
Attachment #258971 - Flags: review?(dbaron)
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9alpha6
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
(dbaron, isn't three months a little long to get around to reviewing a small patch like this?)
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
Attachment #258971 - Flags: superreview?(dbaron)
Attachment #258971 - Flags: superreview+
Attachment #258971 - Flags: review?(dbaron)
Attachment #258971 - Flags: review+
(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".
Probably also need to fix layout/style/test/test_bug365932.html to not test percentage column gaps.
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).
> 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.
Attached patch updated patchSplinter Review
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.
Attachment #258971 - Attachment is obsolete: true
checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 553835
You need to log in before you can comment on or make changes to this bug.