Last Comment Bug 652486 - Computed value of text-decoration should be empty when its style or color is not initial value
: Computed value of text-decoration should be empty when its style or color is ...
Status: RESOLVED FIXED
: css3
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
Depends on:
Blocks: 647421
  Show dependency treegraph
 
Reported: 2011-04-24 18:42 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
Modified: 2011-05-19 22:59 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (10.51 KB, patch)
2011-04-24 22:40 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v1.1 (11.97 KB, patch)
2011-05-11 18:57 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
dbaron: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-04-24 18:42:45 PDT

    
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-04-24 22:40:48 PDT
Created attachment 528061 [details] [diff] [review]
Patch v1.0
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-04-24 22:51:38 PDT
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] (Mozilla Japan) (working slowly due to injured) 2011-04-25 00:26:37 PDT
(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] (Mozilla Japan) (working slowly due to injured) 2011-05-05 17:44:18 PDT
dbaron:

How do you think about the comment 3?
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-05-11 18:57:06 PDT
Created attachment 531828 [details] [diff] [review]
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: ⌚️UTC-7 (review requests must explain patch) 2011-05-19 12:44:15 PDT
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] (Mozilla Japan) (working slowly due to injured) 2011-05-19 19:27:41 PDT
(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: ⌚️UTC-7 (review requests must explain patch) 2011-05-19 19:43:13 PDT
Yes, I misread the enum name.  It's fine as it is.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-05-19 22:59:07 PDT
http://hg.mozilla.org/mozilla-central/rev/fc0c5b6e2df3

Thank you.

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