647421 - -moz-text-decoration-color and -moz-text-decoration-style should be reset by text-decoration

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

spinning out from bug 59109.

> 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.


> Masayuki Nakano (Mozilla Japan) 2011-04-01 19:00:26 PDT 

> Ah, sounds reasonable. How about you, dbaron?


> David Baron [:dbaron] 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 1

[David Baron :dbaron:]

Masayuki, were you planning to do this, or should I take it?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have a patch on my machine. If I cannot solve remaining problem in the patch, I'll ask you.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

This patch makes 2 new properties:

1. -moz-text-blink. this is based on old CSS3 text module:
http://www.w3.org/TR/2003/CR-css3-text-20030514/#text-blink

2. -moz-text-decoration-line. this is based on latest draft of CSS3 text module. But doesn't support cancel-*.

text-decoration accepts only -moz-text-blink and -moz-text-decoration-line values but it's a shorthand property for -moz-text-blink, -moz-text-decoration-line, -moz-text-decoration-style and -moz-text-decoration-color.

If the value is inherit, the color and style are also inherited.

If the value is -moz-initial, the values are also initialized.

Otherwise, they are reset by their initial value.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If text-decoration becomes a shorthand property actually, I worry about the compatibility of the result of getComputedValue(). For example, if there is |getComputedValue("text-decoration") == "underline"| comparison on web sites, isn't this broken only on support browser? Has this been discussed in W3C?

Comment 5

[David Baron :dbaron:]

Comment on attachment 526194 [details] [diff] [review]
Patch v1.0

Declaration.cpp:

You need to refuse to serialize (i.e., return the empty string) if
either of the other subproperties is set to a non-initial value (i.e.,
any value other than the one the parser sets when parsing the shorthand).

It also looks like it's wrong in a whole bunch of other ways.  For
example, I think it will serialize "blink" as "blinknone"... which
should cause mochitests to fail.  If mochitests don't currently fail --
we need tests that do.  If the do -- then you should have run at least
the layout/style mochitests before requesting review.  (When fixing
this, you need to be sure you do return "none" when you need to, rather
than "".)  (I actually suspect the tests don't currently fail, since
"blink" isn't in the list of values for text-decoration in
property_database.js.  You should probably make that list a good bit
more thorough.)

Additionally, you can assert that the units on the all the subproperties
are Enumerated (except color, which can be Enumerated or one of the
color units), since Inherit/Initial are handled at the top of the
function.

general:

I think that kTextDecorationKTable and the NS_STYLE_TEXT_DECORATION_*
constants (not the ones with _LINE_ or _BLINK_, though) should move to
inside nsCSSParser.cpp, which should be the only thing that still uses
them.

nsComputedDOMStyle.cpp:

I think nsComputedDOMStyle::DoGetTextDecoration() would be simpler if
you used only one string rather than separating blinkString and
lineString and then recombining them.  You can, I think, just do
something like:
  if (line != ...NONE) {
    nsStyleUtil::AppendBitmaskCSSValue(..., str);
  }
  if (blink != ...NONE) {
    if (!str.IsEmpty()) {
      str.Append(PRUnichar(' '));
    }
    nsStyleUtil::AppendBitmaskCSSValue(..., str);
  }

nsRuleNode.cpp:

Can the text-blink code use SetDiscrete?

nsStyleContext.cpp:

The use of | is odd.  Can you just use || between tests of both values?


Otherwise this looks good, but I'd like to look at the revised patch.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

> Declaration.cpp:
> 
> You need to refuse to serialize (i.e., return the empty string) if
> either of the other subproperties is set to a non-initial value (i.e.,
> any value other than the one the parser sets when parsing the shorthand).
> 
> It also looks like it's wrong in a whole bunch of other ways.  For
> example, I think it will serialize "blink" as "blinknone"...

I'm not sure what's the caller of Declaration::GetValue(), would you tell me it?

> nsStyleContext.cpp:
> 
> The use of | is odd.  Can you just use || between tests of both values?

I researched it. NS_STYLE_HAS_TEXT_DECORATIONS and HasTextDecorations() are only used for decoration lines, therefore, we can rename them as NS_STYLE_HAS_TEXT_DECORATION_LINES and HasTextDecorationLines(). Then, we don't need to change the logic.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And would you review bug 648299 too? This bug's patch depends on bug 648299's patch. I want to land them at same time.

Comment 8

[David Baron :dbaron:]

(In reply to comment #6)
> I'm not sure what's the caller of Declaration::GetValue(), would you tell me
> it?

The getter for element.style.textDecoration exercises this code.  If you add "blink" as one of the other_values for text-decoration in property_database.js, you should see failure due to the previous patch.

> I researched it. NS_STYLE_HAS_TEXT_DECORATIONS and HasTextDecorations() are
> only used for decoration lines, therefore, we can rename them as
> NS_STYLE_HAS_TEXT_DECORATION_LINES and HasTextDecorationLines(). Then, we don't
> need to change the logic.

I'm not saying the logic is wrong -- it's just hard to read and easily broken by other changes.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #8)
> (In reply to comment #6)
> > I'm not sure what's the caller of Declaration::GetValue(), would you tell me
> > it?
> 
> The getter for element.style.textDecoration exercises this code.  If you add
> "blink" as one of the other_values for text-decoration in property_database.js,
> you should see failure due to the previous patch.

Thanks. Now, the latest patch has "blink" in other_values in property_database.js.

> > I researched it. NS_STYLE_HAS_TEXT_DECORATIONS and HasTextDecorations() are
> > only used for decoration lines, therefore, we can rename them as
> > NS_STYLE_HAS_TEXT_DECORATION_LINES and HasTextDecorationLines(). Then, we don't
> > need to change the logic.
> 
> I'm not saying the logic is wrong -- it's just hard to read and easily broken
> by other changes.

I meant that the code (not logic, sorry) doesn't need to be changed from current trunk except renaming. The callers of HasTextDecorations() handle only decoration lines, so, we don't need to includes blink value.

Comment 10

[David Baron :dbaron:]

(In reply to comment #9)
> I meant that the code (not logic, sorry) doesn't need to be changed from
> current trunk except renaming. The callers of HasTextDecorations() handle only
> decoration lines, so, we don't need to includes blink value.

OK, right.  That looks good.

Comment 11

[David Baron :dbaron:]

(In reply to comment #4)
> If text-decoration becomes a shorthand property actually, I worry about the
> compatibility of the result of getComputedValue(). For example, if there is
> |getComputedValue("text-decoration") == "underline"| comparison on web sites,
> isn't this broken only on support browser? Has this been discussed in W3C?

I think this should still work.  Does it not?

Comment 12

[David Baron :dbaron:]

Comment on attachment 527719 [details] [diff] [review]
Patch v1.1

>+      PRInt32 lineValue = NS_STYLE_TEXT_DECORATION_LINE_NONE;
>+      if (intValue & eDecorationUnderline) {
>+        lineValue |= NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE;
>+      }
>+      if (intValue & eDecorationOverline) {
>+        lineValue |= NS_STYLE_TEXT_DECORATION_LINE_OVERLINE;
>+      }
>+      if (intValue & eDecorationLineThrough) {
>+        lineValue |= NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH;
>+      }
>+      if (intValue & eDecorationPrefAnchors) {
>+        lineValue |= NS_STYLE_TEXT_DECORATION_LINE_PREF_ANCHORS;
>+      }
>+      line.SetIntValue(lineValue, eCSSUnit_Enumerated);

Instead of this, do:

PR_STATIC_ASSERT(eDecorationUnderline == 
                 NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE);
PR_STATIC_ASSERT(eDecorationOverline ==
                 NS_STYLE_TEXT_DECORATION_LINE_OVERLINE);
PR_STATIC_ASSERT(eDecorationLineThrough ==
                 NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH);
PR_STATIC_ASSERT(eDecorationPrefAnchors ==
                 NS_STYLE_TEXT_DECORATION_LINE_PREF_ANCHORS);
line.SetIntValue(intValue & ~PRInt32(eDecorationBlink),
                 eCSSUnit_Enumerated);


r=dbaron with that

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #11)
> (In reply to comment #4)
> > If text-decoration becomes a shorthand property actually, I worry about the
> > compatibility of the result of getComputedValue(). For example, if there is
> > |getComputedValue("text-decoration") == "underline"| comparison on web sites,
> > isn't this broken only on support browser? Has this been discussed in W3C?
> 
> I think this should still work.  Does it not?

# Oops, I meant getComputedValue() is getComputedStyle().getPropertyValue() or getComputedStyle().getPropertyCSSValue().

I worry about CSS 3 text-decoration, not this bug's result. If we implemented CSS 3 text-decoration in the future under current rules, didn't the computed value become empty string for "text-decoration: underline;" due to shorthand property?

I think if we wanted to keep the compatibility, we should return decoration line value (or blink) when both decoration style and decoration color were initial value.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 527719 [details] [diff] [review]
Patch v1.1

sr=me

Comment 15

[David Baron :dbaron:]

No, 'text-decoration: underline' should always lead to a computed value that serializes back to 'underline'.  The shorthand resets the -color and -style to match exactly the case where we *do* serialize the shorthand; that's the point of checking for exactly that case.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/135b8bba35af

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #15)
> No, 'text-decoration: underline' should always lead to a computed value that
> serializes back to 'underline'.  The shorthand resets the -color and -style to
> match exactly the case where we *do* serialize the shorthand; that's the point
> of checking for exactly that case.

But it looks like invalid for this spec:
http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/css.html#CSS-CSSStyleDeclaration-getPropertyCSSValue
That said "This method returns null if the property is a shorthand property".

And shouldn't the computed values for "text-decoration: underline black;", "text-decoration: underline wavy;" and "text-decoration: underline black wavy;" become empty when -moz- prefix is removed?

Comment 18

[David Baron :dbaron:]

(In reply to comment #17)
> (In reply to comment #15)
> > No, 'text-decoration: underline' should always lead to a computed value that
> > serializes back to 'underline'.  The shorthand resets the -color and -style to
> > match exactly the case where we *do* serialize the shorthand; that's the point
> > of checking for exactly that case.
> 
> But it looks like invalid for this spec:
> http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/css.html#CSS-CSSStyleDeclaration-getPropertyCSSValue
> That said "This method returns null if the property is a shorthand property".

Yeah, that's a bad idea, so we shouldn't do it for properties that have changed from longhand to shorthand.

> And shouldn't the computed values for "text-decoration: underline black;",
> "text-decoration: underline wavy;" and "text-decoration: underline black wavy;"
> become empty when -moz- prefix is removed?

Why can't they just look like the specified values?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #18)
> (In reply to comment #17)
> > And shouldn't the computed values for "text-decoration: underline black;",
> > "text-decoration: underline wavy;" and "text-decoration: underline black wavy;"
> > become empty when -moz- prefix is removed?
> 
> Why can't they just look like the specified values?

I guess that authors want consistent result for computed values on all browsers. Then, the order of the values become a problem. Therefore, don't the spec want authors to use each longhand properties at getting their computed values?

Comment 20

[Florian Scholz (Open Web Docs)]

Updated
https://developer.mozilla.org/en/CSS/text-decoration#Notes

Added
https://developer.mozilla.org/en/CSS/text-decoration-line
https://developer.mozilla.org/en/CSS/-moz-text-blink

Mentioned on
https://developer.mozilla.org/en/Firefox_6_for_developers

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, the patch returns the computed value of text-decoration same as CSS 2. However, if the author set -moz-text-decoration-color or -moz-text-decoration-style to non-initial value, then, the author should *know* the text-decoration is a shorthand property. So, should we return empty string if -moz-text-decoration-color or -moz-text-decoration-style isn't initial value?

Comment 22

[David Baron :dbaron:]

(In reply to comment #21)
> Ah, the patch returns the computed value of text-decoration same as CSS 2.
> However, if the author set -moz-text-decoration-color or
> -moz-text-decoration-style to non-initial value, then, the author should *know*
> the text-decoration is a shorthand property. So, should we return empty string
> if -moz-text-decoration-color or -moz-text-decoration-style isn't initial
> value?

Yes.