Last Comment Bug 59109 - implement CSS3 text module's text-decoration-style and text-decoration-color
: implement CSS3 text module's text-decoration-style and text-decoration-color
Status: RESOLVED FIXED
docs: note this was backed of Gecko 5
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P3 enhancement with 13 votes (vote)
: mozilla6
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Jet Villegas (:jet)
Mentors:
http://dev.w3.org/csswg/css3-text/#li...
Depends on: 1777 365336 365414 392785 403524 482138 537230 647421 648299 649551 780436 812995
Blocks: css-text-3 56702
  Show dependency treegraph
 
Reported: 2000-11-04 05:06 PST by Matthew Paul Thomas
Modified: 2012-11-19 01:02 PST (History)
46 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.0 (work in progress) (26.53 KB, patch)
2010-10-03 21:05 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part2. Cleaning up current text decoration implementation v1.0 (38.97 KB, patch)
2010-10-03 21:06 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.0 (work in progress) (33.17 KB, patch)
2010-10-03 21:07 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.1 (29.14 KB, patch)
2010-10-03 23:42 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.1 (32.56 KB, patch)
2010-10-03 23:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.2 (30.11 KB, patch)
2010-10-04 00:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.2 (32.14 KB, patch)
2010-10-05 01:00 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.3 (30.15 KB, patch)
2010-10-20 04:32 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part4. reftests v1.0 (58.36 KB, patch)
2010-10-20 04:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.3 (30.16 KB, patch)
2011-03-23 08:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part2. Cleaning up current text decoration implementation v1.0 (37.55 KB, patch)
2011-03-23 08:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
dbaron: review+
Details | Diff | Splinter Review
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.2 (34.14 KB, patch)
2011-03-23 08:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
dbaron: review+
Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.4 (28.59 KB, patch)
2011-03-24 05:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.5 (28.70 KB, patch)
2011-03-25 09:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
dbaron: review+
Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.6 (mq) (20.25 KB, patch)
2011-03-29 17:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bzbarsky: superreview+
Details | Diff | Splinter Review
Part2. Cleaning up current text decoration implementation v1.1 (mq) (22.59 KB, patch)
2011-03-29 17:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part4. reftests v1.1 (58.35 KB, patch)
2011-03-29 18:04 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
dbaron: review+
Details | Diff | Splinter Review
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.6 (mq) (20.26 KB, patch)
2011-03-29 21:50 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.3 (mq) (24.87 KB, patch)
2011-03-30 17:51 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part4. reftests v1.1.1 (mq) (56.15 KB, patch)
2011-03-30 17:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.4 (mq) (24.88 KB, patch)
2011-03-30 21:19 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review

Description Matthew Paul Thomas 2000-11-04 05:06:22 PST
A number of useful features for Mozilla would be best implemented using 
underlining effects which are more varied than CSS's awkward `text-decoration' 
property provides for.

These features include:
* a different underline style for links which will open in a new window <bug
  14027>
* a different underline style for links which are to internal anchors in the same
  document <bug 58135>
* a dotted underline for elements with TITLE attributes <bug 56702>
* a spell-as-you go feature in Composer which gives misspelled words a red zigzag
  underline as the user types them.

So, I suggest a `-moz-text-underline' property and its children be introduced. 
Straw man spec as follows.

`-moz-text-underline' 

    Value: none | [ <`-moz-text-underline-width'> ||
      <`-moz-text-underline-style'> || <`-moz-text-underline-color'> ] | inherit 
    Initial: none
    Applies to: all elements 
    Inherited: no (see prose)
    Percentages: N/A 
    Media: visual 

    This property describes underlining that is added to the text of an element.
    Unlike the `text-decoration' property, it allows for underlining which is a
    different color from the text of an element, it allows for a number of
    different underlining styles, and it allows for underlining which is not
    mutually exclusive with overlining or other text decorations.

    If the property is specified for a block-level element, it affects all
    inline-level descendants of the element. If it is specified for (or affects)
    an inline-level element, it affects all boxes generated by the element. If
    the element has no content or no text content (e.g., the IMG element in
    HTML), user agents must ignore this property. 

    This property is not inherited, but descendant boxes of a block box should be
    formatted with the same underlining. The color of underlining should remain
    the same even if descendant elements have different 'color' values.

    The height of an element should be increased, where necessary, to make room
    for underlining added to text in that element.

`-moz-text-underline-width'

    [as for `border-top-width']

`-moz-text-underline-style'

    Value: none | solid | double | dotted | dashed | wavy | inherit 
    Initial: none
    Applies to: all elements 
    Inherited: no (see note for `-moz-text-underline')
    Percentages: N/A
    Media: visual 

    The 'border-style' property sets the style of the underlining for the
    element. The values have the following effects:

    none
        No underlining.
    solid
        The underline is a single line segment.
    double 
        The underlining consists of two solid lines, each with a width equal to
        the value of `-moz-text-underline-width', with a space between them which
        also has a width equal to the value of `-moz-text-underline-width'. (Note
        that this interaction between the `-moz-text-underline-width' property
        and the `double' value for the `-moz-text-underline-style' property, is
        different from the interaction between the `border-width' property and
        the `double' value for the `border-style' property.)
    dotted
        The underline is a series of dots. 
    dashed 
        The underline is a series of short line segments. 
    wavy
        The underline is a curved or zig-zagged line.

    Conforming lizards may interpret any or all of `dotted', `dashed', `double',
    and `wavy' as `solid', but it would be nice if they didn't.
Comment 1 Matthew Paul Thomas 2000-11-04 05:08:41 PST
Errr ... I omitted out the spec for `-moz-text-underline-color', but it should be 
pretty obvious.
Comment 2 Marc Attinasi 2000-11-10 10:43:40 PST
This is very close to the CSS3 proposal for 'text-underline-style',
'text-overline-style' and the related properties thereon (mode,position,color).
The CSS3 proposal includes a few more styles (thick, single-accounting,
double-accounting, dot-dash, dot-dot-dash) but does not suggest width as a
separate property (I think it probably should). If we decide to implement this
as -moz-properties before the spec is public, then we should probably try to
stick with the details of the CSS3 proposal.
Comment 3 Hixie (not reading bugmail) 2001-02-12 16:29:05 PST
Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...
Comment 4 David Baron :dbaron: ⌚️UTC-10 2001-05-19 06:05:01 PDT
A working draft of the CSS3 text module was recently made public:

http://www.w3.org/TR/css3-text/#text-underline-props
Comment 5 Kai Lahmann (is there, where MNG is) 2002-05-26 08:21:41 PDT
-width still doesn't exist at W3C, but they have now:
text-underline-color (the color)
text-underline-style (values: none | solid | double | dotted | thick | dashed |
dot-dash | dot-dot-dash | wave)
text-underline-position (this is btw. implemented in MSIE but totally broken)
text-underline-mode (should spaces be underlines)
and a shorthand text-underline for all this.

same exists for text-overline and text-line-through
Comment 6 Jesse Ruderman 2002-06-10 13:46:47 PDT
See also bug 127962, which might add -moz-dashed-underline for accessibility
until this CSS3 draft is stable enough to implement.
Comment 7 David Baron :dbaron: ⌚️UTC-10 2002-06-19 21:06:33 PDT
Assigning pierre's remaining Style System-related bugs to myself.
Comment 8 Arthit Suriyawongkul 2003-01-16 08:09:03 PST
Thai example & Hindi example
also other RFEs related to "text-decoration-mode"
(like "skip cell")

bug 156881
Comment 9 Steffen Wilberg 2003-09-11 05:54:15 PDT
The css3 text module is a candidate recommendation since May 14 -> tweaking summary.
http://www.w3.org/TR/css3-text/#text-decoration-overview
Comment 10 Mitrophan Chin 2004-12-25 17:46:54 PST
I would love to see the double underline support using :

text-underline-style: double;

This would be useful in transcribing the Chinese Bible, where place names are
represented with the double underline, whereas person names are represented by
single underline.

See http://www.w3.org/TR/css3-text/#text-decoration-style

9.2. Text decoration style: the 'text-underline-style',
'text-line-through-style' and 'text-overline-style' properties
Names:  text-underline-style, text-line-through-style, text-overline-style  
Value:  none | solid | double | dotted | dashed | dot-dash | dot-dot-dash | wave  
Initial:  none  
Applies to:  all elements with and generated content with textual content 
Inherited:  no  
Percentages:  N/A  
Media:  visual  
Computed value:  specified value (except for initial and inherit)  
Comment 11 Anne (:annevk) 2004-12-26 05:05:26 PST
Please, do not add useless advocacy comments to a bug report. Use the forums
instead.
Comment 12 fantasai 2007-11-12 21:30:46 PST
Spec's been updated, see http://dev.w3.org/csswg/css3-text/#text-decoration-style
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-11-12 22:17:57 PST
(In reply to comment #12)
> Spec's been updated, see
> http://dev.w3.org/csswg/css3-text/#text-decoration-style

now, we can implement the solid/dotted/dashed/dot-dash/dot-dot-dash styles easily.

double: we need to think about overflowed decoration line rendering.
wave: we need to think about overflow too, and we need to find good way for wave line rendering.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-12-03 01:21:50 PST
I fixed bug 392785, so, we can implement this easily. However, I think that it is pretty late for Gecko1.9. So, I will take this bug after Gecko1.9.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-05-21 07:14:18 PDT
taking. but we should fix bug 403524 before this.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-05-21 07:26:08 PDT
I have a worry. The text-decoration-width is gone on the latest draft (comment 12). That is very good news for us. Because that is too difficult work for overflow issues. However, we still have the overflow issue on 'wavy' style and 'double' style. If such style is specified, we have a performance issue like bug 421353.

a {
  text-decoration: none;
}

a:hover {
  text-decoration: underline wavy;
}

Then, only hovered style may need overflow area for wavy line. But for computing it, we need to rise reflow. But that is very bad...

roc or dbaron, do you have good idea for this issue?

# If the authors specify the following style, we don't need to recompute the overflow area, but that is not good for us...

a {
  text-decoration: none wavy;
}

a:hover {
  text-decoration: underline wavy;
}

or

a {
  text-decoration: none wavy;
}

a:hover {
  text-decoration-line: underline;
}
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-05-21 15:26:44 PDT
We can special-case the use of wavy and double and do reflows for them.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-05-21 21:19:16 PDT
(In reply to comment #17)
> We can special-case the use of wavy and double and do reflows for them.

Excuse me, I cannot understand what you meant.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-05-21 21:22:00 PDT
Changes to and from "wavy" and "double" can trigger reflow hints and be slow.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-05-21 21:24:49 PDT
(In reply to comment #19)
> Changes to and from "wavy" and "double" can trigger reflow hints and be slow.

ah, ok.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-07-23 21:12:15 PDT
fantasai:

Hi, the latest WD of comment 12 doesn't define the blink value of text-decoration shorthand property. Isn't it still supported only with text-decoration property?

And also dot-dash, dot-dot-dash and wave are dropped from border-style. I think that dot-dash and dot-dot-dash are not needed. But wave is needed only for text-decoration-style. How about you?

dbaron:

It seems that
https://developer.mozilla.org/En/Adding_a_new_style_property
is old. Do you have a plan to update it?

And we should use -moz- prefix for the related new properties, but can we change text-decoration property behavior without the prefix? It seems bad. Should I add -moz-text-decoration property for the new behavior of CSS3?
Comment 22 fantasai 2009-07-24 11:08:59 PDT
Wrt spec: Fixed. :) Wrt -moz- and the text-decoration shorthand: how about leaving the color and style out of the shorthand for now, just require the longhand properties if you want to set those aspects?
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 21:05:04 PDT
Created attachment 480558 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.0 (work in progress)
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 21:06:11 PDT
Created attachment 480559 [details] [diff] [review]
Part2. Cleaning up current text decoration implementation v1.0
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 21:07:41 PDT
Created attachment 480560 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.0 (work in progress)
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 21:11:48 PDT
I need to learn about ":visited". Where are the documents about that?
Comment 27 David Baron :dbaron: ⌚️UTC-10 2010-10-03 22:05:04 PDT
http://dbaron.org/mozilla/visited-privacy , perhaps?

Hopefully this won't conflict too much with bug 403524 (which we *might* have to do for Firefox 4).
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 22:21:21 PDT
(In reply to comment #27)
> http://dbaron.org/mozilla/visited-privacy , perhaps?

Thank you.

> Hopefully this won't conflict too much with bug 403524 (which we *might* have
> to do for Firefox 4).

Part2 and Part3 may be conflict, but I think it's easy to merge. And I think that this should be landed after Fx4 is branched. If we'll land this for Fx4, we need to fix bug 537230 for quirks mode rendering too.
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 23:42:18 PDT
Created attachment 480571 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.1
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-03 23:43:01 PDT
Created attachment 480572 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.1
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-04 00:53:57 PDT
Created attachment 480575 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.2
Comment 32 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-05 01:00:09 PDT
Created attachment 480866 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.2

test builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/masayuki@d-toybox.com-e41111cad7bf/
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-20 04:32:52 PDT
Created attachment 484650 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.3
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-20 04:34:41 PDT
Created attachment 484651 [details] [diff] [review]
Part4. reftests v1.0

Ready to review, but we should wait until branching for Gecko 2.0.
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-23 08:22:05 PDT
Created attachment 521181 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.3
Comment 36 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-23 08:22:53 PDT
Created attachment 521183 [details] [diff] [review]
Part2. Cleaning up current text decoration implementation v1.0
Comment 37 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-23 08:24:08 PDT
Created attachment 521185 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.2
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-24 00:50:41 PDT
Comment on attachment 521181 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.3

Part1 is outdated and I found some bugs. I'll post new patch.
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-24 05:16:00 PDT
Created attachment 521465 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.4
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-25 09:46:33 PDT
Created attachment 521845 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.5

updated for latest trunk.
Comment 41 David Baron :dbaron: ⌚️UTC-10 2011-03-28 17:12:55 PDT
Comment on attachment 521845 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.5

the spec actually changed wave back to wavy.  So you should
search-and-replace the patch for wave->wavy and WAVE->WAVY

You'll need to update to trunk again now that bug 645620 landed, but
the update is really easy:  you don't need the nsCSSStruct.h changes
anymore, and you have to remove two of the lines from each
CSS_PROP_TEXTRESET entry in nsCSSPropList.h (see patch 5 in that bug).

Also, in the nsCSSPropList.h changes, please don't use _moz_ prefixes
on the second parameter.  (Some properties have them, some don't -- and
we're really better off leaving them out.)  That'll also require some
search-replacing.

In the third parameter in the nsCSSPropList.h changes, you should use
the CSS_PROP_DOMPROP_PREFIXED() macro instead of writing the "Moz".
This means that ValueForMozTextDecorationColor() will change to
ValueForTextDecorationColor(), etc. (in nsRuleNode.cpp).

Also, this comment:
>+    VARIANT_HCK, // used only internally
is incorrect since you use CSS_PROPERTY_PARSE_VALUE.  Just remove the
comment.

>+#define NS_STYLE_TEXT_DECORATION_STYLE_NONE     0 // not in CSS spec, but useful for selection underline

Are you using it?  If not, please don't add this; the spec has
the text-decoration-line property that does this and more.

It's also bad that you have a value that we don't know how to serialize
(e.g., in GetComputedStyle).

So actually I think you should just remove it.


In property_database.js, you should also test -moz-use-text-color (in
the initial_values line).

nsRuleNode.cpp:

>+  else if (eCSSUnit_Enumerated == decorationColorValue->GetUnit()) {
>+    switch (decorationColorValue->GetIntValue()) {
>+      case NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR:
>+        text->SetDecorationColorToForeground();
>+        break;
>+    }
>+  }
>+  else if (eCSSUnit_Initial == decorationColorValue->GetUnit()) {
>+    text->SetDecorationColorToForeground();
>+  }

It's better to combine these two cases, as:

+  else if (eCSSUnit_Initial == decorationColorValue->GetUnit() ||
+           eCSSUnit_Enumerated == decorationColorValue->GetUnit()) {
+    NS_ABORT_IF_FALSE(eCSSUnit_Enumerated != decorationColorValue->GetUnit() ||
+                      decorationColorValue->GetIntValue() ==
+                        NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR,
+                      "unexpected enumerated value");
+    text->SetDecorationColorToForeground();
+  }

nsStyleContext.cpp:
>+      if (thisVisDecColorIsFG != otherVisDecColorIsFG ||
>+          (!thisVisDecColorIsFG && thisVisDecColor != otherVisDecColorIsFG)) {

The second |otherVisDecColorIsFG| should be |otherVisDecColor|.

nsStyleStruct.h:

>+  void SetDecorationStyle(PRUint8 aStyle)
>+  {
>+    mTextDecorationStyle &= ~BORDER_STYLE_MASK;
>+    mTextDecorationStyle |= (aStyle & BORDER_STYLE_MASK);
>+  }

This should have an
  NS_ABORT_IF_FALSE((aStyle & BORDER_STYLE_MASK) == aStyle,
                    "style doesn't fit");

r=dbaron with those things fixed
Comment 42 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-28 18:54:22 PDT
(In reply to comment #41)
> Comment on attachment 521845 [details] [diff] [review]
> Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.5
> 
> >+#define NS_STYLE_TEXT_DECORATION_STYLE_NONE     0 // not in CSS spec, but useful for selection underline
> 
> Are you using it?  If not, please don't add this; the spec has
> the text-decoration-line property that does this and more.
> 
> It's also bad that you have a value that we don't know how to serialize
> (e.g., in GetComputedStyle).
> 
> So actually I think you should just remove it.

Hmm, we've used the decoration-style-none already (see part 2). If a clause in IME composition string doesn't need underline, it needs to indicate the none style.

> 
> 
> In property_database.js, you should also test -moz-use-text-color (in
> the initial_values line).
> 
> nsRuleNode.cpp:
> 
> >+  else if (eCSSUnit_Enumerated == decorationColorValue->GetUnit()) {
> >+    switch (decorationColorValue->GetIntValue()) {
> >+      case NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR:
> >+        text->SetDecorationColorToForeground();
> >+        break;
> >+    }
> >+  }
> >+  else if (eCSSUnit_Initial == decorationColorValue->GetUnit()) {
> >+    text->SetDecorationColorToForeground();
> >+  }
> 
> It's better to combine these two cases, as:
> 
> +  else if (eCSSUnit_Initial == decorationColorValue->GetUnit() ||
> +           eCSSUnit_Enumerated == decorationColorValue->GetUnit()) {
> +    NS_ABORT_IF_FALSE(eCSSUnit_Enumerated != decorationColorValue->GetUnit()
> ||
> +                      decorationColorValue->GetIntValue() ==
> +                        NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR,
> +                      "unexpected enumerated value");
> +    text->SetDecorationColorToForeground();
> +  }
> 
> nsStyleContext.cpp:
> >+      if (thisVisDecColorIsFG != otherVisDecColorIsFG ||
> >+          (!thisVisDecColorIsFG && thisVisDecColor != otherVisDecColorIsFG)) {
> 
> The second |otherVisDecColorIsFG| should be |otherVisDecColor|.
> 
> nsStyleStruct.h:
> 
> >+  void SetDecorationStyle(PRUint8 aStyle)
> >+  {
> >+    mTextDecorationStyle &= ~BORDER_STYLE_MASK;
> >+    mTextDecorationStyle |= (aStyle & BORDER_STYLE_MASK);
> >+  }
> 
> This should have an
>   NS_ABORT_IF_FALSE((aStyle & BORDER_STYLE_MASK) == aStyle,
>                     "style doesn't fit");
> 
> r=dbaron with those things fixed
Comment 43 David Baron :dbaron: ⌚️UTC-10 2011-03-28 19:09:34 PDT
Comment on attachment 521183 [details] [diff] [review]
Part2. Cleaning up current text decoration implementation v1.0

Change WAVE back to WAVY, and either remove NONE or (if you have to) add a -moz-none value to the CSS property so we have a way to serialize the computed value, and r=dbaron.
Comment 44 David Baron :dbaron: ⌚️UTC-10 2011-03-28 19:10:26 PDT
(In reply to comment #42)
> Hmm, we've used the decoration-style-none already (see part 2). If a clause in
> IME composition string doesn't need underline, it needs to indicate the none
> style.

Then the property should accept a -moz-none value so that GetComputedStyle() doesn't break when it's the computed value.
Comment 45 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-29 17:56:54 PDT
Created attachment 522895 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.6 (mq)
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-29 17:58:12 PDT
Created attachment 522897 [details] [diff] [review]
Part2. Cleaning up current text decoration implementation v1.1 (mq)
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-29 18:01:35 PDT
Comment on attachment 522895 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.6 (mq)

Boris, would you sr the interface change?
Comment 48 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-29 18:04:35 PDT
Created attachment 522898 [details] [diff] [review]
Part4. reftests v1.1
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2011-03-29 19:28:45 PDT
Comment on attachment 522895 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.6 (mq)

sr=me, though it might be good to add these at the end of the Moz list.
Comment 50 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-29 21:50:23 PDT
Created attachment 522921 [details] [diff] [review]
Part1. Adding -moz-text-decoration-color and -moz-text-decoration-style. v1.6 (mq)

Thank you, Boris.
Comment 51 David Baron :dbaron: ⌚️UTC-10 2011-03-30 12:14:14 PDT
Comment on attachment 521185 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.2

In nsHTMLContainerFrame::GetTextDecorations:

>-    aDecorations = this->GetStyleTextReset()->mTextDecoration &
>+    const nsStyleTextReset* styleTextReset = this->GetStyleTextReset();
>+    aDecorations = styleTextReset->mTextDecoration &
>                    NS_STYLE_TEXT_DECORATION_LINES_MASK;
>     if (aDecorations) {
>-      nscolor color = this->GetVisitedDependentColor(eCSSProperty_color);
>-      aUnderColor = color;
>-      aOverColor = color;
>-      aStrikeColor = color;
+      nscolor color =
>+        this->GetVisitedDependentColor(eCSSProperty__moz_text_decoration_color);
>+      aUnderColor = aOverColor = aStrikeColor = color;
>+      aUnderStyle = aOverStyle = aStrikeStyle =
>+        this->GetStyleTextReset()->GetDecorationStyle();

On the last line quoted, use styleTextReset instead of
this->GetStyleTextReset().

In nsHTMLContainerFrame.h:
>+   *    @param aStyle             the style of the text-decoration

You should mention here that it's one of the 
NS_STYLE_TEXT_DECORATION_STYLE_* constants.

Probably the same for the previous function as well.


In nsTextBoxFrame.cpp:

>+          PRBool isForeground;
>+          styleText->GetDecorationColor(color, isForeground);
>+          if (isForeground) {
>+            color = context->GetStyleColor()->mColor;
>+          }

This should actually use GetVisitedDependentColor.  That it wasn't
before was a bug -- but you should fix that.

r=dbaron with that
Comment 52 David Baron :dbaron: ⌚️UTC-10 2011-03-30 12:18:19 PDT
Comment on attachment 522898 [details] [diff] [review]
Part4. reftests v1.1

I don't think it's worth my trying to review these in detail -- I'll just rubber-stamp them.  r=dbaron

But you should probably avoid the "\ No newline at end of file" by adding a newline at the end of those files (just one though -- no need for a blank line to show up).
Comment 53 David Baron :dbaron: ⌚️UTC-10 2011-03-30 12:19:11 PDT
(That said, I'm happy about the use of != tests in addition to == tests.)
Comment 54 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-30 17:51:40 PDT
Created attachment 523184 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.3 (mq)
Comment 55 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-30 17:52:15 PDT
Created attachment 523185 [details] [diff] [review]
Part4. reftests v1.1.1 (mq)
Comment 56 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-30 21:19:43 PDT
Created attachment 523216 [details] [diff] [review]
Part3. Implement text-decoration-color and text-decoration-style rendering. v1.4 (mq)
Comment 58 dotnetCarpenter 2011-03-31 12:18:43 PDT
This is awesome, albeit 11 years... Just to be clear, the target milestone 2.2 means gecko 2.2?

Cheers - jon
Comment 59 Daniel Holbert [:dholbert] 2011-03-31 12:28:40 PDT
Yup, that's correct.
Comment 60 dotnetCarpenter 2011-03-31 13:29:11 PDT
I'm looking forward for this to be in the nightlies and writing dev doc for it.
Test uri: http://jsfiddle.net/dotnetCarpenter/LEkNZ/4/embedded/result/

ps. use nightly ff (as of writing - not working)
Comment 61 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-03-31 18:24:11 PDT
The points about current implementation are:

* -moz- prefix needed.
* text-decoration-line hasn't been implemented yet because it's still same as text-decoration of current implementation except blink value.
* text-decoration isn't shorthand property for text-decoration-* properties because text-decoration has been since CSS1 and we cannot append vender prefix for such property.
* see bug 537230, there is a problem for connecting complex style lines.
Comment 62 dotnetCarpenter 2011-04-01 00:41:47 PDT
So a red wave underline would be declared as:
text-decoration: underline;
-moz-text-decoration-style: wave;
-moz-text-decoration-color: red;

http://jsfiddle.net/dotnetCarpenter/LEkNZ/6/

I've tested with the 31th of Marts nightly and can't get it to work.
Comment 63 Steffen Wilberg 2011-04-01 00:49:42 PDT
The color works fine here, and the style as well once you change wave to wavy.
Comment 64 dotnetCarpenter 2011-04-01 02:15:46 PDT
Sorry my bad... hmm on second glance at the specs, they specify wave. Was this changes based on discussions on the w3 mailing list? Would think that the draft paper was up to date. Either way, I changed it to wavy and it works. Yay!

http://jsfiddle.net/dotnetCarpenter/LEkNZ/11/
Comment 65 Steffen Wilberg 2011-04-01 02:20:46 PDT
The Editor's draft changed to wavy:
http://dev.w3.org/csswg/css3-text/#text-decoration-style0
Comment 66 fantasai 2011-04-01 11:46:58 PDT
Masayuki, I think you should implement text-decoration as the shorthand per spec. It might not accept the new values, but the behavior it has of resetting the color and the style is significant.
Comment 67 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-01 19:00:26 PDT
Ah, sounds reasonable. How about you, dbaron?
Comment 68 David Baron :dbaron: ⌚️UTC-10 2011-04-01 22:14:48 PDT
Yes, sounds correct.  Sorry, I should have caught that in review.  We should add -moz-text-decoration-line to be what text-decoration is now, and make text-decoration a shorthand.

Except I'm not quite sure how to do that if blink isn't a value of -moz-text-decoration-line -- we'd need a place to store it, and I'd rather have it be a value of -moz-text-decoration-line than have a fourth, hidden property there.

You don't need to do cancel-* yet.
Comment 69 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-02 02:22:56 PDT
filed bug 647421.
Comment 70 Matthew Paul Thomas 2011-04-04 13:06:08 PDT
(In reply to comment #58)
> This is awesome, albeit 11 years...

That's okay, I didn't need it urgently. ;-) Thanks, Masayuki!
Comment 71 David Baron :dbaron: ⌚️UTC-10 2011-04-11 17:00:20 PDT
I'm inclined to back this out for Firefox 5 since we haven't yet fixed bug 647421.
Comment 72 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-04-11 17:08:18 PDT
(In reply to comment #71)
> I'm inclined to back this out for Firefox 5 since we haven't yet fixed bug
> 647421.

Yeah. I agree. We don't have much time for Fx5 due to the irregular schedule.
Comment 73 :Ehsan Akhgari 2011-04-11 20:05:04 PDT
This should be backed out on aurora tomorrow.
Comment 74 David Baron :dbaron: ⌚️UTC-10 2011-04-13 10:33:01 PDT
bug 649551 covers backing this out from Aurora.
Comment 75 Eric Shepherd [:sheppy] 2011-04-22 13:38:43 PDT
Documented:

https://developer.mozilla.org/en/CSS/text-decoration-style
https://developer.mozilla.org/en/CSS/text-decoration-color

And added to Firefox 6 for developers.

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