Bug 73002 – Incorrect value of CSSStyleDeclaration#length when a shorthand property is inherit or initial

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Lea Verou]

Check testcase URL. Other browsers expand the shorthand even when the value is inherit or initial, at least for calculating the length property. Webkit does not.

(Not sure which category to file this in, as there's none for CSSOM)

[David Barr]

Please clarify the expected result, I found that the value of length matched the number properties in the value of cssText when read back.

[Lea Verou]

(In reply to comment #1)
> Please clarify the expected result, I found that the value of length matched the number properties in the value of cssText when read back.

When shorthands are set to any other value, they are expanded into their longhand properties, as can be observed in the first 2 examples. This is consistent in every browser, even though the exact counts may differ. 

Webkit differentiates its behavior when inherit and initial are used and doesn't expand the shorthand properties, thus causing inconsistency which can break scripts depending on the same count for any value. Every other browser tested consistently expanded shorthands regardless of their value, as can be observed in examples 3-6. 
If cssText differentiates its behavior in that way, then I would assume that's buggy too. The spec is a bit unclear as to what is supposed to happen, but the de facto standard behavior of the other UAs is different to what Webkit does.

[Shane Stephens]

We should seek clarification from the CSSWG as to the correct behavior here. In the meantime, I think Lea is right - WebKit should seek to conform with the de-facto standard.

[Alexis Menard (darktears)]

Created attachment 122264 [details]
Patch

[Alexis Menard (darktears)]

This is work in progress but I think it's the time to bring the discussion here.

Should the shorthands be part of the list of properties of the style, so part of the count/length? Implementation wise should we call CSSParser::addProperty.

Today if inherit or initial is set on a shorthant it will always be added to the property list but the longhands will not.
If the shorthand is correctly parsed only the longhands are added to the m_properties list of the style. It is valid for most of them but for example font: 15px arial,sans-serif expands the longhands, add them to m_properties and also add the font shorthand to the list. CSSMutableStyleDeclaration::fontValue even expect the font shorthand to be present to even do something.

const CSSProperty* fontSizeProperty = findPropertyWithId(CSSPropertyFontSize);

So let say to be consistent we also add the shorthands now even when successfully parsed, which value they should get when we call addProperty(). A unquoted string that we managed to parse?

Secondly, when we set initial to a shorthand will it mean that the longhands will have initial implicitely set or would it be explicitely?

[WebKit Review Bot]

Comment on attachment 122264 [details]
Patch

Attachment 122264 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11227108

New failing tests:
fast/css/shorthands-expand-when-inherit-or-initial.html
inspector/styles/styles-new-API.html

[Alexis Menard (darktears)]

Comment on attachment 122264 [details]
Patch

Clearing review for now.

I asked clarification on the working group : http://lists.w3.org/Archives/Public/www-style/2012Jan/1122.html

[David Barr]

*** Bug 75538 has been marked as a duplicate of this bug. ***

[Alexander Pavlov (apavlov)]

*** Bug 102345 has been marked as a duplicate of this bug. ***

[Alexander Pavlov (apavlov)]

Created attachment 175409 [details]
Patch

[Antti Koivisto]

At some point all this serialization stuff should be factored out from StylePropertySet. It might belong to PropertySetCSSStyleDeclaration (assuming there are no internal clients).

[Alexis Menard (darktears)]

Comment on attachment 175409 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175409&action=review

> Source/WebCore/ChangeLog:14
> +        When reconstructing shorthands, a "universal value" is tracked. If all longhands involved have the same explicit value, it becomes the

Universal is a strange name but I don't have better suggestion. What we are trying to say is that all longhands should have the same value. So maybe "common value" is tracked.

> Source/WebCore/css/CSSParser.cpp:1746
> +                addProperty(longhands[i], initialValue, important);

These two blocks of code are almost identical and the only difference is rather you add the longhands with initial or inherit. Could you factorize the code into a function on its own?

> LayoutTests/css3/flexbox/flex-property-parsing-expected.txt:110
> +PASS flexitem.style.webkitFlex is "0 0 auto"

Cool :)

[Alexander Pavlov (apavlov)]

Created attachment 175792 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 175995 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 175995 [details]
Patch

It makes the length of the style finally consistent no matter if you set inherit, initial, or any value. It matches Firefox and Opera (at least for inherit, the latter seems to not expand initial). We are not patching the computed style here but the declaration so "initial" and "inherit" are possible return values in that case. Indeed we're the only browser supporting that : for example elem.style.background = "inherit"; elem.style.background will return "inherit" as is today and it's valid for any other property. Antti has a good suggestion on moving out the property expansion and construction out of StylePropertySet if there is no internal clients and I think it could be done (provided it's feasible) in a latter patch, moving altogether what's need to be moved.

[Alexander Pavlov (apavlov)]

Created attachment 176227 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 176227 [details]
Patch for landing

Clearing flags on attachment: 176227

Committed r135848: <http://trac.webkit.org/changeset/135848>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.