New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Some animation cleanups and fixes. #20757

Merged
merged 5 commits into from Oct 16, 2018

Conversation

Projects
None yet
9 participants
@emilio
Member

emilio commented May 6, 2018

The transitions code is still terribly broken, but I ran out of time fixing it. We have nothing that stops transitions, which is just plain wrong. Most of this code should probably be rewritten, since with the current setup is pretty hard to get it right. Anyway...

Fixes #20731.
Fixes #20116.


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented May 6, 2018

Heads up! This PR modifies the following files:

  • @bholley: components/style/matching.rs, components/style/animation.rs, components/style/properties/helpers/animated_properties.mako.rs
  • @canaltinova: components/style/matching.rs, components/style/animation.rs, components/style/properties/helpers/animated_properties.mako.rs
@highfive

This comment has been minimized.

highfive commented May 6, 2018

warning Warning warning

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

@emilio emilio force-pushed the emilio:animations branch from fba4937 to 31ffd84 May 6, 2018

@emilio

This comment has been minimized.

Member

emilio commented May 6, 2018

@emilio

This comment has been minimized.

Member

emilio commented May 7, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented May 7, 2018

⌛️ Trying commit 31ffd84 with merge 458a54c...

bors-servo added a commit that referenced this pull request May 7, 2018

Auto merge of #20757 - emilio:animations, r=<try>
style: Some animation cleanups and fixes.

The transitions code is still terribly broken, but I ran out of time fixing it. We have nothing that stops transitions, which is just plain wrong. Most of this code should probably be rewritten, since with the current setup is pretty hard to get it right. Anyway...

Fixes #20731.
Fixes #20116.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20757)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented May 7, 2018

💔 Test failed - linux-rel-css

@CYBAI

This comment has been minimized.

Collaborator

CYBAI commented May 7, 2018

In linux-rel-css:

{"status": "FAIL", "group": "default", "message": "assert_in_array: color is green after transition runs value \"rgb(0, 0, 255)\" not in array [\"rgb(0, 128, 0)\", \"rgba(0, 128, 0, 1)\"]", "stack": "@http://web-platform.test:8000/css/css-variables/variable-transitions-transition-property-all-before-value.html:59:9\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1490:20\nTest.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1530:17\n", "subtest": "Verify substituted color value after transition", "test": "/css/css-variables/variable-transitions-transition-property-all-before-value.html", "line": 40081, "action": "test_result", "expected": "PASS"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "font-style animation", "test": "/css/css-fonts/variations/font-style-interpolation.html", "line": 60956, "action": "test_result", "expected": "TIMEOUT"}
{"status": "TIMEOUT", "group": "default", "message": "Test timed out", "stack": null, "subtest": "font-style transition", "test": "/css/css-fonts/variations/font-style-interpolation.html", "line": 60957, "action": "test_result", "expected": "PASS"}
{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "font-weight animation", "test": "/css/css-fonts/variations/font-weight-interpolation.html", "line": 104452, "action": "test_result", "expected": "TIMEOUT"}
{"status": "FAIL", "group": "default", "message": "assert_in_array: color is green after transition runs value \"rgb(0, 0, 255)\" not in array [\"rgb(0, 128, 0)\", \"rgba(0, 128, 0, 1)\"]", "stack": "@http://web-platform.test:8000/css/css-variables/variable-transitions-value-before-transition-property-all.html:58:9\nTest.prototype.step@http://web-platform.test:8000/resources/testharness.js:1490:20\nTest.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1530:17\n", "subtest": "Verify substituted color value after transition", "test": "/css/css-variables/variable-transitions-value-before-transition-property-all.html", "line": 115469, "action": "test_result", "expected": "PASS"}

In mac-rel-wpt4:

{"status": "PASS", "group": "default", "message": null, "stack": null, "subtest": "font-style animation", "test": "/css/css-fonts/variations/font-style-interpolation.html", "line": 22039, "action": "test_result", "expected": "TIMEOUT"}
{"status": "TIMEOUT", "group": "default", "message": "Test timed out", "stack": null, "subtest": "font-style transition", "test": "/css/css-fonts/variations/font-style-interpolation.html", "line": 22040, "action": "test_result", "expected": "PASS"}
{"status": "NOTRUN", "group": "default", "message": null, "stack": null, "subtest": "Overall test", "test": "/_mozilla/webgl/conformance-1.0.3/conformance/ogles/GL/swizzlers/swizzlers_017_to_024.html", "line": 90995, "action": "test_result", "expected": "PASS"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/webgl/conformance-1.0.3/conformance/ogles/GL/swizzlers/swizzlers_017_to_024.html", "line": 91012, "action": "test_result", "expected": "OK"}
@@ -207,7 +207,7 @@ pub enum Animation {
/// the f64 field is the start time as returned by `time::precise_time_s()`.
///
/// The `bool` field is werther this animation should no longer run.

This comment has been minimized.

@gootorov

gootorov May 8, 2018

Contributor

Don't forget to remove this line too.

% endif
% if prop.animatable:
AnimatedProperty::${prop.camel_case}(..) => LonghandId::${prop.camel_case},
% endif

This comment has been minimized.

@nox

nox May 10, 2018

Member

I wonder if AnimatedProperty should use the same trick as AnimationValue to just read the discriminant value as the LonghandId result.

@fabricedesre

This comment has been minimized.

Contributor

fabricedesre commented May 30, 2018

@emilio do you need help pushing that to the finish line?

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 24, 2018

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

emilio added some commits May 5, 2018

@emilio emilio force-pushed the emilio:animations branch from 31ffd84 to 9f3f438 Oct 15, 2018

emilio added some commits May 5, 2018

style: Remove unused expired boolean in Animation::Transition.
The last caller who used was #14418, which did fix a problem but introduced
multiple. In particular, now transitions don't get expired ever, until they
finish running of course.

That is not ok, given you can have something that the user can trigger to change
the style (hi, :hover, for example), and right now that triggers new
transitions, getting this into a really funny state.

I should give fixing this a shot, but it's non-trivial at all.
style: Expire keyframes animations when no longer referenced by the s…
…tyle.

It's a long way to make this sound in general...

Fixes #20731

@emilio emilio force-pushed the emilio:animations branch from 9f3f438 to 7979b25 Oct 15, 2018

@emilio

This comment has been minimized.

Member

emilio commented Oct 15, 2018

bors-servo added a commit that referenced this pull request Oct 15, 2018

Auto merge of #20757 - emilio:animations, r=<try>
style: Some animation cleanups and fixes.

The transitions code is still terribly broken, but I ran out of time fixing it. We have nothing that stops transitions, which is just plain wrong. Most of this code should probably be rewritten, since with the current setup is pretty hard to get it right. Anyway...

Fixes #20731.
Fixes #20116.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20757)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

⌛️ Trying commit 7979b25 with merge c1a3d7e...

@jdm

This comment has been minimized.

Member

jdm commented Oct 15, 2018

The results for these tests appear stable on linux and mac:

  • /css/css-transitions/properties-value-003.html
  • /css/css-transitions/properties-value-001.html
  • /css/css-transitions/properties-value-inherit-002.html
  • /css/css-transitions/properties-value-implicit-001.html
@emilio

This comment has been minimized.

Member

emilio commented Oct 15, 2018

Hmm, that's somewhat surprising, because the only difference with the previous try run (which didn't have that test-case) is the test expectations...

@emilio

This comment has been minimized.

Member

emilio commented Oct 15, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

⌛️ Trying commit 561e9c8 with merge 8cf3be0...

bors-servo added a commit that referenced this pull request Oct 15, 2018

Auto merge of #20757 - emilio:animations, r=<try>
style: Some animation cleanups and fixes.

The transitions code is still terribly broken, but I ran out of time fixing it. We have nothing that stops transitions, which is just plain wrong. Most of this code should probably be rewritten, since with the current setup is pretty hard to get it right. Anyway...

Fixes #20731.
Fixes #20116.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20757)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Member

jdm commented Oct 15, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

📌 Commit 561e9c8 has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 15, 2018

⌛️ Testing commit 561e9c8 with merge 3b03f6d...

bors-servo added a commit that referenced this pull request Oct 15, 2018

Auto merge of #20757 - emilio:animations, r=jdm
style: Some animation cleanups and fixes.

The transitions code is still terribly broken, but I ran out of time fixing it. We have nothing that stops transitions, which is just plain wrong. Most of this code should probably be rewritten, since with the current setup is pretty hard to get it right. Anyway...

Fixes #20731.
Fixes #20116.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20757)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 16, 2018

@bors-servo bors-servo merged commit 561e9c8 into servo:master Oct 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@emilio emilio deleted the emilio:animations branch Oct 16, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2018

Bug 1500260 - Remove unused expired boolean in Animation::Transition.…
… r=emilio

The last caller who used was #14418, which did fix a problem but introduced
multiple. In particular, now transitions don't get expired ever, until they
finish running of course.

That is not ok, given you can have something that the user can trigger to change
the style (hi, :hover, for example), and right now that triggers new
transitions, getting this into a really funny state.

I should give fixing this a shot, but it's non-trivial at all.

This cherry-picks part of servo/servo#20757.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2018

Bug 1500260 - Expire keyframes animations when no longer referenced b…
…y the style. r=emilio

It's a long way to make this sound in general...

Fixes #20731

This cherry-picks part of servo/servo#20757.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 18, 2018

Bug 1500260 - More useful logging for transition-related stuff. r=emilio
Transitions are still broken, but I found these messages more helpful than the
previous ones when diagnosing problems.

This cherry-picks part of servo/servo#20757.

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Oct 19, 2018

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Oct 19, 2018

Bug 1500260 - Remove unused expired boolean in Animation::Transition.…
… r=emilio

The last caller who used was #14418, which did fix a problem but introduced
multiple. In particular, now transitions don't get expired ever, until they
finish running of course.

That is not ok, given you can have something that the user can trigger to change
the style (hi, :hover, for example), and right now that triggers new
transitions, getting this into a really funny state.

I should give fixing this a shot, but it's non-trivial at all.

This cherry-picks part of servo/servo#20757.

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Oct 19, 2018

Bug 1500260 - Expire keyframes animations when no longer referenced b…
…y the style. r=emilio

It's a long way to make this sound in general...

Fixes #20731

This cherry-picks part of servo/servo#20757.

xeonchen pushed a commit to xeonchen/gecko-cinnabar that referenced this pull request Oct 19, 2018

Bug 1500260 - More useful logging for transition-related stuff. r=emilio
Transitions are still broken, but I found these messages more helpful than the
previous ones when diagnosing problems.

This cherry-picks part of servo/servo#20757.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment