Bug 157569 – Correctly handle prefixed and unprefixed variants in CSSStyleDeclaration

• Component: CSS
• Status: RESOLVED FIXED

[Antoine Quint]

For a few years now, we would add the variant of a property specified with a prefix, or for which a prefixed variant exists, when specifying it. This means that simply specifying "transition-property: width" would also set the "-webkit-transition-property" property to "width". This was added with https://bugs.webkit.org/show_bug.cgi?id=110011. This would yield some undesirable behaviour, for instance the `cssText` would contain more properties than those that were specified, and it would also lead to incorrect handling of shorthand properties that would not correctly reset previously set longhand properties (see https://bugs.webkit.org/show_bug.cgi?id=156944).

We need to fix this and also ensure interoperability with other browsers when reading style rules. For instance, specifying "transition-property: width" will make both `style.transitionProperty` and `style.Mow|webkitTransitionProperty` return "width" in Firefox and Chrome, we should follow the same behaviour.

[Radar WebKit Bug Importer]

<rdar://problem/26223115>

[Antoine Quint]

Created attachment 278631 [details]
First cut for patch with source changes and update to existing tests, will add more tests

[Antoine Quint]

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

[Antoine Quint]

Created attachment 278654 [details]
Patch

[Dean Jackson]

So other browsers add the prefixed variant when reading the style rules, but not via cssText?

[Antoine Quint]

That's correct, see the quick test case attached.

[Antoine Quint]

Created attachment 278702 [details]
test case showing behaviour of cssText

[Dean Jackson]

Comment on attachment 278654 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        * css/CSSParser.h:
> +
> +        Remove all code adding duplicated properties for the prefixed or unprefixed

No need for the blank line here.

> Source/WebCore/ChangeLog:28
> +        * css/CSSPropertyNames.in:
> +
> +        Treat transition properties as we do animation properties. 

Or here. etc.

> Source/WebCore/css/StyleProperties.cpp:882
> +            case CSSPropertyAnimationPlayState:

Why was this missing in the first place? I see that play-state is part of the shorthand (although I wish it wasn't).

If this is a change in behaviour I'm not sure we should do it, unless other browsers are compatible.

> LayoutTests/animations/play-state-start-paused.html:24
> -            animation: move 1s linear;
> +            animation: move 1s linear -0.5s paused;

OK. So this could be a change.

I'm reluctant to do this, but maybe because I just don't think play-state should be in the shorthand (and I'm not convinced it should even exist in the first place)

> LayoutTests/fast/css/prefixed-unprefixed-variant-style-declaration.html:18
> +    testProperty(property, property, prefixedProperty, accessor, prefixedAccessor, value);
> +    testProperty(prefixedProperty, property, prefixedProperty, accessor, prefixedAccessor, value);
> +}
> +
> +function testProperty(propertyToSet, property, prefixedProperty, accessor, prefixedAccessor, value) {

Can you name these parameters a bit more clearly?

It took a while to understand what the difference between propertyToSet and property was. Maybe property should be unprefixedProperty? You could also generate the prefixed form from that.

> LayoutTests/fast/css/prefixed-unprefixed-variant-style-declaration.html:48
> +    function test(message, actual, expected) {
> +        expected = expected || value;
> +        if (actual === expected)
> +            testPassed(message);
> +        else
> +            testFailed(`expected ${message} to be "${expected}" but got "${actual}"`);
> +    }

Can you put the test() function earlier inside testProperty()? (it still weirds me out that JS allows you to call a function before you've defined it)

> LayoutTests/fast/css/prefixed-unprefixed-variant-style-declaration.html:55
> +function accessorForProperty(property) {

Same here. And I guess you could technically put this inside testPropertyVariants the way you did with test().

[Antoine Quint]

(In reply to comment #8)
> Comment on attachment 278654 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278654&action=review
> 
> > Source/WebCore/css/StyleProperties.cpp:882
> > +            case CSSPropertyAnimationPlayState:
> 
> Why was this missing in the first place? I see that play-state is part of
> the shorthand (although I wish it wasn't).
> 
> If this is a change in behaviour I'm not sure we should do it, unless other
> browsers are compatible.

I think Simon simply missed it when fixing https://bugs.webkit.org/show_bug.cgi?id=156944.

> > LayoutTests/animations/play-state-start-paused.html:24
> > -            animation: move 1s linear;
> > +            animation: move 1s linear -0.5s paused;
> 
> OK. So this could be a change.
> 
> I'm reluctant to do this, but maybe because I just don't think play-state
> should be in the shorthand (and I'm not convinced it should even exist in
> the first place)

This really is something to bring up on https://bugs.webkit.org/show_bug.cgi?id=156944, this patch simply ensures that the correct behaviour for shorthands and longhands is respected for animation properties. Which longhands are in the shorthand is a different topic.

> > LayoutTests/fast/css/prefixed-unprefixed-variant-style-declaration.html:18
> > +    testProperty(property, property, prefixedProperty, accessor, prefixedAccessor, value);
> > +    testProperty(prefixedProperty, property, prefixedProperty, accessor, prefixedAccessor, value);
> > +}
> > +
> > +function testProperty(propertyToSet, property, prefixedProperty, accessor, prefixedAccessor, value) {
> 
> Can you name these parameters a bit more clearly?
> 
> It took a while to understand what the difference between propertyToSet and
> property was. Maybe property should be unprefixedProperty? You could also
> generate the prefixed form from that.

True, I'll improve this based on that feedback.

> > LayoutTests/fast/css/prefixed-unprefixed-variant-style-declaration.html:48
> > +    function test(message, actual, expected) {
> > +        expected = expected || value;
> > +        if (actual === expected)
> > +            testPassed(message);
> > +        else
> > +            testFailed(`expected ${message} to be "${expected}" but got "${actual}"`);
> > +    }
> 
> Can you put the test() function earlier inside testProperty()? (it still
> weirds me out that JS allows you to call a function before you've defined it)

Sure.

> > LayoutTests/fast/css/prefixed-unprefixed-variant-style-declaration.html:55
> > +function accessorForProperty(property) {
> 
> Same here. And I guess you could technically put this inside
> testPropertyVariants the way you did with test().

Sure.

[Antoine Quint]

Created attachment 278729 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 278729 [details]
Patch for landing

Clearing flags on attachment: 278729

Committed r200769: <http://trac.webkit.org/changeset/200769>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

(In reply to comment #9)

> Which longhands are in the shorthand is a different
> topic.

Yes. But do other browsers support it? If not, I'll push to have it removed from the shorthand.

[Antoine Quint]

(In reply to comment #13)
> (In reply to comment #9)
> 
> > Which longhands are in the shorthand is a different
> > topic.
> 
> Yes. But do other browsers support it? If not, I'll push to have it removed
> from the shorthand.

Both Firefox and Chrome support it.

[Dean Jackson]

This was rolled out in http://trac.webkit.org/r203380

[Ahmad Saleem]

This commit reapplied these fixes:

https://github.com/WebKit/WebKit/commit/fc6451342b2d9f1896b14e04ed7b8012f8690cd8

I am going to mark this as "RESOLVED FIXED", please reopen if I am wrong. Thanks!