Implement CSS3 column-rule-*

RESOLVED FIXED in mozilla1.9.1a1

Status

()

enhancement
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: juergenherz, Assigned: ventnor.bugzilla)

Tracking

({css3, dev-doc-complete})

Trunk
mozilla1.9.1a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 15 obsolete attachments)

36.31 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
3.15 KB, patch
ventnor.bugzilla
: review+
Details | Diff | Splinter Review
Reporter

Description

15 years ago
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:

Updated

15 years ago
Keywords: css3
Assignee: nobody → roc
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Implement CSS3 column-rule-* and column-gap → Implement CSS3 column-rule-*
Comment on attachment 167808 [details] [diff] [review]
part 1: add style properties to the style system

oops, that was missing the DOM IDL changes
Attachment #167808 - Attachment is obsolete: true
Attachment #167808 - Flags: review-
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.
According to the spec we have to add the column-rule width to the column-gap,
so it affects layout. I've done that.
Attachment #167809 - Flags: superreview?(dbaron)
Attachment #167809 - Flags: review?(dbaron)
Note that the patches can be applied separately, but they must be applied in the
given order.
Attachment #167810 - Flags: superreview?(dbaron)
Attachment #167810 - Flags: review?(dbaron)
Attachment #167811 - Attachment description: apply column-rule in nsColumnSetFrame layout and rendering → part 3: apply column-rule in nsColumnSetFrame layout and rendering
Attachment #167811 - Flags: superreview?(dbaron)
Attachment #167811 - Flags: review?(dbaron)
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?
Dunno. I'll refresh the patches --- they have to be updated for the layout file
reorg, anyway.
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 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.
(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 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...)
Attachment #167810 - Flags: superreview?(dbaron)
Attachment #167810 - Flags: superreview+
Attachment #167810 - Flags: review?(dbaron)
Attachment #167810 - Flags: review+
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.
Attachment #167811 - Flags: superreview?(dbaron)
Attachment #167811 - Flags: superreview+
Attachment #167811 - Flags: review?(dbaron)
Attachment #167811 - Flags: review+
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
Attachment #167809 - Flags: superreview?(dbaron)
Attachment #167809 - Flags: superreview+
Attachment #167809 - Flags: review?(dbaron)
Attachment #167809 - Flags: review+

Comment 15

14 years ago
Why hasn't this been checked in?
The patch has got to be updated and there are other higher priority items right now

Comment 17

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

Comment 18

13 years ago
Robert, will column-rule-* be in 1.9?
Not sure. Persuade me that it's important :-)
Assignee

Comment 20

11 years ago
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.
Assignee: roc → ventnor.bugzilla
Assignee

Comment 21

11 years ago
Posted patch Parsing patch (obsolete) — Splinter Review
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).
Assignee

Comment 22

11 years ago
Posted patch Rendering patch (obsolete) — Splinter Review
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).
Attachment #326644 - Flags: superreview?(roc)
Attachment #326644 - Flags: review?(roc)
+  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.
Assignee

Comment 24

11 years ago
Posted patch Rendering patch 2 (obsolete) — Splinter Review
Fix comments.
Attachment #326644 - Attachment is obsolete: true
Attachment #327079 - Flags: superreview?(roc)
Attachment #327079 - Flags: review?(roc)
Attachment #326644 - Flags: superreview?(roc)
Attachment #326644 - Flags: review?(roc)
+    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.
Assignee

Comment 26

11 years ago
Posted patch Rendering patch 3 (obsolete) — Splinter Review
Addresses comments.
Attachment #327079 - Attachment is obsolete: true
Attachment #327240 - Flags: superreview?(roc)
Attachment #327240 - Flags: review?(roc)
Attachment #327079 - Flags: superreview?(roc)
Attachment #327079 - Flags: review?(roc)
Attachment #327240 - Flags: superreview?(roc)
Attachment #327240 - Flags: superreview+
Attachment #327240 - Flags: review?(roc)
Attachment #327240 - Flags: review+
Assignee

Updated

11 years ago
Attachment #326643 - Flags: superreview?(dbaron)
Attachment #326643 - Flags: review?(dbaron)
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?
Attachment #326643 - Flags: superreview?(dbaron)
Attachment #326643 - Flags: superreview-
Attachment #326643 - Flags: review?(dbaron)
Attachment #326643 - Flags: review-
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?
Assignee

Comment 29

11 years ago
Posted patch Complete patch (obsolete) — Splinter Review
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.
Attachment #326643 - Attachment is obsolete: true
Attachment #327240 - Attachment is obsolete: true
Attachment #328657 - Flags: superreview?(dbaron)
Attachment #328657 - Flags: review?(dbaron)
Assignee

Comment 30

11 years ago
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 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).
Attachment #328657 - Flags: superreview?(dbaron)
Attachment #328657 - Flags: superreview-
Attachment #328657 - Flags: review?(dbaron)
Attachment #328657 - Flags: review-
Assignee

Comment 32

11 years ago
"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?
Assignee

Comment 33

11 years ago
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?
(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.
Assignee

Comment 35

11 years ago
Posted patch Complete patch 2 (obsolete) — Splinter Review
Contains the already-reviewed rendering part. I think I covered all the comments.
Attachment #328657 - Attachment is obsolete: true
Attachment #329640 - Flags: superreview?(dbaron)
Attachment #329640 - Flags: review?(dbaron)
Assignee

Comment 36

11 years ago
Posted patch Complete patch 2.1 (obsolete) — Splinter Review
Previous patch had a slight fail in it.
Attachment #329640 - Attachment is obsolete: true
Attachment #329652 - Flags: superreview?(dbaron)
Attachment #329652 - Flags: review?(dbaron)
Attachment #329640 - Flags: superreview?(dbaron)
Attachment #329640 - Flags: review?(dbaron)
Assignee

Comment 37

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

Found another slight problem with this patch. New one soon.
Attachment #329652 - Attachment is obsolete: true
Attachment #329652 - Flags: superreview?(dbaron)
Attachment #329652 - Flags: review?(dbaron)
Assignee

Comment 38

11 years ago
Posted patch Complete patch 2.2 (obsolete) — Splinter Review
Attachment #329971 - Flags: superreview?(dbaron)
Attachment #329971 - Flags: review?(dbaron)
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.
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 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.
Attachment #329971 - Flags: superreview?(dbaron)
Attachment #329971 - Flags: superreview+
Attachment #329971 - Flags: review?(dbaron)
Attachment #329971 - Flags: review+
(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.
Assignee

Comment 43

11 years ago
(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.
Assignee

Comment 44

11 years ago
Attachment #329971 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Attachment #167809 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Attachment #167810 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Attachment #167811 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Keywords: checkin-needed
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.
Assignee

Updated

11 years ago
Keywords: checkin-needed
Assignee

Comment 46

11 years ago
Posted patch Addressed comments 2 (obsolete) — Splinter Review
Attachment #330338 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Keywords: checkin-needed
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".
Attachment #330342 - Flags: superreview+
Attachment #330342 - Flags: review+
Assignee

Comment 48

11 years ago
Posted patch Addressed wrapping (obsolete) — Splinter Review
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.
Attachment #330342 - Attachment is obsolete: true
Assignee

Comment 49

11 years ago
One last-minute issue I noticed.
Attachment #330347 - Attachment is obsolete: true
http://hg.mozilla.org/index.cgi/mozilla-central/rev/8f3ff1995383
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Assignee

Updated

11 years ago
Keywords: dev-doc-needed
Was the last-minute issue something that the mochitests caught, or do we need a mochitest to catch that case?
Assignee

Comment 52

11 years ago
(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.

Updated

11 years ago
Depends on: 446273

Comment 53

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

(URL changed to latest draft)

Comment 54

11 years ago
Michael filed bug 446569 for the handy shorthand.
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.
Attachment #332265 - Flags: review?(ventnor.bugzilla)
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.
Assignee

Updated

11 years ago
Attachment #332265 - Flags: review?(ventnor.bugzilla) → review+
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
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.