Bug 73002 - Incorrect value of CSSStyleDeclaration#length when a shorthand property is inherit or initial
: Incorrect value of CSSStyleDeclaration#length when a shorthand property is in...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To: Alexis Menard (darktears)
http://jsfiddle.net/leaverou/SGf9v/show
:
Depends on: 103245
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-22 22:49 PST by Lea Verou
Modified: 2012-11-27 04:40 PST (History)
20 users (show)

See Also:


Attachments
Patch (23.36 KB, patch)
2012-01-12 09:56 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (21.20 KB, patch)
2012-11-21 04:29 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (21.70 KB, patch)
2012-11-23 06:22 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (26.12 KB, patch)
2012-11-26 06:34 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch for landing (26.13 KB, patch)
2012-11-27 03:58 PST, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lea Verou 2011-11-22 22:49:49 PST
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)
Comment 1 David Barr 2011-11-23 19:59:29 PST
Please clarify the expected result, I found that the value of length matched the number properties in the value of cssText when read back.
Comment 2 Lea Verou 2011-11-23 20:08:26 PST
(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.
Comment 3 Shane Stephens 2011-11-30 19:49:45 PST
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.
Comment 4 Alexis Menard (darktears) 2012-01-12 09:56:23 PST
Created attachment 122264 [details]
Patch
Comment 5 Alexis Menard (darktears) 2012-01-12 10:08:14 PST
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?
Comment 6 WebKit Review Bot 2012-01-12 10:57:52 PST
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
Comment 7 Alexis Menard (darktears) 2012-01-26 12:04:56 PST
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
Comment 8 David Barr 2012-02-15 19:29:11 PST
*** Bug 75538 has been marked as a duplicate of this bug. ***
Comment 9 Alexander Pavlov (apavlov) 2012-11-20 00:03:34 PST
*** Bug 102345 has been marked as a duplicate of this bug. ***
Comment 10 Alexander Pavlov (apavlov) 2012-11-21 04:29:30 PST
Created attachment 175409 [details]
Patch
Comment 11 Antti Koivisto 2012-11-22 06:36:22 PST
At some point all this serialization stuff should be factored out from StylePropertySet. It might belong to PropertySetCSSStyleDeclaration (assuming there are no internal clients).
Comment 12 Alexis Menard (darktears) 2012-11-23 04:56:44 PST
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 :)
Comment 13 Alexander Pavlov (apavlov) 2012-11-23 06:22:08 PST
Created attachment 175792 [details]
Patch
Comment 14 Alexander Pavlov (apavlov) 2012-11-26 06:34:59 PST
Created attachment 175995 [details]
Patch
Comment 15 Alexis Menard (darktears) 2012-11-27 03:39:06 PST
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.
Comment 16 Alexander Pavlov (apavlov) 2012-11-27 03:58:10 PST
Created attachment 176227 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-11-27 04:40:44 PST
Comment on attachment 176227 [details]
Patch for landing

Clearing flags on attachment: 176227

Committed r135848: <http://trac.webkit.org/changeset/135848>
Comment 18 WebKit Review Bot 2012-11-27 04:40:51 PST
All reviewed patches have been landed.  Closing bug.