930665 - domUtils.getCSSValuesForProperty("text-decoration") only returns color values.

Status: RESOLVED DUPLICATE of bug 1156019

(Core :: CSS Parsing and Computation, defect)

Description

[Tim Nguyen :ntim]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131024030204

Steps to reproduce:

Open the inspector
Add a new text-decoration rule



Actual results:

It returns all color values


Expected results:

It should return the correct values (underline,...)

Comment 1

[Girish Sharma [:Optimizer]]

ughh. looks like domUtils.getCSSValuesForProperty("text-decoration") returns color values ..

CC'ing bz and Mina.

Comment 2

[Boris Zbarsky [:bzbarsky]]

http://dev.w3.org/csswg/css-text-decor-3/#text-decoration color values are in fact valid values for text-decoration.  See the "text-decoration: navy dotted underline;" example there.

That doesn't seem to be what Gecko implements so far, but we do mark text-decoration as a shorthand for a color property (-moz-text-decoration-color), since it will reset that property.  So the code that expands out valid values to the union of shorthand values sees the color values.

David, is the long-term plan to implement the draft as above?  Because if so, then the current autocomplete behavior will in fact become correct at that point, as far as I can tell.   Useless as it is for common text-decoration uses.  :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

And maybe at that point the right answer will be to not sort the whole list alphabetically but instead to group it by some sort of functional meanin, with the decoration-style values first and the colors after?

Comment 4

[Girish Sharma [:Optimizer]]

But the list does not even contain "underline".

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah, that's because the parsing for -moz-text-decoration-line is custom parsing, not table-driven.  In particular, the parser variant is 0, which does not include VARIANT_KEYWORD, so getCSSValuesForProperty ignores the keyword table.

David, would it make sense to set VARIANT_HK here even though we're CSS_PROPERTY_VALUE_PARSER_FUNCTION?  If not, we're going to need to do some sort of special-casing in getCSSValuesForProperty or something....

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems a little scary to go down that path, for two reasons:

 (1) Code that expects the variant field to have a particular meaning will find values that are less and less accurate if we start adding the variant data to properties where it isn't actually accurate.  If we start doing this, when do we stop?  I'd like to at least know what the rules are.

 (2) Related to (1), I think the invariant that the variant field is only set if it's accurate is a reasonable one.  But again, if we're going to change it, I'd like to know what the new rule is.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The thing is, VARIANT_HK is in fact accurate in this case, I believe.  If you look at the code in ParseTextDecorationLine it in fact does exactly what you'd expect from an array of keywords except for the special handling for NS_STYLE_TEXT_DECORATION_LINE_NONE and disallowed keyword duplication (which is what leads us to have a custom parser here at all).

But I can see the argument for not setting it at all for VALUE_PARSER_FUNCTION properties.  In which case, the only way to make the work with getCSSValuesForProperty is to special-case them all, right?

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> The thing is, VARIANT_HK is in fact accurate in this case, I believe.  If
> you look at the code in ParseTextDecorationLine it in fact does exactly what
> you'd expect from an array of keywords except for the special handling for
> NS_STYLE_TEXT_DECORATION_LINE_NONE and disallowed keyword duplication (which
> is what leads us to have a custom parser here at all).
> 
> But I can see the argument for not setting it at all for
> VALUE_PARSER_FUNCTION properties.  In which case, the only way to make the
> work with getCSSValuesForProperty is to special-case them all, right?

Does this mean that we will have issues with inDOMUtils::GetCSSValuesForProperty(), inDOMUtils::.CssPropertySupportsType() etc. for all properties and values under CSSParserImpl::ParsePropertyByFunction() ( http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#9538 )

Comment 9

[Boris Zbarsky [:bzbarsky]]

Sort of.

Some of those are shorthands, and both GetCSSValuesForProperty and CssPropertySupportsType examine the longhands in that case, so there would be no problem.

But yes, for the longhands some of those may have parser variants that say something useful if their custom parsing function happens to use the variant but there are no guarantees that they do.

In practice, the list of things that will have issues is:

-moz-outline-radius-topleft, -moz-outline-radius-topright, -moz-outline-radius-bottomright, -moz-outline-radius-bottomleft, background-position, background-repeat, background-size, -moz-border-bottom-colors, border-image-slice, border-image-width, border-image-outset, border-image-repeat, -moz-border-left-colors, -moz-border-right-colors, border-spacing, -moz-border-top-colors, border-top-left-radius, border-top-right-radius, border-bottom-right-radius, border-bottom-left-radius, box-shadow, clip, content, counter-increment, counter-reset, cursor, grid-auto-flow, grid-auto-columns, grid-auto-rows, grid-template-areas, grid-template-columns, grid-template-rows, grid-column-start, grid-column-end, grid-row-start, grid-row-end, -moz-image-region, paint-order, quotes, size, text-shadow, transform, transform-origin, perspective-origin, transition-property, fill, filter, stroke, stroke-dasharray, will-change

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er, no, that list is missing things that have CSS_PROPERTY_VALUE_PARSER_FUNCTION, like -moz-text-decoration-line.

Comment 11

[Tom Tromey :tromey]

I believe this was fixed in bug 1156019.