Closed Bug 652486 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: css3)

Attachments

(1 file, 1 obsolete file)

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.
(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
Attached patch Patch v1.1Splinter Review
See comment 2 before review. I don't change the result of DoGetTextDecoration() from null to empty string.
Attachment #528061 - Attachment is obsolete: true
Attachment #528061 - Flags: review?(dbaron)
Attachment #531828 - Flags: review?(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
Attachment #531828 - Flags: review?(dbaron) → review+
(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.
Yes, I misread the enum name.  It's fine as it is.
http://hg.mozilla.org/mozilla-central/rev/fc0c5b6e2df3

Thank you.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.