Complete animations whether or not cascade needs done #12623

notriddle · 2016-07-27T17:46:53Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes
These changes fix #12554 (github issue number if applicable).

This change is

highfive · 2016-07-27T17:46:58Z

Heads up! This PR modifies the following files:

@bholley: components/style/traversal.rs

highfive · 2016-07-27T17:46:59Z

Warning

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

emilio · 2016-07-27T17:50:17Z

components/style/traversal.rs

+            if let Some(ref animations) = animations_to_expire {
+                for animation in *animations {
+                    // NB: Expiring a keyframes animation is the same as not
+                    // applying the keyframes style to it, so we're safe.


This is not quite true, could you make a note about supporting animation fill mode?

emilio · 2016-07-28T01:15:06Z

Reviewed 2 of 2 files at r2, 2 of 2 files at r3, 1 of 2 files at r5.
Review status: 1 of 4 files reviewed at latest revision, 2 unresolved discussions.

components/style/matching.rs, line 477 [r3] (raw file):

                for animation in *animations {
                    // TODO: support animation-fill-mode
                    if let Animation::Transition(_, _, ref frame, _) = *animation {

Can we factor this code out and use it in both situations?

components/style/traversal.rs, line 239 [r1] (raw file):

    } else {
        // Finish any expired transitions, without performing the cascade.
        let node_opaque = node.opaque();

I'm kind of worried this is a bit overkill in the common case there aren't transitions to expire. Also, I wonder if this will even work now #12563 has landed, where we won't process the nodes that aren't marked as needing a restyle. We also don't construct flows again for every node, so this might be fixed with that patch?

In case it's not: What about just layout marking nodes as dirty once it detects there's an animation to expire, and running the cascade then? Hmm... But layout only knows about OpaqueNodes, and by design these are not convertible back to a node... Maybe they should be from script?

Other way to do it is adding a flag to the node (IS_ANIMATED?) in start_transitions_if_applicable, but then we should have another flag, let's say HAS_ANIMATED_CHILDREN, and keep doing the traversal for these too.

Another solution that may actually work even better is updating the expired animation style from layout (in recalc_style_for_animations)?

We could remove the items in the expired animations map there if it's not needed from the cascade (haven't actually tested it), but even if it's needed, updating the style there should prevent it getting updated again until a proper restyle with cascade is done.

components/style/traversal.rs, line 252 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

This is not quite true, could you make a note about supporting animation fill mode?

Thanks!

Comments from Reviewable

emilio · 2016-07-28T01:17:59Z

Review status: 1 of 4 files reviewed at latest revision, 2 unresolved discussions.

components/style/traversal.rs, line 239 [r1] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

I'm kind of worried this is a bit overkill in the common case there aren't transitions to expire. Also, I wonder if this will even work now #12563 has landed, where we won't process the nodes that aren't marked as needing a restyle. We also don't construct flows again for every node, so this might be fixed with that patch?

In case it's not: What about just layout marking nodes as dirty once it detects there's an animation to expire, and running the cascade then? Hmm... But layout only knows about OpaqueNodes, and by design these are not convertible back to a node... Maybe they should be from script?

Other way to do it is adding a flag to the node (IS_ANIMATED?) in start_transitions_if_applicable, but then we should have another flag, let's say HAS_ANIMATED_CHILDREN, and keep doing the traversal for these too.

Another solution that may actually work even better is updating the expired animation style from layout (in recalc_style_for_animations)?

We could remove the items in the expired animations map there if it's not needed from the cascade (haven't actually tested it), but even if it's needed, updating the style there should prevent it getting updated again until a proper restyle with cascade is done.

Actually, thinking about it a bit more, this applies in the case an element isn't restyled, but its children are right? (i.e. the `HAS_DIRTY_DESCENDANTS` flag). In that case, I guess the overhead is not so much.

Comments from Reviewable

notriddle · 2016-07-28T01:42:18Z

components/style/matching.rs, line 477 [r3] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Can we factor this code out and use it in both situations?

I've been thinking about it, and I'm pretty sure I can do that, yeah.

Comments from Reviewable

emilio · 2016-07-28T02:23:28Z

components/style/traversal.rs

+        node.opaque(),
+        node.mutate_data().unwrap().style.as_mut().unwrap(),
+        context.shared_context()
+    );


Aren't we doing this twice now in the cascade case?

notriddle · 2016-07-28T02:34:49Z

components/style/traversal.rs, line 244 [r7] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Aren't we doing this twice now in the cascade case?

Oh, right, oops.

Comments from Reviewable

emilio · 2016-07-28T16:48:31Z

@bors-servo: try

Let's check if there's any other transition/animation test that could check this in wpt.

bors-servo · 2016-07-28T16:48:34Z

⌛ Trying commit d908d52 with merge 8e3eafb...


        Auto merge of #12623 - notriddle:master, r=<try>

Complete animations whether or not cascade needs done - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12554 (github issue number if applicable). - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12623)

bors-servo · 2016-07-28T17:45:17Z

💔 Test failed - linux-rel

notriddle · 2016-07-28T18:14:38Z

  ▶ Unexpected subtest result in /_mozilla/mozilla/mozbrowser/iframe_visibility.html:
  │ FAIL [expected PASS] Requesting animation frame composites only when frame is visible
  │   → assert_true: expected true got false
  │ 
  │ iframe.onload</<@http://web-platform.test:8000/_mozilla/mozilla/mozbrowser/iframe_visibility.html:58:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1426:20

Other than that, no unexpected test results, including passes.

emilio · 2016-07-28T18:15:38Z

Cool, r=me then.

@bors-servo: r+

On 07/28/2016 11:14 AM, Michael Howell wrote:

|▶ Unexpected subtest result in
/_mozilla/mozilla/mozbrowser/iframe_visibility.html: │ FAIL [expected
PASS] Requesting animation frame composites only when frame is visible │
→ assert_true: expected true got false │ │
iframe.onload</<@http://web-platform.test:8000/_mozilla/mozilla/mozbrowser/iframe_visibility.html:58:13
│
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
└
Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1426:20
|

Other than that, no unexpected test results, including passes.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12623 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwuhcySOSOPd1ueQRC0SJ9sW6JTyzqks5qaPGbgaJpZM4JWcsF.

bors-servo · 2016-07-28T18:15:39Z

📌 Commit d908d52 has been approved by emilio

emilio · 2016-07-28T18:18:32Z

@bors-servo: r-

Could you squash before, please?

@bors-servo: delegate+

bors-servo · 2016-07-28T18:18:33Z

✌️ @notriddle can now approve this pull request

notriddle · 2016-07-28T18:19:28Z

Is iframe_visibliity.html a known intermittent? Because there's no issue for it.

jdm · 2016-07-28T18:20:36Z

@notriddle: #11825


        Complete animations whether or not cascade is done

Fixes #12554


        Add tests for #12554

notriddle · 2016-07-28T18:22:51Z

@bors-servo r=emilio

bors-servo · 2016-07-28T18:22:52Z

📌 Commit f70b8c7 has been approved by emilio

bors-servo · 2016-07-28T19:54:37Z

⌛ Testing commit f70b8c7 with merge 27d8fb3...


        Auto merge of #12623 - notriddle:master, r=emilio

Complete animations whether or not cascade needs done - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12554 (github issue number if applicable). - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12623)

bors-servo · 2016-07-28T20:52:44Z

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

highfive assigned Ms2ger Jul 27, 2016

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

jdm added the S-needs-tests label Jul 27, 2016

emilio reviewed Jul 27, 2016
View changes

notriddle removed the S-needs-tests label Jul 27, 2016

emilio unassigned Ms2ger Jul 28, 2016

emilio reviewed Jul 28, 2016
View changes

components/style/traversal.rs Outdated

node.opaque(),

node.mutate_data().unwrap().style.as_mut().unwrap(),

context.shared_context()

);

This comment has been minimized.

Sign in to view

Copy link

emilio Jul 28, 2016
Member

Aren't we doing this twice now in the cascade case?

jdm added the S-fails-travis label Jul 28, 2016

notriddle removed the S-fails-travis label Jul 28, 2016

highfive added the S-tests-failed label Jul 28, 2016

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

highfive assigned emilio Jul 28, 2016

notriddle added 2 commits Jul 27, 2016

Complete animations whether or not cascade is done

6a37877

Fixes #12554

Add tests for #12554

Loading status checks…

f70b8c7

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

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

bors-servo merged commit f70b8c7 into servo:master Jul 28, 2016
3 checks passed

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

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

bholley mentioned this pull request Oct 21, 2016

Fix animation processing ordering and reenable animations/basic-transition.html test #13865

Closed

servo / servo

Complete animations whether or not cascade needs done #12623

Complete animations whether or not cascade needs done #12623

notriddle commented Jul 27, 2016 •

edited by emilio

highfive commented Jul 27, 2016

highfive commented Jul 27, 2016

This comment has been minimized.

emilio commented Jul 28, 2016

emilio commented Jul 28, 2016

notriddle commented Jul 28, 2016

This comment has been minimized.

notriddle commented Jul 28, 2016

emilio commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

notriddle commented Jul 28, 2016

emilio commented Jul 28, 2016

bors-servo commented Jul 28, 2016

emilio commented Jul 28, 2016

bors-servo commented Jul 28, 2016

notriddle commented Jul 28, 2016

jdm commented Jul 28, 2016

notriddle commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

servo / servo

Join GitHub today

Complete animations whether or not cascade needs done #12623

Complete animations whether or not cascade needs done #12623

Conversation

notriddle commented Jul 27, 2016 • edited by emilio

highfive commented Jul 27, 2016

highfive commented Jul 27, 2016

This comment has been minimized.

emilio Jul 27, 2016

emilio commented Jul 28, 2016

emilio commented Jul 28, 2016

notriddle commented Jul 28, 2016

This comment has been minimized.

emilio Jul 28, 2016

notriddle commented Jul 28, 2016

emilio commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

notriddle commented Jul 28, 2016

emilio commented Jul 28, 2016

bors-servo commented Jul 28, 2016

emilio commented Jul 28, 2016

bors-servo commented Jul 28, 2016

notriddle commented Jul 28, 2016

jdm commented Jul 28, 2016

notriddle commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

bors-servo commented Jul 28, 2016

notriddle commented Jul 27, 2016 •

edited by emilio