Bug 96658 - REGRESSION: transition doesn’t always override transition-property
Summary: REGRESSION: transition doesn’t always override transition-property
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Simon Fraser (smfr)
URL: http://result.dabblet.com/gist/3715069
Keywords: InRadar
Depends on:
Reported: 2012-09-13 08:56 PDT by Lea Verou
Modified: 2012-09-14 15:26 PDT (History)
7 users (show)

See Also:

Patch (50.42 KB, patch)
2012-09-14 14:41 PDT, Simon Fraser (smfr)
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lea Verou 2012-09-13 08:56:54 PDT
As you can see in the testcase, there is no smooth transition because the transition shorthand does not seem to override the first transition-property, even though it should, and it does in every other engine and older versions of WebKit. Chrome 19 and Safari 6.0 do not have this issue. It can be reproduced in Chrome > 19 and WebKit nightlies. I can also reproduce it in my Safari 6.0, but not BrowserStack’s Safari 6.0, so I'm not sure whether it occurs there.

Steps to reproduce:
- Go to testcase
- Hover over the gold paragraph
- Observe how there is no smooth transition

Things that fix the issue:
- Explicitly adding the transition-property value in the shorthand. i.e. converting transition: 2s; to transition: 2s all; or transition 2s background;
- Merging the two rules

Things that don't fix the issue:
- Increasing the specificity of the second rule
- Adding a separate transition-property declaration in the second rule WITHOUT removing the first one

Other observations:
- Other transition-* properties like transition-duration or transition-delay don't seem to exhibit this issue.
Comment 1 Simon Fraser (smfr) 2012-09-13 11:34:07 PDT
That does seem like a bug.
Comment 2 Simon Fraser (smfr) 2012-09-13 17:25:43 PDT
Interesting. In debug, we hit:
ASSERTION FAILED: prop >= firstCSSProperty && prop < (firstCSSProperty + numCSSProperties)
Comment 3 Simon Fraser (smfr) 2012-09-13 18:42:57 PDT
When parsing the shorthand, CSSParser::parseTransitionShorthand() fills in unsupplied values with the "initial" value:

    // Fill in any remaining properties with the initial value.
    for (i = 0; i < numProperties; ++i) {
        if (!parsedProperty[i])
            addAnimationValue(values[i], cssValuePool().createImplicitInitialValue());

and later on we take this to imply that we should use Animation::initialAnimationProperty() which is CSSPropertyInvalid. Maybe that should be All?
Comment 4 Simon Fraser (smfr) 2012-09-14 10:43:21 PDT
Regressed with http://trac.webkit.org/changeset/113225 when Alexis changed initialAnimationProperty() to return CSSPropertyInvalid rather than cAnimateAll.

Also, the use of CSSPropertyID in platform/animation code is a layering violation.
Comment 5 Simon Fraser (smfr) 2012-09-14 14:11:17 PDT
Possibly seeing this (the assertion at least) on
Comment 6 Simon Fraser (smfr) 2012-09-14 14:41:09 PDT
Created attachment 164228 [details]
Comment 7 Simon Fraser (smfr) 2012-09-14 14:41:33 PDT
Comment 8 Simon Fraser (smfr) 2012-09-14 14:53:00 PDT
Comment 9 Simon Fraser (smfr) 2012-09-14 15:26:19 PDT
Build fix in http://trac.webkit.org/changeset/128660