Bug 157569 - Correctly handle prefixed and unprefixed variants in CSSStyleDeclaration
Summary: Correctly handle prefixed and unprefixed variants in CSSStyleDeclaration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
: 156944 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-05-11 09:53 PDT by Antoine Quint
Modified: 2022-08-20 16:27 PDT (History)
9 users (show)

See Also:


Attachments
First cut for patch with source changes and update to existing tests, will add more tests (26.38 KB, patch)
2016-05-11 10:07 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (55.37 KB, patch)
2016-05-11 12:45 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
test case showing behaviour of cssText (178 bytes, text/html)
2016-05-11 23:29 PDT, Antoine Quint
no flags Details
Patch for landing (55.52 KB, patch)
2016-05-12 05:17 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-05-11 09:53:58 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-05-11 09:54:27 PDT
<rdar://problem/26223115>
Comment 2 Antoine Quint 2016-05-11 10:07:29 PDT
Created attachment 278631 [details]
First cut for patch with source changes and update to existing tests, will add more tests
Comment 3 Antoine Quint 2016-05-11 10:09:02 PDT
*** Bug 156944 has been marked as a duplicate of this bug. ***
Comment 4 Antoine Quint 2016-05-11 12:45:15 PDT
Created attachment 278654 [details]
Patch
Comment 5 Dean Jackson 2016-05-11 17:25:34 PDT
So other browsers add the prefixed variant when reading the style rules, but not via cssText?
Comment 6 Antoine Quint 2016-05-11 23:28:35 PDT
That's correct, see the quick test case attached.
Comment 7 Antoine Quint 2016-05-11 23:29:05 PDT
Created attachment 278702 [details]
test case showing behaviour of cssText
Comment 8 Dean Jackson 2016-05-12 04:59:54 PDT
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().
Comment 9 Antoine Quint 2016-05-12 05:07:06 PDT
(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.
Comment 10 Antoine Quint 2016-05-12 05:17:09 PDT
Created attachment 278729 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2016-05-12 05:46:31 PDT
Comment on attachment 278729 [details]
Patch for landing

Clearing flags on attachment: 278729

Committed r200769: <http://trac.webkit.org/changeset/200769>
Comment 12 WebKit Commit Bot 2016-05-12 05:46:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Dean Jackson 2016-05-12 11:07:28 PDT
(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.
Comment 14 Antoine Quint 2016-05-12 11:31:17 PDT
(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.
Comment 15 Dean Jackson 2016-07-18 16:43:59 PDT
This was rolled out in http://trac.webkit.org/r203380
Comment 16 Ahmad Saleem 2022-08-20 16:27:21 PDT
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!