Bug 110011 - transition properties can't be found in CSSStyleDeclaration
Summary: transition properties can't be found in CSSStyleDeclaration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL: http://jsfiddle.net/syoichi/nnAey/
Keywords: InRadar
Depends on:
Blocks: 93136 156944
  Show dependency treegraph
 
Reported: 2013-02-16 06:36 PST by Syoichi Tsuyuhara
Modified: 2016-05-11 10:08 PDT (History)
15 users (show)

See Also:


Attachments
Patch (31.06 KB, patch)
2013-02-28 12:55 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (30.96 KB, patch)
2013-03-01 11:04 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Syoichi Tsuyuhara 2013-02-16 06:36:11 PST
On Chromium 27.0.1414.0 (182962), transition properties can't be found in CSSStyleDeclaration.
But prefixed transition properties can be found.

transition properties
http://jsfiddle.net/syoichi/nnAey/
prefixed transition properties
http://jsfiddle.net/syoichi/E2nDn/

And, by this bug, it seems that Web Inspector's Suggest box doesn't suggest transition properties in Styles pane of Elements panel.
Comment 1 Radar WebKit Bug Importer 2013-02-17 12:19:37 PST
<rdar://problem/13232242>
Comment 2 Michał Gołębiowski-Owczarek 2013-02-17 15:39:38 PST
I can confirm it on both Chrome Canary 27.0.1415.0 and WebKit nightly r143127.
Comment 3 Alexis Menard (darktears) 2013-02-28 12:55:06 PST
Created attachment 190788 [details]
Patch
Comment 4 Dean Jackson 2013-03-01 09:59:15 PST
Comment on attachment 190788 [details]
Patch

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

Minor typos and grammar in changelog.

I'm happy with the approach. As you said on IRC, antti or kling should probably double check.

> Source/WebCore/ChangeLog:11
> +        identically parse the two versions which means that when we populate StylePropertySet

Suggestion: break sentence after "two versions". e.g "..parse the two versions. This means that..."

> Source/WebCore/ChangeLog:15
> +        prefixed), each properties have their own id and we know handle them in

Suggestion: again, break after "prefixed)". And then "Each property has its own id and we NOW handle them..." (typo for know/now)

> Source/WebCore/ChangeLog:16
> +        the parsing code as disctints properties and add both versions to the

typo: distinct (singular)

> Source/WebCore/ChangeLog:19
> +        prefixed and the unprefixed entries. Last point the style resolution

Typo: "Last point" -> "Finally,"

> Source/WebCore/ChangeLog:22
> +        and the unprefixed versions are not resolved. This is to avoid creating
> +        two times animation objects for the resolved style.

"This is to avoid creating the animation objects two times for the resolved style"

> Source/WebCore/ChangeLog:34
> +        (WebCore::CSSParser::parseTransitionShorthand): add both prefixed and

Nit, "add" -> "Add"

> Source/WebCore/ChangeLog:52
> +        (WebCore::StylePropertySet::removePrefixedOrUnprefixedProperty): Remove
> +        also the prefixed or unprefixed shorthand if it exists.

Nit: "Also remove..."

> Source/WebCore/ChangeLog:58
> +        (WebCore::StylePropertySet::setPrefixedOrUnPrefixedProperty): If it
> +        exists a unprefixed or prefixed counterpart of the property we're
> +        trying to set then we update the other one.

"If a unprefixed or prefixed counterpart of the the property we're trying to set exists then we update the other one"

> Source/WebCore/ChangeLog:74
> +        unprefixed property we do not want to resolve two times and create

"do not want to resolve twice and create..."

> Source/WebCore/css/CSSParser.cpp:1539
> +void CSSParser::addPrefixedAndUnPrefixedProperty(CSSPropertyID propId, PassRefPtr<CSSValue> value, bool important, bool implicit)

This name is a little cumbersome, but I'm not coming up with great alternatives.

addPropertyWithPrefixingVariant

> Source/WebCore/css/CSSProperty.h:95
> +inline CSSPropertyID prefixedOrUnPrefixedPropertyId(CSSPropertyID propId)

Similarly, how about

prefixingVariantForPropertyId

> Source/WebCore/css/StylePropertySet.cpp:588
> +    bool ret = removePropertiesInSet(shorthand.properties(), shorthand.length());
> +
> +    CSSPropertyID prefixedOrUnprefixedID = prefixedOrUnPrefixedPropertyId(propertyID);
> +    if (prefixedOrUnprefixedID == propertyID)
> +        return ret;

We should have a better name for the "ret" variable.

> Source/WebCore/css/StylePropertySet.cpp:590
> +    StylePropertyShorthand prefixedOrUnprefixedShorthand = shorthandForProperty(prefixedOrUnprefixedID);

And here is another case where it would be nice to avoid the ugly "prefixedOrUnprefixed" (it also avoids the issue of where to capitalize :)

shorthandPrefixingVariant

> Source/WebCore/css/StylePropertySet.cpp:709
> +void StylePropertySet::appendPrefixedAndUnprefixed(const CSSProperty& property)

appendPrefixingVariant
Comment 5 Dean Jackson 2013-03-01 10:01:44 PST
Comment on attachment 190788 [details]
Patch

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

>> Source/WebCore/ChangeLog:22
>> +        two times animation objects for the resolved style.
> 
> "This is to avoid creating the animation objects two times for the resolved style"

errr... "creating the animation objects twice for" is better

>> Source/WebCore/ChangeLog:58
>> +        trying to set then we update the other one.
> 
> "If a unprefixed or prefixed counterpart of the the property we're trying to set exists then we update the other one"

"set exists, then" (add comma)

> Source/WebCore/ChangeLog:75
> +        animations in double, therefore when we try to resolve the unprefixed

"and create duplicate animations. Therefore.."
Comment 6 Alexis Menard (darktears) 2013-03-01 11:04:59 PST
Created attachment 190994 [details]
Patch
Comment 7 Antti Koivisto 2013-03-04 06:44:29 PST
Comment on attachment 190994 [details]
Patch

r=me
Comment 8 WebKit Review Bot 2013-03-04 07:11:05 PST
Comment on attachment 190994 [details]
Patch

Clearing flags on attachment: 190994

Committed r144626: <http://trac.webkit.org/changeset/144626>
Comment 9 WebKit Review Bot 2013-03-04 07:11:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Syoichi Tsuyuhara 2013-03-05 18:54:01 PST
I confirmed that this bug is fixed on Chromium 27.0.1431.0 (186272).
Thanks!
Comment 11 Simon Fraser (smfr) 2016-03-07 21:22:37 PST
I want to revert this patch. It causes element.style.cssText to report both prefixed and unprefixed versions of animation and transition properties, which is wrong.

Also, why was this all specific to just animations and transitions, and not other prefixed properties?
Comment 12 Antoine Quint 2016-05-11 10:08:52 PDT
This is mostly reverted by the fix for https://bugs.webkit.org/show_bug.cgi?id=157569.