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
: WebKit
CSS
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To:
: http://jsfiddle.net/leaverou/SGf9v/show
:
: 103245
:
  Show dependency treegraph
 
Reported: 2011-11-22 22:49 PST by
Modified: 2012-11-27 04:40 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 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 From 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 From 2012-01-12 09:56:23 PST -------
Created an attachment (id=122264) [details]
Patch
------- Comment #5 From 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 From 2012-01-12 10:57:52 PST -------
(From update of attachment 122264 [details])
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 From 2012-01-26 12:04:56 PST -------
(From update of attachment 122264 [details])
Clearing review for now.

I asked clarification on the working group : http://lists.w3.org/Archives/Public/www-style/2012Jan/1122.html
------- Comment #8 From 2012-02-15 19:29:11 PST -------
*** Bug 75538 has been marked as a duplicate of this bug. ***
------- Comment #9 From 2012-11-20 00:03:34 PST -------
*** Bug 102345 has been marked as a duplicate of this bug. ***
------- Comment #10 From 2012-11-21 04:29:30 PST -------
Created an attachment (id=175409) [details]
Patch
------- Comment #11 From 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 From 2012-11-23 04:56:44 PST -------
(From update of attachment 175409 [details])
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 From 2012-11-23 06:22:08 PST -------
Created an attachment (id=175792) [details]
Patch
------- Comment #14 From 2012-11-26 06:34:59 PST -------
Created an attachment (id=175995) [details]
Patch
------- Comment #15 From 2012-11-27 03:39:06 PST -------
(From update of attachment 175995 [details])
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 From 2012-11-27 03:58:10 PST -------
Created an attachment (id=176227) [details]
Patch for landing
------- Comment #17 From 2012-11-27 04:40:44 PST -------
(From update of attachment 176227 [details])
Clearing flags on attachment: 176227

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