style: Add support to the animation shorthand and fix parsing of animation-name #12118

emilio · 2016-07-01T19:16:22Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors

Part of #11916.

r? @SimonSapin

This change is

highfive · 2016-07-01T19:16:25Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/box.mako.rs, components/style/properties/shorthand/box.mako.rs, components/style/properties/helpers.mako.rs
@KiChjang: components/script/dom/webidls/CSSStyleDeclaration.webidl

highfive · 2016-07-01T19:16:26Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

bors-servo · 2016-07-03T04:02:52Z

☔ The latest upstream changes (presumably #11972) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2016-07-07T17:10:42Z

r=me with one change

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/style/properties/shorthand/box.mako.rs, line 129 [r1] (raw file):

        loop {
            try_parse_one!(input, name, animation_name);

Per https://drafts.csswg.org/css-animations/#typedef-single-animation , name should be last (so that keywords valid for other longhands are not interpreted as names).

Please also add a comment saying so, and that duration needs to be before delay (which it already id) with that URL included in the comment.

Comments from Reviewable

emilio · 2016-07-07T17:52:11Z

Thanks for the careful review as always Simon :)

@bors-servo: r=SimonSapin

Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/style/properties/shorthand/box.mako.rs, line 129 [r1] (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Per https://drafts.csswg.org/css-animations/#typedef-single-animation , name should be last (so that keywords valid for other longhands are not interpreted as names).

Please also add a comment saying so, and that duration needs to be before delay (which it already id) with that URL included in the comment.

Whoops, good catch :)

Done

Comments from Reviewable

bors-servo · 2016-07-07T17:52:12Z

📌 Commit 1313181 has been approved by SimonSapin

bors-servo · 2016-07-07T17:55:41Z

⌛ Testing commit 1313181 with merge fe300ae...


        Auto merge of #12118 - emilio:animation-shorthand, r=SimonSapin

@SimonSapin

style: Add support to the animation shorthand and fix parsing of animation-name  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  Part of #11916. r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12118)

bors-servo · 2016-07-07T17:59:39Z

💔 Test failed - linux-rel

jdm · 2016-07-07T19:16:51Z

@bors-servo: retry

infra

bors-servo · 2016-07-07T20:43:44Z

⌛ Testing commit 1313181 with merge a8ee30f...


        Auto merge of #12118 - emilio:animation-shorthand, r=SimonSapin

@SimonSapin

style: Add support to the animation shorthand and fix parsing of animation-name  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  Part of #11916. r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12118)

bors-servo · 2016-07-07T21:26:42Z

💔 Test failed - mac-rel-css

jdm · 2016-07-07T21:50:07Z

  ▶ TIMEOUT [expected PASS] /css-flexbox-1_dev/html/css-flexbox-height-animation-stretch.htm

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/transition-timing-function-001.htm:
  └ PASS [expected FAIL] parse 'steps(3)'


        style: Add support to the animation shorthand and fix parsing of anim…

…ation-name.


        style: bonus: Fix parsing of steps() function.

Still doesn't work properly (this also happened with transitions). This is spec'd in: https://www.w3.org/TR/css3-transitions/#transition-timing-function


        style: Fix timing-function overriding from the keyframe declaration l…

…ist. The previous behavior is plain wrong, since that array has always at least one element, so we effectively couldn't specify anything else than "ease" in our animations.


        style: stylistic nit. be consistent on how we write inline annotations.

emilio · 2016-07-07T23:11:03Z

Updated the test expectations an opened #12328 regarding the flexbox test, which we rendered incorrectly anyway.

I probably need to dedicate some time to #12120, adding a test refresh mode to the constellation or similar I guess, and enabling some private APIs to allow testing animations à lo mochitest in gecko.

I want approval from @jdm or @SimonSapin for landing that last expectation change, I think it's reasonable but I prefer to double-check.

SimonSapin · 2016-07-08T11:58:04Z

Sounds ok to me.

@bors-servo r+

bors-servo · 2016-07-08T11:58:04Z

📌 Commit 3a4539f has been approved by SimonSapin

bors-servo · 2016-07-08T13:01:53Z

⌛ Testing commit 3a4539f with merge 1fabfee...


        Auto merge of #12118 - emilio:animation-shorthand, r=SimonSapin

@SimonSapin

style: Add support to the animation shorthand and fix parsing of animation-name  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  Part of #11916. r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12118)

bors-servo · 2016-07-08T13:57:04Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned SimonSapin Jul 1, 2016

highfive added the S-awaiting-review label Jul 1, 2016

highfive added the S-needs-rebase label Jul 3, 2016

emilio force-pushed the emilio:animation-shorthand branch from d993344 to 3586da2 Jul 3, 2016

emilio removed the S-needs-rebase label Jul 3, 2016

emilio force-pushed the emilio:animation-shorthand branch from 3586da2 to 1313181 Jul 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 7, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 7, 2016

emilio added 2 commits Jul 1, 2016

style: Add support to the animation shorthand and fix parsing of anim…

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

411ae84

…ation-name.

emilio added 2 commits Jul 1, 2016

style: stylistic nit. be consistent on how we write inline annotations.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

3a4539f

emilio force-pushed the emilio:animation-shorthand branch from 1313181 to 3a4539f Jul 7, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Jul 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 8, 2016

bors-servo merged commit 3a4539f into servo:master Jul 8, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 8, 2016

servo / servo

style: Add support to the animation shorthand and fix parsing of animation-name #12118

style: Add support to the animation shorthand and fix parsing of animation-name #12118

emilio commented Jul 1, 2016 •

edited by larsbergstrom

highfive commented Jul 1, 2016

highfive commented Jul 1, 2016

bors-servo commented Jul 3, 2016

SimonSapin commented Jul 7, 2016

emilio commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

jdm commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

jdm commented Jul 7, 2016

emilio commented Jul 7, 2016

SimonSapin commented Jul 8, 2016

bors-servo commented Jul 8, 2016

bors-servo commented Jul 8, 2016

bors-servo commented Jul 8, 2016

servo / servo

Join GitHub today

style: Add support to the animation shorthand and fix parsing of animation-name #12118

style: Add support to the animation shorthand and fix parsing of animation-name #12118

Conversation

emilio commented Jul 1, 2016 • edited by larsbergstrom

highfive commented Jul 1, 2016

highfive commented Jul 1, 2016

bors-servo commented Jul 3, 2016

SimonSapin commented Jul 7, 2016

emilio commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

jdm commented Jul 7, 2016

bors-servo commented Jul 7, 2016

bors-servo commented Jul 7, 2016

jdm commented Jul 7, 2016

emilio commented Jul 7, 2016

SimonSapin commented Jul 8, 2016

bors-servo commented Jul 8, 2016

bors-servo commented Jul 8, 2016

bors-servo commented Jul 8, 2016

emilio commented Jul 1, 2016 •

edited by larsbergstrom