271586 - Implement CSS3 column-rule-*

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jürgen Herz]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.8a5) Gecko/20041124
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.8a5) Gecko/20041124

After the basic column property and even column-gap has been implemented as
preliminary -moz..., column-rule-* would be great.

Reproducible: Always
Steps to Reproduce:

Comment 1

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

Attachment: part 1: add style properties to the style system

Comment 2

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

Comment on attachment 167808 [details] [diff] [review]
part 1: add style properties to the style system

oops, that was missing the DOM IDL changes

Comment 3

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

Attachment: part 1: style system changes to add -moz-column-rule-width, -moz-column-rule-style, and -moz-column-rule-color properties, with -moz-column-rule shorthand

Comment 4

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

Attachment: part 2: implement column-rule rendering in nsCSSRendering

To get this to work I did some refactoring. I removed the unused
DrawDashedSegments and PaintBorderEdges methods and nsBorderEdges type. I
collapsed the two DrawDashedSides methods into one simpler method that takes
detailed per-side instructions in the DashedSide array.

I draw the column-rule basically by just drawing the left side of a
hypothetical box border. This works well except for the styles that depend on
which side you're talking about: 'groove', 'inset', 'outset' etc. But the
rendering of those styles in column-rules is really ambiguous anyway. I'll
raise it with the CSS WG.

Comment 5

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

Attachment: part 3: apply column-rule in nsColumnSetFrame layout and rendering

According to the spec we have to add the column-rule width to the column-gap,
so it affects layout. I've done that.

Comment 6

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

Note that the patches can be applied separately, but they must be applied in the
given order.

Comment 7

[Martijn Wargers (dead)]

Well, I applied the three patches and tried to rebuild, but I get a build error,
see here:
http://martijn.heelveel.info/test/mozilla/build_error.txt
Maybe I'm doing something wrong?

Comment 8

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

Dunno. I'll refresh the patches --- they have to be updated for the layout file
reorg, anyway.

Comment 9

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

Comment on attachment 167809 [details] [diff] [review]
part 1: style system changes to add -moz-column-rule-width, -moz-column-rule-style, and -moz-column-rule-color properties, with -moz-column-rule shorthand


Please patch nsStyleContext::DumpRegressionData.

You seem to be using something different for representing the column rule color
being the foreground color.  This means we now have three ways of doing that
(one for 'currentColor', one for borders, and one for this).  The
'currentColor' one doesn't share as well in the rule tree, but it seems like a
little much.  It seems better to do this the same way as one of the others, at
least.

>@@ -501,16 +502,17 @@ nsCSSDeclaration::GetValue(nsCSSProperty
>   // XXXldb Can we share shorthand logic with ToString?
>   switch (aProperty) {
>     case eCSSProperty_margin: 
>     case eCSSProperty_padding: 
>     case eCSSProperty_border_color: 
>     case eCSSProperty_border_style: 
>     case eCSSProperty__moz_border_radius: 
>     case eCSSProperty__moz_outline_radius: 
>+    case eCSSProperty__moz_column_rule_width: 
>     case eCSSProperty_border_width: {
>       const nsCSSProperty* subprops =
>         nsCSSProps::SubpropertyEntryFor(aProperty);

This doesn't make sense -- this is for rectangular shorthand properties.  (It's
unreachable, though, but pointless.)

>@@ -479,16 +479,28 @@ interface nsIDOMNSCSS2Properties : nsIDO

rev IID.

Comment 10

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

Comment on attachment 167809 [details] [diff] [review]
part 1: style system changes to add -moz-column-rule-width, -moz-column-rule-style, and -moz-column-rule-color properties, with -moz-column-rule shorthand

Also, you should probably modify nsCSSDeclaration::ToString for shorthand
output.

Comment 11

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

(In reply to comment #4)
> hypothetical box border. This works well except for the styles that depend on
> which side you're talking about: 'groove', 'inset', 'outset' etc. But the
> rendering of those styles in column-rules is really ambiguous anyway. I'll
> raise it with the CSS WG.

I think these (excluding 'groove', which is fine) should probably be treated
either like they are for border-collapse tables or the reverse way.  (It was
specified backwards and interoperably implemented backwards.)  i.e., inset and
outset should be mapped to groove and ridge, one way around or the other.

Comment 12

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

Comment on attachment 167810 [details] [diff] [review]
part 2: implement column-rule rendering in nsCSSRendering

Rubber-stamp r+sr on this modulo my previous comment and emailing the wg to ask
about it.  (There's actually a chance we corrected the spec for the
border-collapse case, but I don't think so.  Then again, if Mozilla fixes the
code to be the right way round, there's a better chance we would correct the
spec...)

Comment 13

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

Comment on attachment 167811 [details] [diff] [review]
part 3: apply column-rule in nsColumnSetFrame layout and rendering

ChooseColumnStrategy could use a variable that's the sum of colRuleWidth and
colGap (does it ever need them separately?), but other than that, r+sr=dbaron.

Comment 14

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

Comment on attachment 167809 [details] [diff] [review]
part 1: style system changes to add -moz-column-rule-width, -moz-column-rule-style, and -moz-column-rule-color properties, with -moz-column-rule shorthand

r+sr=dbaron given the changes in comment 9, although I might want to look again
if you change the way the foreground color thing works

Comment 15

[Anne (:annevk)]

Why hasn't this been checked in?

Comment 16

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

The patch has got to be updated and there are other higher priority items right now

Comment 17

[Felix Miata]

Sure is a shame this missed 1.8. Can we officially target 1.9?

Comment 18

[Thomas Schwecherl]

Robert, will column-rule-* be in 1.9?

Comment 19

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

Not sure. Persuade me that it's important :-)

Comment 20

[Michael Ventnor]

Taking. I have a very, very small rendering patch (using an awesome trick I thought up) and may be able to re-use the already-reviewed parsing patch.

Comment 21

[Michael Ventnor]

Attachment: Parsing patch

This pretty much brings part 1 up to date, so I'm not sure whether it needs another review (it would be nice to avoid waiting for dbaron to finish his vacation).

Comment 22

[Michael Ventnor]

Attachment: Rendering patch

Good news roc, you have the easy part. This strategy works extremely well in all cases that I've tried it with, and saves an absolute boatload of code (as you could probably tell with the sheer smallness of the patch).

Comment 23

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

+  const nsStyleColumn* colStyle = GetStyleColumn();
+  nscolor ruleColor;
+  if (colStyle->mColumnRuleColorIsForeground) {
+    ruleColor = GetStyleColor()->mColor;
+  } else {
+    ruleColor = colStyle->mColumnRuleColor;
+  }

Why not just put all this in nsDisplayColumnRule?

+  aLists.Content()->AppendNewToTop(new (aBuilder)
+      nsDisplayColumnRule(this, &mFrames, colStyle->CalcColumnRuleWidth(PresContext()),
+                          ruleColor, colStyle->mColumnRuleStyle));

I don't think you need any of these parameters, they can all be obtained from the frame. Use GetFirstChild(nsnull). Then you could use nsDisplayGeneric instead of writing your own class.

+      // columns go RTL in RTL, as you would hope...
+      nsPoint leftOfPrevColumn = child->GetRect().TopLeft() + thisPt;
+      nsPoint rightOfNextColumn = nextSibling->GetRect().TopRight() + thisPt;
+      linePt = nsPoint((leftOfPrevColumn.x + rightOfNextColumn.x) / 2,
+                       leftOfPrevColumn.y);
+    } else {
+      nsPoint rightOfPrevColumn = child->GetRect().TopRight() + thisPt;
+      nsPoint leftOfNextColumn = nextSibling->GetRect().TopLeft() + thisPt;
+      linePt = nsPoint((rightOfPrevColumn.x + leftOfNextColumn.x) / 2,
+                       rightOfPrevColumn.y);

You can simplify this by defining nsIFrame* leftFrame = child; nsIFrame* rightFrame = nextSibling; and then swapping them for RTL.

Nice trick with the border.

Comment 24

[Michael Ventnor]

Attachment: Rendering patch 2

Fix comments.

Comment 25

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

+    nsPoint linePt = nsPoint((edgeOfLeftSibling.x + edgeOfRightSibling.x - ruleWidth) / 2,
+                             edgeOfLeftSibling.y);

nsPoint linePt(..., ...);

+    nsRect lineRect = nsRect(linePt, nsSize(ruleWidth, minimumHeight));
+

nsRect lineRect(..., ...);

The three calls to PresContext() should be shared via a local variable.

+  aLists.Content()->AppendNewToTop(new (aBuilder)
+      nsDisplayGeneric(this, ::PaintColumnRule, "ColumnRule"));

I think this should probably be the BorderBackground list, not Content, so that the rule paints just above the background of the column set. I believe that with the current code, the rule will paint over the top of the backgrounds of block children of the column set, but I think it should paint under them. The spec is silent and needs to be clarified. You should write a test for this and also check what Webkit does.

Comment 26

[Michael Ventnor]

Attachment: Rendering patch 3

Addresses comments.

Comment 27

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

Comment on attachment 326643 [details] [diff] [review]
Parsing patch

>diff -r 2cbe07bd5857 dom/public/idl/css/nsIDOMCSS2Properties.idl
>--- a/dom/public/idl/css/nsIDOMCSS2Properties.idl	Tue Jun 24 19:52:40 2008 +1200
>+++ b/dom/public/idl/css/nsIDOMCSS2Properties.idl	Wed Jun 25 17:53:23 2008 +1000
>@@ -484,6 +484,18 @@ interface nsIDOMNSCSS2Properties : nsIDO

Please rev the IID and add to the end of the interface.

>diff -r 2cbe07bd5857 layout/style/nsCSSDataBlock.cpp
>--- a/layout/style/nsCSSDataBlock.cpp	Tue Jun 24 19:52:40 2008 +1200
>+++ b/layout/style/nsCSSDataBlock.cpp	Wed Jun 25 17:53:23 2008 +1000
>@@ -227,6 +227,7 @@ nsCSSCompressedDataBlock::MapRuleInfoInt
>                                  iProp == eCSSProperty_border_left_color_value ||
>                                  iProp == eCSSProperty_border_left_color_ltr_source ||
>                                  iProp == eCSSProperty_border_left_color_rtl_source ||
>+                                 iProp == eCSSProperty__moz_column_rule_color ||
>                                  iProp == eCSSProperty_outline_color) {
>                             if (ShouldIgnoreColors(aRuleData)) {
>                                 if (iProp == eCSSProperty_background_color) {

Please add a test for this in layout/style/test/test_dont_use_document_colors.html.

>diff -r 2cbe07bd5857 layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp	Tue Jun 24 19:52:40 2008 +1200
>+++ b/layout/style/nsCSSParser.cpp	Wed Jun 25 17:53:23 2008 +1000
>@@ -340,6 +340,7 @@ protected:
>                    nsCSSProperty aPropID);
>   PRBool DoParseRect(nsCSSRect& aRect, nsresult& aErrorCode);
>   PRBool ParseContent(nsresult& aErrorCode);
>+  PRBool ParseColumnRule(nsresult& aErrorCode);

Why can't you just use ParseBorderSide?

(And we should really get rid of -moz-use-text-color and special handling of transparent for borders, since we now have currentColor and general handling of transparent...)

I'd note that it seems odd that ParseColumnRule sets the NS_STYLE_MOZ_USE_TEXT_COLOR value, but you never parse -moz-use-text-color in the longhand case, which is inconsistent and ought to cause some tests to fail.


>diff -r 2cbe07bd5857 layout/style/nsComputedDOMStyle.cpp

I'm not going to review this yet given the issues with the nsStyleStruct code that need to be fixed.

>diff -r 2cbe07bd5857 layout/style/nsRuleNode.cpp
>--- a/layout/style/nsRuleNode.cpp	Tue Jun 24 19:52:40 2008 +1200
>+++ b/layout/style/nsRuleNode.cpp	Wed Jun 25 17:53:23 2008 +1000
>@@ -4623,6 +4623,53 @@ nsRuleNode::ComputeColumnData(void* aSta
>     column->mColumnCount = parent->mColumnCount;
>   }
> 
>+  // column-rule-width: length, enum, inherit
>+  const nsCSSValue &widthValue = columnData.mColumnRuleWidth;
>+  if (eCSSUnit_Initial == widthValue.GetUnit()) {
>+    column->mColumnRuleWidth.SetIntValue(NS_STYLE_BORDER_WIDTH_MEDIUM, eStyleUnit_Enumerated);
>+  } else {
>+    SetCoord(columnData.mColumnRuleWidth, column->mColumnRuleWidth,
>+             parent->mColumnRuleWidth,
>+             SETCOORD_LEH, aContext, mPresContext, inherited);
>+  }

Handle this like border-width and actually compute to an nscoord.

>+  // column-rule-color: color, inherit
>+  const nsCSSValue &colorValue = columnData.mColumnRuleColor;
>+  if (eCSSUnit_Inherit == colorValue.GetUnit() && parentContext) {
>+    inherited = PR_TRUE;
>+    if (parent->mColumnRuleColorIsForeground) {
>+      // We want to inherit the color from the parent, not use the
>+      // color on the element where this chunk of style data will be
>+      // used.  We can ensure that the data for the parent are fully
>+      // computed (unlike for the element where this will be used, for
>+      // which the color could be specified on a more specific rule).
>+      column->mColumnRuleColor = parentContext->GetStyleColor()->mColor;
>+    } else {
>+      column->mColumnRuleColor = parent->mColumnRuleColor;
>+    }
>+    column->mColumnRuleColorIsForeground = PR_FALSE;
>+  }
>+  else if (eCSSUnit_Initial == colorValue.GetUnit()) {
>+    column->mColumnRuleColorIsForeground = PR_TRUE;
>+  }
>+  else if (SetColor(colorValue, 0, mPresContext, aContext, column->mColumnRuleColor, inherited)) {
>+    column->mColumnRuleColorIsForeground = PR_FALSE;
>+  }

Don't do this complicated mColumnRuleColorIsForeground business; just use NS_COLOR_CURRENTCOLOR instead.  (Could you do a separate patch, to land first, that replaces the -moz-use-text-color and transparent handling for border-color with the generic currentColor and transparent handling?)

>diff -r 2cbe07bd5857 layout/style/nsStyleStruct.h
>--- a/layout/style/nsStyleStruct.h	Tue Jun 24 19:52:40 2008 +1200
>+++ b/layout/style/nsStyleStruct.h	Wed Jun 25 17:53:23 2008 +1000
>@@ -1287,6 +1287,13 @@ struct nsStyleColumn {
>   PRUint32     mColumnCount; // [reset] see nsStyleConsts.h
>   nsStyleCoord mColumnWidth; // [reset]
>   nsStyleCoord mColumnGap;   // [reset] coord
>+
>+  nsStyleCoord mColumnRuleWidth;  // [reset] coord
>+  nscolor      mColumnRuleColor;  // [reset]
>+  PRUint8      mColumnRuleStyle;  // [reset]
>+  PRPackedBool mColumnRuleColorIsForeground;  // [reset]

You don't want mColumnRuleColorIsForeground; you want to just set it to the foreground color instead.

>+
>+  nscoord CalcColumnRuleWidth(nsPresContext* aContext) const;

Why can't you do this computation in nsRuleNode::ComputeColumnData?

>diff -r 2cbe07bd5857 layout/style/test/property_database.js
>+	"-moz-column-rule-color": {
>+		domProp: "MozColumnRuleColor",
>+		inherited: false,
>+		type: CSS_TYPE_LONGHAND,
>+		prerequisites: { "color": "black" },

I'd use something other than black as a prerequisite. (twice)


Are all the places where you're adding known fails to tests places where border-top also fails?

Comment 28

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

And see also comment 9 and comment 10, in which I raised many of the same issues.  (I didn't realize I'd reviewed it before until after reviewing this one.  But why didn't you address the first set of review comments before requesting review again?)


Also, does the rendering patch do as suggested in comment 11?

Comment 29

[Michael Ventnor]

Attachment: Complete patch

Though it probably wasn't a good idea, I went ahead and killed -moz-use-text-color as part of this patch. Because I couldn't be bothered specifying every file to Hg, I just diffed the entire tree so this includes the already-reviewed rendering parts in nsColumnSetFrame.cpp and the reftests.

Comment 30

[Michael Ventnor]

And I had to use black as a prerequisite because that is not only what every border does as well, but the test only seems to expect black as an initial colour value, even when I explicitly specify a colour in initial_values.

Comment 31

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

Comment on attachment 328657 [details] [diff] [review]
Complete patch

Please read through all the way to the end of this review before starting to address the comments.  A large number of them no longer apply given what I say at the end, but I'd like to still submit them because they're useful to somebody picking up the foreground/transparent removal later.

Note also that I haven't yet gone through my previous review comments; just the patch.

>@@ -869,13 +870,7 @@ nsCSSDeclaration::TryBorderShorthand(nsA
> 
>     nsAutoString valueString;
>     AppendValueToString(eCSSProperty_border_top_color, valueString);
>-    if (!valueString.EqualsLiteral("-moz-use-text-color")) {
>-      /* don't output this value, it's proprietary Mozilla and  */
>-      /* not intended to be exposed ; we can remove it from the */
>-      /* values of the shorthand since this value represents the */
>-      /* initial value of border-*-color */
>-      aString.Append(valueString);
>-    }
>+    aString.Append(valueString);
>     AppendImportanceToString(isImportant, aString);
>     aString.AppendLiteral("; ");
>   }
>@@ -903,10 +898,8 @@ nsCSSDeclaration::TryBorderSideShorthand
> 
>     nsAutoString valueString;
>     AppendValueToString(OrderValueAt(aBorderColor-1), valueString);
>-    if (!valueString.EqualsLiteral("-moz-use-text-color")) {
>-      aString.AppendLiteral(" ");
>-      aString.Append(valueString);
>-    }
>+    aString.AppendLiteral(" ");
>+    aString.Append(valueString);
>     AppendImportanceToString(isImportant, aString);
>     aString.AppendLiteral("; ");
>     return PR_TRUE;

I think you should leave these checks but change them to test for currentColor, since currentColor is a css3 thing, and since we want to output shorthands as minimally as possible.  (You should check if "currentColor" is the case we output it as.)

And we ought to add a mochitest for that, although I can do it if you want (but please ping me).

(Note that if we weren't keeping the tests, I'd have told you to remove the valueString variable.)

>diff -r e6ac4c25521f layout/style/nsCSSParser.cpp
>--- a/layout/style/nsCSSParser.cpp	Tue Jul 08 12:33:17 2008 +0300
>+++ b/layout/style/nsCSSParser.cpp	Wed Jul 09 21:38:59 2008 +1000
>@@ -4703,6 +4710,7 @@ PRBool CSSParserImpl::ParseSingleValuePr
>   case eCSSProperty_text_shadow:
>   case eCSSProperty_box_shadow:
>   case eCSSProperty_COUNT:
>+  case eCSSProperty__moz_column_rule:

Could you insert the new line before COUNT?

>@@ -4805,6 +4813,14 @@ PRBool CSSParserImpl::ParseSingleValuePr
>     return ParsePositiveVariant(aErrorCode, aValue, VARIANT_AHL, nsnull);
>   case eCSSProperty__moz_column_gap:
>     return ParsePositiveVariant(aErrorCode, aValue, VARIANT_HL | VARIANT_NORMAL, nsnull);
>+  case eCSSProperty__moz_column_rule_width:
>+    return ParsePositiveVariant(aErrorCode, aValue, VARIANT_HKL,
>+                                nsCSSProps::kBorderWidthKTable);
>+  case eCSSProperty__moz_column_rule_style:
>+    return ParseVariant(aErrorCode, aValue, VARIANT_HOK, 
>+                        nsCSSProps::kBorderStyleKTable);
>+  case eCSSProperty__moz_column_rule_color:
>+    return ParseVariant(aErrorCode, aValue, VARIANT_HC, nsnull);
>   case eCSSProperty__moz_outline_radius_topLeft:
>   case eCSSProperty__moz_outline_radius_topRight:
>   case eCSSProperty__moz_outline_radius_bottomRight:

I'd actually insert these case statements along with the border* ones so you don't have to duplicate the code.

>+nsresult
>+nsComputedDOMStyle::GetColumnRuleWidth(nsIDOMCSSValue** aValue)
>+{
>+  nsROCSSPrimitiveValue *val = GetROCSSPrimitiveValue();
>+  if (!val)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  const nsStyleColumn* column = GetStyleColumn();
>+  // Spec says that if the rule style is none or hidden, the computed value
>+  // of the rule width is 0.
>+  if (column->mColumnRuleStyle == NS_STYLE_BORDER_STYLE_NONE ||
>+      column->mColumnRuleStyle == NS_STYLE_BORDER_STYLE_HIDDEN)
>+    val->SetAppUnits(0);
>+  else
>+    SetValueToCoord(val, column->mColumnRuleWidth);
>+
>+  return CallQueryInterface(val, aValue);
>+}

Oh, this business.  I forgot about this.

Dealing with this in nsComputedDOMStyle isn't good enough because the 'inherit' value has to reflect this as well.

So you actually need to add a bit of the complexity that's present in the border-width handling; we need two separate values in the style struct so that you get inheritance correct.  See the mBorder and mActualBorder in nsRuleNode.  You should have an mColumnRuleWidth (or maybe mPreColumnRuleWidth) and a mComputedColumnRuleWidth.  You need to be careful to get explicit inheritance correct, but also overriding of cached rule data.  (And we should also be careful of inheritance-by-copy-construction.  In fact, I need to check that the existing code and the code used for bug 378217 works correctly in this case.)

And that reminds me -- like border widths, column rule widths should be rounded to the nearest pixel using the same macro used for border widths.  You should probably round both the PreWidth and the ComputedWidth; that's what we're switching to in bug 378217 for borders.

>@@ -4659,6 +4646,54 @@ nsRuleNode::ComputeColumnData(void* aSta
>   } else if (eCSSUnit_Inherit == columnData.mColumnCount.GetUnit()) {
>     inherited = PR_TRUE;
>     column->mColumnCount = parent->mColumnCount;
>+  }
>+
>+  // column-rule-width: length, enum, inherit

As I said, this is going to need significant changes.

>+  // column-rule-style: enum, none, inherit

...because of the interaction with mColumnRuleStyle == NS_STYLE_BORDER_NONE.

>+  // column-rule-color: color, inherit
>+  const nsCSSValue &colorValue = columnData.mColumnRuleColor;
>+  if (eCSSUnit_Inherit == colorValue.GetUnit() && parentContext) {

parent will always be non-null (it's the same as column when column is the root), so you don't need to null-check parentContext.

>diff -r e6ac4c25521f layout/style/nsStyleStruct.cpp
>--- a/layout/style/nsStyleStruct.cpp	Tue Jul 08 12:33:17 2008 +0300
>+++ b/layout/style/nsStyleStruct.cpp	Wed Jul 09 21:38:59 2008 +1000
>@@ -625,11 +625,15 @@ nsChangeHint nsStyleXUL::MaxDifference()
> // --------------------
> // nsStyleColumn
> //
>-nsStyleColumn::nsStyleColumn() 
>-{ 
>+nsStyleColumn::nsStyleColumn(nsPresContext* aPresContext)
>+{
>   mColumnCount = NS_STYLE_COLUMN_COUNT_AUTO;
>   mColumnWidth.SetAutoValue();
>   mColumnGap.SetNormalValue();
>+
>+  mColumnRuleWidth.SetCoordValue((aPresContext->GetBorderWidthTable())[NS_STYLE_BORDER_WIDTH_MEDIUM]);
>+  mColumnRuleStyle = NS_STYLE_BORDER_STYLE_NONE;
>+  mColumnRuleColor = NS_RGB(0, 0, 0);

Initializing to black doesn't make sense.  See long comment at the very end.


> nsStyleColumn::~nsStyleColumn() 
>@@ -652,8 +656,13 @@ nsChangeHint nsStyleColumn::CalcDifferen
>     return nsChangeHint_ReconstructFrame;
> 
>   if (mColumnWidth != aOther.mColumnWidth ||
>-      mColumnGap != aOther.mColumnGap)
>+      mColumnGap != aOther.mColumnGap ||
>+      mColumnRuleWidth != aOther.mColumnRuleWidth)
>     return nsChangeHint_ReflowFrame;
>+
>+  if (mColumnRuleStyle != aOther.mColumnRuleStyle ||
>+      mColumnRuleColor != aOther.mColumnRuleColor)
>+    return nsChangeHint_RepaintFrame;
> 
>   return NS_STYLE_HINT_NONE;
> }

You also need to check the rule-width-including-rule-style here, since a change in column-rule-style from 'none' to non-none requires a reflow, since the computed width changes.

>@@ -662,8 +671,7 @@ nsChangeHint nsStyleColumn::CalcDifferen
> /* static */
> nsChangeHint nsStyleColumn::MaxDifference()
> {
>-  return NS_CombineHint(nsChangeHint_ReconstructFrame,
>-                        nsChangeHint_ReflowFrame);
>+  return nsChangeHint_ReconstructFrame;
> }
> #endif

This seems wrong; it seems like you should include RepaintFrame as well.  Or was there a reason you chose what you did?



There are a *lot* more changes to nsStyleStruct that you need to make as part of eliminating transparent and foreground border colors.  There's a whole bunch of stuff that can be eliminated, and the callers fixed up to not handle the special cases.  However, in a few cases, the callers might be handling transparent as a special case in order to optimize things away; in such cases you'd want to replace the check with a check of the color against NS_RGBA(0, 0, 0, 0) rather than removing the check entirely.

I'd prefer if this were a separate patch on top of this one.

Things to eliminate include: BORDER_COLOR_SPECIAL, BORDER_COLOR_TRANSPARENT, BORDER_COLOR_FOREGROUND (note that you need to leave OUTLINE_COLOR_INITIAL and thus BORDER_STYLE_MASK), the third argument to AppendBorderColor, nsBorderColors::mTransparent, the second and third arguments to GetBorderColor, SetBorderTransparent, and SetBorderToForeground.


But that makes me realize what you have here isn't even sufficient for the computation end of the MOZ_USE_TEXT_COLOR removal, because the nsStyleBorder default constructor uses BORDER_COLOR_FOREGROUND, and we still depend on that with your patch.  That means we'll also serialize to -moz-use-text-color as well.  So maybe we should switch back to the other approach for now (i.e., having the same setup in the column rule style, with a bit indicating that it's set to foreground, so we can get the correct default), and note this experience in the bug on removing -moz-use-text-color.

Probably the way to move forward on this issue is to allow the style context to be passed as an argument to style struct constructors, and do so for nsStyleBorder and nsStyleColumn, so that they can initialize the colors in question to GetStyleColor()->mColor.  This would also require that we pass the 'inherited' PRBool be initialized differently.  But a style context wouldn't work for the callers in nsStyleSet.

This gets complicated pretty fast, so you should not attempt it; sorry for suggesting that you should earlier.  I think you should undo the removal of the foreground/transparent stuff on border-colors and add the same mechanism back to column rule style/color (although maybe using the same setup of bits as border).

Comment 32

[Michael Ventnor]

"Probably the way to move forward on this issue is to allow the style context to
be passed as an argument to style struct constructors, and do so for
nsStyleBorder and nsStyleColumn, so that they can initialize the colors in
question to GetStyleColor()->mColor.  This would also require that we pass the
'inherited' PRBool be initialized differently.  But a style context wouldn't
work for the callers in nsStyleSet."

So hang on, what exactly are you saying here? If it wouldn't work with nsStyleSet then how can I proceed with this approach?

Comment 33

[Michael Ventnor]

Also, what's wrong with just having one value, and a getter function that checks the style value and returns 0 if invisible, and the width value if visible?

Comment 34

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

(In reply to comment #32)
> "Probably the way to move forward on this issue is to allow the style context
> to
> be passed as an argument to style struct constructors, and do so for
> nsStyleBorder and nsStyleColumn, so that they can initialize the colors in
> question to GetStyleColor()->mColor.  This would also require that we pass the
> 'inherited' PRBool be initialized differently.  But a style context wouldn't
> work for the callers in nsStyleSet."
> 
> So hang on, what exactly are you saying here? If it wouldn't work with
> nsStyleSet then how can I proceed with this approach?

I'm saying not to proceed with this approach.  Please undo the changes resulting from my suggestion in comment 27 to get rid of -moz-use-text-color.  See the last paragraph of comment 31.

(In reply to comment #33)
> Also, what's wrong with just having one value, and a getter function that
> checks the style value and returns 0 if invisible, and the width value if
> visible?

That works too.

Comment 35

[Michael Ventnor]

Attachment: Complete patch 2

Contains the already-reviewed rendering part. I think I covered all the comments.

Comment 36

[Michael Ventnor]

Attachment: Complete patch 2.1

Previous patch had a slight fail in it.

Comment 37

[Michael Ventnor]

Comment on attachment 329652 [details] [diff] [review]
Complete patch 2.1

Found another slight problem with this patch. New one soon.

Comment 38

[Michael Ventnor]

Attachment: Complete patch 2.2

Comment 39

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

So you should make nsStyleColumn::mColumnRuleWidth an nscoord rather than an nsStyleCoord.  You should also make it private.

Your nsStyleStruct changes also need to be merged with the border-image landing.

The nsRuleNode code that handles -moz-column-rule-width: inherit needs to use the parent's GetComputedColumnRuleWidth, not mColumnRuleWidth.

Comment 40

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

Why do mColumnRuleWidth changes cause reflow hints?  It doesn't seem like reflow considers the column rule width.

(Although maybe it should, in case the rules are so big they overflow -- which would mean they'd need to contribute to overflow area.)

Comment 41

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

Comment on attachment 329971 [details] [diff] [review]
Complete patch 2.2

r+sr=dbaron with the changes in comment 39 and comment 40, although I wouldn't mind taking a look at the merged nsStyleStruct.h.

Comment 42

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

(In reply to comment #40)
> (Although maybe it should, in case the rules are so big they overflow -- which
> would mean they'd need to contribute to overflow area.)

Note that if you think you should do this, it should probably be a followup bug.

Comment 43

[Michael Ventnor]

(In reply to comment #40)
> (Although maybe it should, in case the rules are so big they overflow -- which
> would mean they'd need to contribute to overflow area.)
> 

I don't think this would ever be the case since the rules would have to be so thick they would completely cover the column frame, in which case it would be better to use a background anyway.

Comment 44

[Michael Ventnor]

Attachment: Complete patch addressed comments

Comment 45

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

I'm not sure how that call to SetCoord even compiles (passing an nscoord to a parameter expecting nsStyleCoord&) -- but you should instead just call CalcLength (if the value IsLengthUnit()) and handle the inherit case yourself (if eCSSUnit_Inherit).

Could you also go back to having IsVisibleStyle be a method on nsStyleBorder -- just change it to return IsVisibleBorderStyle(GetBorderStyle(aSide)).  Then the comment will still make sense, and the callers will be simpler.

Comment 46

[Michael Ventnor]

Attachment: Addressed comments 2

Comment 47

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

Comment on attachment 330342 [details] [diff] [review]
Addressed comments 2

r+sr=dbaron

Whoever checks it in should fix this overly long line:

>+    column->SetColumnRuleWidth(CalcLength(widthValue, aContext, mPresContext, inherited));

by wrapping "mPresContext, inherited" to line up with "widthValue".

Comment 48

[Michael Ventnor]

Attachment: Addressed wrapping

Don't worry, I'll do it. Might as well do something while waiting for the stubborn orange unit test boxes to finally start building again.

Comment 49

[Michael Ventnor]

Attachment: aInherited => inherited

One last-minute issue I noticed.

Comment 50

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/8f3ff1995383

Comment 51

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

Was the last-minute issue something that the mochitests caught, or do we need a mochitest to catch that case?

Comment 52

[Michael Ventnor]

(In reply to comment #51)
> Was the last-minute issue something that the mochitests caught, or do we need a
> mochitest to catch that case?
> 

I just made a typo, thats all. I needed to set "inherited" instead of "aInherited" in nsRuleNode. It still compiled and I don't know if there would've been any problems had it not been caught, but it wasn't the "right" thing.

Comment 53

[j.j.]

What's about the very handy shorthand |columns:| ?

(URL changed to latest draft)

Comment 54

[Jesse Ruderman]

Michael filed bug 446569 for the handy shorthand.

Comment 55

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

Attachment: fix the test_dont_use_document_colors.html changes

This fixes the changes to test_dont_use_document_colors.html, which, by comparing cs2 and cs3, were assuming that inputs have the same default 'color' as divs; this is only true for some sets of themes and color preferences.  We should compare cs1 and cs2 instead.

Comment 56

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

With this patch, the test passes for me.  But I verified that with this patch, if I revert the change to block -moz-column-rule-color in nsCSSDataBlock, the test fails.

Comment 57

[Xidorn Quan [:xidorn] UTC+10]

A comment in nsStyleColumn [1] points to comment 43 for why column-rule-color cannot use currentcolor, but comment 43 seems to be unrelated, and searching the whole page, I don't find any justification that currentcolor cannot be used.

dbaron, it seems the comment was added by you [2], could you explain why currentcolor is hard? I guess the reason may no longer be relevant? If you forgot, I can probably just try and see what would happen.

[1] https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/layout/style/nsStyleStruct.h#3527-3528
[2] https://hg.mozilla.org/mozilla-central/rev/84a1b8853494

Comment 58

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

I suspect it was referring to the last 3 paragraphs of comment 31.  And I think the reason was largely that that the computed value resulting from currentcolor was then a color.  I think this is no longer a problem now that the computed value is a 'currentcolor' value rather than a color.