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.
<rdar://problem/26223115>
Created attachment 278631 [details] First cut for patch with source changes and update to existing tests, will add more tests
*** Bug 156944 has been marked as a duplicate of this bug. ***
Created attachment 278654 [details] Patch
So other browsers add the prefixed variant when reading the style rules, but not via cssText?
That's correct, see the quick test case attached.
Created attachment 278702 [details] test case showing behaviour of cssText
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().
(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.
Created attachment 278729 [details] Patch for landing
Comment on attachment 278729 [details] Patch for landing Clearing flags on attachment: 278729 Committed r200769: <http://trac.webkit.org/changeset/200769>
All reviewed patches have been landed. Closing bug.
(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.
(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.
This was rolled out in http://trac.webkit.org/r203380
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!