Closed Bug 1039488 Opened 10 years ago Closed 10 years ago

support the full css3-text-decor syntax for the 'text-decoration' shorthand rather than only the CSS2.1 syntax

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: sebo, Assigned: arai)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

Turn 'text-decoration' into a shorthand.
30.46 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
As of the CSS Text Decoration Module Level 3 the 'text-decoration' property is now used as a shorthand for 'text-decoration-line', 'text-decoration-style' and 'text-decoration-color'.[1]

While the longhand properties were implemented in bug 59109, 'text-decoration' wasn't changed to allow having multiple values (like e.g. 'navy dotted underline') assigned yet.

Sebastian

[1] http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property
Blocks: css3test
If another testcases are needed, let me know.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a19dbcea770a
(pushed the patches in bug 825004 first)
Attachment #8522114 - Flags: review?(dbaron)
Comment on attachment 8522114 [details] [diff] [review]
Turn 'text-decoration' into a shorthand.

>-  enum {
>-    eDecorationNone         = NS_STYLE_TEXT_DECORATION_LINE_NONE,
>-    eDecorationUnderline    = NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE,
>-    eDecorationOverline     = NS_STYLE_TEXT_DECORATION_LINE_OVERLINE,
>-    eDecorationLineThrough  = NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH,
>-    eDecorationBlink        = NS_STYLE_TEXT_DECORATION_LINE_BLINK,
>-    eDecorationPrefAnchors  = NS_STYLE_TEXT_DECORATION_LINE_PREF_ANCHORS
>   };
>-  static_assert((eDecorationNone ^ eDecorationUnderline ^
>-                 eDecorationOverline ^ eDecorationLineThrough ^
>-                 eDecorationBlink ^ eDecorationPrefAnchors) ==
>-                (eDecorationNone | eDecorationUnderline |
>-                 eDecorationOverline | eDecorationLineThrough |
>-                 eDecorationBlink | eDecorationPrefAnchors),
>-                "text decoration constants need to be bitmasks");

Instead of removing this assertion, could you move it to
CSSParserImpl::ParseTextDecorationLine ?  (It's probably best to remove
the enum and just substitute the values straight into the assertion,
though.)

>+  nsCSSValue  values[numProps];

Change the two spaces between nsCSSValue and values to one space.

>+  int32_t index;
>+  for (index = 0; index < numProps; index++) {

Move the declaration of index inside the for().


In property_database.js:

>-                      "underline red solid", "underline #ff0000", "solid underline", "red underline", "#ff0000 underline" ]

Please move these from invalid_values to other_values instead of removing them.

r=dbaron with those changes
Attachment #8522114 - Flags: review?(dbaron) → review+
Assignee: nobody → arai_a
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Turn 'text-decoration' into a shorthand → support the full css3-text-decor syntax for the 'text-decoration' shorthand rather than only the CSS2.1 syntax
Thank you for reviewing.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #2)
> Comment on attachment 8522114 [details] [diff] [review]
> Turn 'text-decoration' into a shorthand.
> 
> In property_database.js:
> 
> >-                      "underline red solid", "underline #ff0000", "solid underline", "red underline", "#ff0000 underline" ]
> 
> Please move these from invalid_values to other_values instead of removing
> them.

Sorry, I misunderstood around CSS2.1 compatibility.
it seems that testsuite needs some dedicated codes to move them into other_values,
because text-decoration returns empty and non-empty string, depending on its current value.
I'll prepare them.
I noticed that node.style.getPropertyValue("text-decoration") returns empty string if style and color is not initial value,
but it should also be fixed by this bug, sorry for the oversight.

Computed value also returns empty string for getPropertyValue("text-decoration"),
but I think it should be addressed by bug 137688.


I implemented following serialization for text-decoration in Declaration::GetValue
  1. if both of style and color are initial value, return line value,
     for comptibility with CSS 2.1
  2. if style is not initial value but color is initial value,
     return line and style values,
     to avoid returning -moz-use-text-color.
  3. otherwise, return line, style, and color values


About the testsuite, I added following properties to property_database.js

> //   transition_values: Values contained in other_values and will cause
> //     transition. (text-decoration only)
Used by test_transitions_per_property.html to avoid transition.
For text-decoration, values which has color.

> //   shorthand_values:  Values contained in other_values and will make this a
> //     shorthand property. (text-decoration only)
Used by test_value_storage.html to avoid calcurating computed value.
For text-decoration, values which has non-initial style or color.
(I guess this could be removed after bug 137688 was fixed)


Currently, test_transitions_per_property.html does not test transition
of text-decoration, because it needs bug 137688 to be fixed to get computed
value of text-decoration as a shorthand property.


Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=95ff037e04a1

dbaron, could you review those part?
Attachment #8522114 - Attachment is obsolete: true
Attachment #8523352 - Flags: review?(dbaron)
Why did you choose to add extra fields to property_database.js for text-decoration only?  I'd like to keep property_database.js relatively uniform.

(I could probably figure this out for myself in the morning (when I will hopefully get a chance to look at the patch more closely), but I suspect you might have a useful answer.)

Also, if the reason was failures in test_transitions_per_property.html, you probably need to add an entry for text-decoration to test_transitions_per_property.html to test animations of text-decoration-color.  That test tests that transitions are not supported for all the properties that it doesn't have tests for; the fix there would be to add a test for text-decoration.  In fact, you probably should do this even if that wasn't the reason.
Flags: needinfo?(arai_a)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #5)
> Why did you choose to add extra fields to property_database.js for
> text-decoration only?  I'd like to keep property_database.js relatively
> uniform.
> 
> (I could probably figure this out for myself in the morning (when I will
> hopefully get a chance to look at the patch more closely), but I suspect you
> might have a useful answer.)
> 
> Also, if the reason was failures in test_transitions_per_property.html, you
> probably need to add an entry for text-decoration to
> test_transitions_per_property.html to test animations of
> text-decoration-color.  That test tests that transitions are not supported
> for all the properties that it doesn't have tests for; the fix there would
> be to add a test for text-decoration.  In fact, you probably should do this
> even if that wasn't the reason.

Thanks, yes, about transition_values, I added it because of the failures in test_transitions_per_property.html.
I thought text-decoration should be tested in both way, supported/non-supported,
because CSS2.1 text-decoration does not support it,
but it seems to be wrong.
(You mean that it's not required to test it as non-supported property, right?)

Then, about animation of text-decoration,
computed value becomes an empty string when setting color to non-initial value
because of bug 137688, and we cannot test it by comparing computed values,
otherwise getting the value from text-decoration-color.
text-decoration-color is already tested there, so I thought it's sufficient for now.
But it may be better to add partial test, I'll add it.


For shorthand_values, I added it because of the failures in test_value_storage.html.
This is also because of bug 137688,
values with non-initial color or styles make text-decoration a shorthand property
and make its computed value an empty string,
So, in test_value_storage.html, we should apply same workaround as CSS_TYPE_TRUE_SHORTHAND,
to avoid testing computed value as a storage.

Then, I decided to add it to property_database.js
becase shorthand_values is a subset of other_values,
and there is no simple way to calculate it from other_values automatically
in test_value_storage.html.
I guess someone may add another entry to other_values later.

So, to keep property_database.js uniform,
it may be better to add some comment around text-decoration's other_values
(something like "please sync this value with test_value_storage.html")
and move shorthand_values to test_value_storage.html.


I'll prepare a patch with above 2 fixes.
Flags: needinfo?(arai_a)
Added text-decoration to supported_properties in test_transitions_per_property.html,
with test_color_shorthand_transition and test_border_color_shorthand_transition.
I re-used test_color_transition and test_border_color_transition for them,
by adding extra default parameter "sub", which is the suffix of the subproperty to get computed value of the color (e.g. "-color" for text-decoration).
I guess we can add some other shorthand property to supported_properties in same way (not sure it's needed ot not).

Also, moved shorthand_values to test_value_storage.html.


Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e992b6c8f04e
Attachment #8523352 - Attachment is obsolete: true
Attachment #8523352 - Flags: review?(dbaron)
Attachment #8523420 - Flags: review?(dbaron)
Comment on attachment 8523420 [details] [diff] [review]
Turn 'text-decoration' into a shorthand.

>     case eCSSProperty_text_decoration: {
>-      // If text-decoration-color or text-decoration-style isn't initial value,
>-      // we cannot serialize the text-decoration shorthand value.
>       const nsCSSValue *decorationColor =
>         data->ValueFor(eCSSProperty_text_decoration_color);
>       const nsCSSValue *decorationStyle =
>         data->ValueFor(eCSSProperty_text_decoration_style);
> 
>       NS_ABORT_IF_FALSE(decorationStyle->GetUnit() == eCSSUnit_Enumerated,
>                         nsPrintfCString("bad text-decoration-style unit %d",
>                                         decorationStyle->GetUnit()).get());
> 
>-      if (decorationColor->GetUnit() != eCSSUnit_Enumerated ||
>-          decorationColor->GetIntValue() != NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR ||
>-          decorationStyle->GetIntValue() !=
>-            NS_STYLE_TEXT_DECORATION_STYLE_SOLID) {
>+      bool isMozUseTextColor =
>+        decorationColor->GetUnit() == eCSSUnit_Enumerated &&
>+        decorationColor->GetIntValue() == NS_STYLE_COLOR_MOZ_USE_TEXT_COLOR;
>+      bool isSolid =
>+        decorationStyle->GetIntValue() == NS_STYLE_TEXT_DECORATION_STYLE_SOLID;
>+      if (!isSolid || !isMozUseTextColor) {
>+        if (!AppendValueToString(eCSSProperty_text_decoration_line, aValue,
>+                                 aSerialization) ||
>+            !(aValue.Append(char16_t(' ')),
>+              AppendValueToString(eCSSProperty_text_decoration_style, aValue,
>+                                  aSerialization)) ||
>+            // Don't output color value when it's -moz-use-text-color.
>+            !(isMozUseTextColor ||
>+              (aValue.Append(char16_t(' ')),
>+               AppendValueToString(eCSSProperty_text_decoration_color, aValue,
>+                                   aSerialization)))) {
>+          aValue.Truncate();
>+        }
>         return;
>       }
> 
>+      // If text-decoration-color and text-decoration-style are initial value,
>+      // serialize only text-decoration-line, for compatibility.
>       AppendValueToString(eCSSProperty_text_decoration_line, aValue,
>                           aSerialization);

A few things here:

 (1) there's no point checking the return value of AppendValueToString when we know the property exists in the declaration.

 (2) it's probably better to skip serializing the style as well if it's the default

Together, these suggest that you should be able to merge the two branches and simplify the code.  Just:
 (1) serialize the line
 (2) serialize the style if it is non-default (with a space before)
 (3) serialize the color if it is non-default (with a space before)


>+// Values contained in other_values and make the property a shorthand.
>+var shorthand_values = {
>+  "text-decoration": [ "underline red solid", "underline #ff0000", "red underline", "#ff0000 underline", "dotted underline" ],
>+};

This still seems like a little bit too much of a special case.

I suppose CSS_TYPE_SHORTHAND_AND_LONGHAND can mean two different things, and should be split into two things:

  // Properties that we handle as shorthands but were previously longhands, so they always have a computed value.
  CSS_TYPE_SHORTHAND_ALWAYS_HAS_COMPUTED

  // Properties that we handle as shorthands but were previously longhands, and which sometimes (when expressible) has a computed value.
  CSS_TYPE_SHORTHAND_SOMETIMES_HAS_COMPUTED


However, maybe that's not needed here.  Why not just change nsComputedDOMStyle::DoGetTextDecoration so that it does basically the same thing that your changes in Declaration.cpp do, and just always produce a value.  Then you shouldn't need to worry about test_value_storage changes or splitting up CSS_TYPE_SHORTHAND_AND_LONGHAND, and it's probably more consistent as well.


Otherwise this looks great -- thanks for sticking with these remaining issues.  Hopefully one more revision should be enough.
Attachment #8523420 - Flags: review?(dbaron) → review-
Thanks again!

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #8)
> A few things here:
> 
>  (1) there's no point checking the return value of AppendValueToString when
> we know the property exists in the declaration.
> 
>  (2) it's probably better to skip serializing the style as well if it's the
> default
> 
> Together, these suggest that you should be able to merge the two branches
> and simplify the code.  Just:
>  (1) serialize the line
>  (2) serialize the style if it is non-default (with a space before)
>  (3) serialize the color if it is non-default (with a space before)

Thanks, nice solution :)

> However, maybe that's not needed here.  Why not just change
> nsComputedDOMStyle::DoGetTextDecoration so that it does basically the same
> thing that your changes in Declaration.cpp do, and just always produce a
> value.  Then you shouldn't need to worry about test_value_storage changes or
> splitting up CSS_TYPE_SHORTHAND_AND_LONGHAND, and it's probably more
> consistent as well.

Correct me if I'm wrong.

DoGet* methods are called by GetPropertyCSSValue, and GetPropertyValue is implemented on GetPropertyCSSValue.

http://hg.mozilla.org/mozilla-central/file/5d633c6a5f89/layout/style/nsComputedDOMStyle.cpp#l359
> NS_IMETHODIMP
> nsComputedDOMStyle::GetPropertyValue(const nsAString& aPropertyName,
>                                      nsAString& aReturn)
> {
>   aReturn.Truncate();
> 
>   ErrorResult error;
>   nsRefPtr<CSSValue> val = GetPropertyCSSValue(aPropertyName, error);

Then, (old) spec says that GetPropertyCSSValue returns null for shorthand properties.

http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleDeclaration
> getPropertyCSSValue
>     Used to retrieve the object representation of the value of a CSS property
>     if it has been explicitly set within this declaration block. This method
>     returns null if the property is a shorthand property. Shorthand property
>     values can only be accessed and modified as strings, using the
>     getPropertyValue and setProperty methods.

If I modify DoGetTextDecoration same as Declaration.cpp, GetPropertyCSSValue returns non-null value for shorthand property, and this does not conform to the spec.
I thought this is what bug 137688 says, isn't it?


Of course, getPropertyCSSValue is deprecated, so it would be better to fix getPropertyValue even if it breaks getPropertyCSSValue or introduce some extra bits to keep compatibility.
But I'm not sure it should be done here, only for text-decoration.

If we can drop compatibility, we can just return shorthand property value in nsROCSSPrimitiveValue from DoGet* methods.

To keep compatibility, I guess we can introduce new CSSValue for shorthand property value,
and return it from DoGet* methods for shorthand property.
Then, add extra parameter to GetPropertyCSSValue which holds "getPropertyValue or not",
and return null if it's not getPropertyValue and the value is a shorthand property value.
(may have to make one more function to keep GetPropertyCSSValue interface)


What do you think is the best way?
Flags: needinfo?(dbaron)
(In reply to Tooru Fujisawa [:arai] from comment #9)
> DoGet* methods are called by GetPropertyCSSValue, and GetPropertyValue is
> implemented on GetPropertyCSSValue.

Correct.

> Then, (old) spec says that GetPropertyCSSValue returns null for shorthand
> properties.

This doesn't make sense for properties that are being converted to shorthands but were previously longhands; it would break Web content.

A future spec should address this problem, and we have the opportunity to influence exactly how it does so by the way we make our implementation decisions.

It seems likely to be better for authors to be consistent about always serializing text-decoration, rather than by doing the odd thing that nsComputedDOMStyle::DoGetTextDecoration currently does.

> If I modify DoGetTextDecoration same as Declaration.cpp, GetPropertyCSSValue
> returns non-null value for shorthand property, and this does not conform to
> the spec.

For this one property, yes.  There's no reasonable way to conform to the spec, so we should not do so.

> I thought this is what bug 137688 says, isn't it?

It's saying to make the change for all shorthands.  We might want to do this at some point, but not as part of this bug!  Whether we want to do that depends on how other browsers behave (which affects whether there's a compatibility risk) and it depends on how important we think fixing it is for authors.


> Of course, getPropertyCSSValue is deprecated, so it would be better to fix
> getPropertyValue even if it breaks getPropertyCSSValue or introduce some
> extra bits to keep compatibility.
> But I'm not sure it should be done here, only for text-decoration.

We already half-break it for text-decoration.  I think we should just do so in all cases.

> If we can drop compatibility, we can just return shorthand property value in
> nsROCSSPrimitiveValue from DoGet* methods.

For just text-decoration, yes.  Doing more should happen in other bugs (maybe bug 137688), if we think doing more is a good idea, which I'm not sure about right now.

> To keep compatibility, I guess we can introduce new CSSValue for shorthand
> property value,

Don't worry much about the data structures returned by GetPropertyCSSValue; what matters is the string returned from GetPropertyValue.  Just use GetROCSSValueList(false) for any case where you have to serialize more than a line value.  (It's probably good to keep the line value using the data structures it uses now, though.)

> Then, add extra parameter to GetPropertyCSSValue which holds
> "getPropertyValue or not",
> and return null if it's not getPropertyValue and the value is a shorthand
> property value.
> (may have to make one more function to keep GetPropertyCSSValue interface)

Don't do any of this, and for this bug please change only text-decoration.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #10)
> (In reply to Tooru Fujisawa [:arai] from comment #9)
> > Then, add extra parameter to GetPropertyCSSValue which holds
> > "getPropertyValue or not",
> > and return null if it's not getPropertyValue and the value is a shorthand
> > property value.
> > (may have to make one more function to keep GetPropertyCSSValue interface)
> 
> Don't do any of this, and for this bug please change only text-decoration.

In case it wasn't clear, the "Don't do any of this" was referring to just the quoted five lines of text.
Thank you for detailed explanation :)

Okay, I'll change DoGetTextDecoration to return nsDOMCSSValueList if style or color is non-initial value.

test_value_storage.html now needs no fix :)

Then, in test_transitions_per_property.html,
I'm going to skip check_distance test for text-decoration,
because SpecialPowers.DOMWindowUtils.computeAnimationDistance does not support shorthand property now.
is it okay? or should it be addressed before this bug?

I'll post updated patch shortly (now testing).
(In reply to Tooru Fujisawa [:arai] from comment #12)
> Then, in test_transitions_per_property.html,
> I'm going to skip check_distance test for text-decoration,
> because SpecialPowers.DOMWindowUtils.computeAnimationDistance does not
> support shorthand property now.
> is it okay? or should it be addressed before this bug?

That sounds fine.
Comment on attachment 8523720 [details] [diff] [review]
Turn 'text-decoration' into a shorthand.

test_bug652486.html:

> /** Test for Bug 652486 **/

Probably change this to say "and Bug 1039488".


r=dbaron with that, and thanks.


Do you need somebody to land this patch and the ones in the related bugs for you, or can you land them?
Attachment #8523720 - Flags: review?(dbaron) → review+
Thank you!
I got tree access today, so I'll land those patches after I read some documents :)
sorry, they are commits for bug 825004.

This is for this bug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4494fa5c82e
https://hg.mozilla.org/mozilla-central/rev/e4494fa5c82e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated following documents:
  https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration
  https://developer.mozilla.org/en-US/Firefox/Releases/36

Site Compatibility page will be updated after Firefox 36 Aurora is released.
:arai : thank you! I made minor changes but nothing fundamental.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: