652486 - Computed value of text-decoration should be empty when its style or color is not initial value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

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

Attachment: Patch v1.0

Comment 2

[David Baron :dbaron:]

I'd have wanted the result, at least for the string form, to be the empty string rather than null.  Is there an easy way to do that?

Also, it's better to call GetStyleTextReset() only once and store the result in a local variable.

Comment 3

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

(In reply to comment #2)
> I'd have wanted the result, at least for the string form, to be the empty
> string rather than null.  Is there an easy way to do that?

If I return empty string from DoGetTextDecoration(), getPropertyCSSValue() returns non-null object which string value is empty. Is this your intentional behavior?

It looks like bad behavior:
http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/css.html#CSS-CSSStyleDeclaration-getPropertyCSSValue

Comment 4

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

dbaron:

How do you think about the comment 3?

Comment 5

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

Attachment: Patch v1.1

See comment 2 before review. I don't change the result of DoGetTextDecoration() from null to empty string.

Comment 6

[David Baron :dbaron:]

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

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>+  else if (eCSSUnit_EnumColor == decorationColorValue->GetUnit() &&
>+           decorationColorValue->GetIntValue() == NS_COLOR_CURRENTCOLOR) {
>+    text->SetDecorationColorToForeground();
>+  }
>   else if (SetColor(*decorationColorValue, 0, mPresContext, aContext,
>                     decorationColor, canStoreInRuleTree)) {
>     text->SetDecorationColor(decorationColor);
>   }
>   else if (eCSSUnit_Initial == decorationColorValue->GetUnit() ||
>            eCSSUnit_Enumerated == decorationColorValue->GetUnit()) {

here you should remove the eCSSUnit_Enumerated check, and the NS_ABORT_IF_FALSE, since you handle that case above now.

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

r=dbaron with that

Comment 7

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

(In reply to comment #6)
> >diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
> >+  else if (eCSSUnit_EnumColor == decorationColorValue->GetUnit() &&
> >+           decorationColorValue->GetIntValue() == NS_COLOR_CURRENTCOLOR) {
> >+    text->SetDecorationColorToForeground();
> >+  }
> >   else if (SetColor(*decorationColorValue, 0, mPresContext, aContext,
> >                     decorationColor, canStoreInRuleTree)) {
> >     text->SetDecorationColor(decorationColor);
> >   }
> >   else if (eCSSUnit_Initial == decorationColorValue->GetUnit() ||
> >            eCSSUnit_Enumerated == decorationColorValue->GetUnit()) {
> 
> here you should remove the eCSSUnit_Enumerated check, and the
> NS_ABORT_IF_FALSE, since you handle that case above now.

No, the above handles EnumColor, not Enum. When I changed so, -moz-use-text-color doesn't work fine (some tests failed).

I assume that you misread the enum name. I'll land it without this change if you're disagree, I'll create a followup patch.

Comment 8

[David Baron :dbaron:]

Yes, I misread the enum name.  It's fine as it is.

Comment 9

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

http://hg.mozilla.org/mozilla-central/rev/fc0c5b6e2df3

Thank you.