Bug 157569

<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!