We need eRestyle_CSSAnimations and eRestyle_CSSTransitions to know whether the style changes are caused by normal style change or animation style change.

I am totally unfamiliar with this area, so I don't know how much work is required for completion. A thing what I realize that we should do is: * propagate RESTYLE_CSS_ANIMATIONS even if the element has other restyle hints A good news with this patch is that I am feeling animation moving smoother. yay!

For reference, I just pushed a try with WIP patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3a66a8fe16aeec185a26afecc811448239cf1d Actually this does not work well yet, with these patches some tests in test_animations.html fails. There must be something that I am missing. But anyway, I don't want to block Boris's work for transitions by this bug. Gosh! I did forget to add try syntax, but it doesn't matter since it won't work. :-p

I only had a glancing look at the patches, but don't we need something like the logic in GeckoRestyleManager::ProcessPendingRestyles() in the ServoRestyleManager for this to work? That is we do the following: If we have any restyles that are *not* animation restyles: * Make a separate RestyleTracker * Get EffectCompositor to add to the RestyleTracker restyles for all elements that have animation restyles (including throttled restyles) * Set a flag on the transition manager to say "animation-only" restyle so it knows not to generate transitions * Process the restyles in the separate restyle tracker (i.e. the animation restyles) * Clear the flag on the transition manager * Process the remaining restyles If we *only* have animation restyles: * Set the flag on the transition manager to say "animation-only" restyle * Process the restyles * Clear the flag on the transition manager

Thanks. I should note that the failure I mentioned in comment 2 is not related to the animation-only restyles because we have no transitions on stylo yet. For the animation-only restyles I think we can re-use the second traversal for CSS animation for animation-only restyles, I am not 100% sure though.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > For the animation-only restyles I think we can re-use the second traversal > for CSS animation for animation-only restyles, I am not 100% sure though. The animation restyle needs to happen first so that during the main traversal (i.e. the current "first" traversal) we can produce the correct before and after change styles for generating transitions. So, for the case where, say, an animation-name property changes and we have animation restyles pending, we will have three restyles: * animation-only restyle * main restyle where we generate the animation changes * "second" restyle where we apply the animation changes

(In reply to Brian Birtles (:birtles) from comment #5) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > > For the animation-only restyles I think we can re-use the second traversal > > for CSS animation for animation-only restyles, I am not 100% sure though. > > The animation restyle needs to happen first so that during the main > traversal (i.e. the current "first" traversal) we can produce the correct > before and after change styles for generating transitions. Gah. CSS transitions need animation styles for the before and after changes? Can't we somehow the animation-only restyles in the first normal traversal? If we really need the animation-only restyles as the zero-th traversal, I am yet too unfamiliar with servo's traversal to introduce the zero-th traversal.

The reason we do it that way is that we need to distinguish between changes due to animation and other changes. So, for example, if we just apply the animation change and the other changes at once, the before and after change styles won't match but we won't know if that's due to animations ticking (which we should ignore) or due to real changes that should generate transitions. We used to do it differently by using a cover rule that--if my memory is correct--we applied to the element to hide animation styles. However, we changed that to use two phases instead. Bug 960465 (and bug 996796) has some description about why this change was made. We probably need to look into that bug a bit more.

OK. I found the reason of the failure. The reason is bug 1339704. In RESTYLE_CSS_ANIMATIONS restyle we update only CSS animation level rule. Bug unfortunately the wrong transition level rule still persists there. So the wrong style overrides the updated CSS animation level style. I am inclined to return None for transition level for now in get_animation_rules(). As for the animation-only restyle I am guessing it in the first traversal.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > OK. I found the reason of the failure. The reason is bug 1339704. In > RESTYLE_CSS_ANIMATIONS restyle we update only CSS animation level rule. Bug > unfortunately the wrong transition level rule still persists there. So the > wrong style overrides the updated CSS animation level style. I am inclined > to return None for transition level for now in get_animation_rules(). > > As for the animation-only restyle I am guessing it in the first traversal. I just updated Bug 1339704, and verified it by Bug 1339704 Comment 0. We don't trigger transition yet, but I think it will work while both transition and animations happen.

Summary of what I am going to do in this bug: * Add a new bit to element to represents that a descendant has animations. We can traverse down dom tree with this bit to find elements that have animations. * When eRestyle_CSSAnimations is posted to an element, set this bit to ancestors of the element and set RESTYLE_CSS_ANIMATIONS hint to the element. * If the root node has this bit, we treat the traversal as *animation-only* traversal. * In the animation-only traversal restyle elements that has RESTYLE_CSS_ANIMATIONS hint and remove the hint. (The new bit is also cleared while traversing down dom tree)

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review124662 ::: servo/components/style/traversal.rs:285 (Diff revision 1) > - .map_or(false, |d| d.has_styles()) > - { > + .map_or(false, |d| d.has_styles()) { > + // XXX: Set the dirty descendtants bit if this is not animation-only restyle? > unsafe { parent.set_dirty_descendants(); } > } One point I don't quite understand is here. I think we don't need to set the dirtry descendants bit in case of animation-only traversal. But without setting the dirty bit here, no visually change happens on animating element (whereas animation value is surely computed).

These patch set actually works fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c18b8f8edd4f7a6565faab73476e377e67aafb05 But I'd like to know the purpose of the set_dirty_descendants() in comment 20. Bobby, could you please tell me about the purpose of the set_dirty_descendants() in traverse_children()? Maybe I am missing an important thing about the dirty bit. Note that I did introduce another bit for animation-only restyle in the first patch.

A general comment here (not in response to the last 2 comments): I think the key thing here is that transitions depend on is that the style changes that result from the refresh driver's time advancing (which do not trigger transitions) need to be separated from other style changes (which do trigger transitions). Gecko handles this by calling UpdateOnlyAnimationStyles and recording the state with nsTransitionManager::SetInAnimationOnlyStyleUpdate. Gecko currently (in GeckoRestyleManager::PostRestyleEvent) uses the somewhat fragile mechanism that the set of restyle hints posted by animation ticking as the result of refresh driver time advance, and the set of restyle hints posted for other things, are disjoint. This is (as the comment there says) somewhat fragile. If you want to depend on that mechanism, you similarly need to ensure that the sets are disjoint.

Thank you David for your quick useful response. (In reply to David Baron :dbaron: ⌚️UTC-7 from comment #22) > I think the key thing here is that transitions depend on is that the style > changes that result from the refresh driver's time advancing (which do not > trigger transitions) need to be separated from other style changes (which do > trigger transitions). Gecko handles this by calling > UpdateOnlyAnimationStyles and recording the state with > nsTransitionManager::SetInAnimationOnlyStyleUpdate. I think I did these in the patch set. Animation-only traversal before normal traversal and added animation_only_restyle flag to SharedStyleContext. > Gecko currently (in GeckoRestyleManager::PostRestyleEvent) uses the somewhat > fragile mechanism that the set of restyle hints posted by animation ticking > as the result of refresh driver time advance, and the set of restyle hints > posted for other things, are disjoint. This is (as the comment there says) > somewhat fragile. I have not read the comment carefully. I will check it. Thank you!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21) > These patch set actually works fine: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c18b8f8edd4f7a6565faab73476e377e67aafb05 > > But I'd like to know the purpose of the set_dirty_descendants() in comment > 20. > Bobby, could you please tell me about the purpose of the > set_dirty_descendants() in traverse_children()? > Maybe I am missing an important thing about the dirty bit. That logic exists to be sure that we can actually find and update restyled elements during the post-traversal. Before the traversal, we propagate the bit up from any "restyle roots" (elements for which we've called Servo_NoteExplicitHints). However, it's possible for restyling at such a root to trigger more restyling in the siblings and subtree, and so we need to set the bit to make sure that we can use it in the post-traversal to find any elements that (a) have updated style or (b) have change hints to apply.

Thank you Bobby. Now I think I understand why the set_dirty_descendants() call is necessary for animation-only restyle, animation-only restyle needs to update style context.

The previous patch did traverse for non-animating element during animation-only traversal.

Comment on attachment 8849720 [details] Bug 1344966 - Add NODE_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO to represent that an element's descendant has animation. https://reviewboard.mozilla.org/r/122500/#review125772 r=me. If you think my suggested names aren't better, it's fine to leave yours. ::: dom/base/Element.h:98 (Diff revision 2) > + // Share for servo. ELEMENT_HAS_PENDING_ANIMATION_ONLY_RESTYLE is only > + // used in RestyleTracker which is not used for styo at all. > + ELEMENT_HAS_ANIMATING_DESCENDANTS_FOR_SERVO = > + ELEMENT_HAS_PENDING_ANIMATION_ONLY_RESTYLE, > + If you specifically want to re-use ELEMENT_HAS_PENDING_ANIMATION_ONLY_RESTYLE, then I think we should define it in nsINode.h as NODE_SHARED_RESTYLE_BIT_3, like we do with NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO. But I think it would be fine just to use NODE_SHARED_RESTYLE_BIT_2. Regarding the name: ELEMENT_HAS_ANIMATING_DESCENDANTS_FOR_SERVO sounds like a bit that would be set if an animation is running, rather than if we've posted an animation-only restyle. But NODE_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO seems a bit long. Not sure I have a better idea. ::: servo/components/style/dom.rs:312 (Diff revision 2) > + /// Similar to the dirty_descendants but for representing a descendant of > + /// the element needs to be updated in animation-only traversal. > + fn has_animating_descendants(&self) -> bool { > + false > + } Here again I think the names of these functions should emphasize that this is checking whether there are descendants that are dirty due to animation-only restyles. So, maybe: has_animation_only_dirty_descendants set_animation_only_dirty_descendants unset_animation_only_dirty_descendants ?

Comment on attachment 8849721 [details] Bug 1344966 - Set has-animating-descendants bit to ancestors of element which has animations for eRestyle_CSSAnimations. https://reviewboard.mozilla.org/r/122502/#review125782 r=me though I might prefer to use an enum instead of a bool for the new argument. ::: servo/ports/geckolib/glue.rs:1430 (Diff revision 2) > debug!("Servo_NoteExplicitHints: {:?}, restyle_hint={:?}, change_hint={:?}", > element, restyle_hint, change_hint); > > let mut maybe_data = element.mutate_data(); > - let maybe_restyle_data = > - maybe_data.as_mut().and_then(|d| unsafe { maybe_restyle(d, element) }); > + let maybe_restyle_data = maybe_data.as_mut().and_then(|d| unsafe { > + maybe_restyle(d, element, restyle_hint == structs::nsRestyleHint_eRestyle_CSSAnimations) Can we assert in here that eRestyle_CSSAnimations only ever appears by itself?

Comment on attachment 8849722 [details] Bug 1344966 - Add flag that represents the traversal is only for animation-only restyle. https://reviewboard.mozilla.org/r/122504/#review125796 ::: servo/components/style/context.rs:91 (Diff revision 2) > pub timer: Timer, > > /// The QuirksMode state which the document needs to be rendered with > pub quirks_mode: QuirksMode, > + > + /// True if the traversal is processed only animation restyles. s/processed/processing/ ::: servo/components/style/traversal.rs:53 (Diff revision 2) > /// Whether we should traverse only unstyled children. > pub fn traverse_unstyled_children_only(&self) -> bool { > self.unstyled_children_only > } > + > + /// Whether we should traverse only animations. Maybe s/animations/descendants with animation restyles/? ::: servo/components/style/traversal.rs:123 (Diff revision 2) > if unstyled_children_only { > return PreTraverseToken { > traverse: true, > unstyled_children_only: true, > + animation_only: root.has_animating_descendants(), > }; > } Will we ever have unstyled_children_only and animation_only both true? If not, then maybe we can assert or warn in that case, if it's unexpected. ::: servo/ports/geckolib/glue.rs:1516 (Diff revision 2) > if result.is_some() { > return result.unwrap().into_strong(); > } > > // We don't have the style ready. Go ahead and compute it as necessary. > - let shared = create_shared_context(&guard, &mut doc_data.borrow_mut()); > + let shared = create_shared_context(&guard, &mut doc_data.borrow_mut(), true); It's not obvious to me why we should pass in true here. Shouldn't we be doing a normal restyle here? Can you explain?

Comment on attachment 8849721 [details] Bug 1344966 - Set has-animating-descendants bit to ancestors of element which has animations for eRestyle_CSSAnimations. https://reviewboard.mozilla.org/r/122502/#review125798

Comment on attachment 8849723 [details] Bug 1344966 - Introduce a closure to replace rule nodes. https://reviewboard.mozilla.org/r/122506/#review125804 ::: servo/components/style/matching.rs:887 (Diff revision 2) > - let style_attribute = self.style_attribute(); > - > + let mut replace_rule_node = |level: CascadeLevel, > + pdb: Option<&Arc<Locked<PropertyDeclarationBlock>>>, > + path: &mut StrongRuleNode, > + guards: &StylesheetGuards| { Do we need to pass in guards? Can we just refer to |&context.shared.guards| from inside the closer instead?

Comment on attachment 8849723 [details] Bug 1344966 - Introduce a closure to replace rule nodes. https://reviewboard.mozilla.org/r/122506/#review125806

Comment on attachment 8849724 [details] Bug 1344966 - Introduce eRestyle_CSSAnimations. https://reviewboard.mozilla.org/r/122508/#review125808 ::: servo/components/style/data.rs:139 (Diff revision 2) > - StoredRestyleHint(if self.0.contains(RESTYLE_DESCENDANTS) { > + // In case of the element has animation restyle hint descendants > + // need just cascading so we don't propagate any restyle hints. Might be a bit clearer to say: // If we have RESTYLE_CSS_ANIMATIONS restyle hint, it means we are in the // middle of an animation only restyle. In that case, we don't need to // propagate any restyle hints. ::: servo/components/style/matching.rs:903 (Diff revision 2) > + if hint.contains(RESTYLE_CSS_ANIMATIONS) { > + let animation_rule = self.get_animation_rule(None); > + replace_rule_node(CascadeLevel::Animations, Can we assert in here that the animation_only_restyle flag on the context is true?

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review125810 I feel like we should be able to avoid passing a boolean value throughout a bunch of these functions. Can we just grab it off the SharedStyleContext? That should be accessible from the |traversal| object. ::: commit-message-d50a3:10 (Diff revision 3) > +1. Traverse only elements that have this restyle hint (animation-only traversal) > + RESTYLE_CSS_ANIMATIONS is stripped off from restyle hints of the elements > +2. Traverse all dirty elements (normal traversal) > +3. Create a SequentialTask if we have updated CSS Animations properties in > + the normal traversal > +4. Traverse elements that need to be updated animation style as a result of 3 s/be/have/ ::: servo/components/style/traversal.rs:169 (Diff revision 3) > + // If the traversal is animation-only and the element has > + // animating descendants bit, we need to traverse it. > + if animation_only && el.has_animating_descendants() { > + return true; > + } If animation_only is true and has_animating_descendants() is false, we should return false. Otherwise we will return true if has_dirty_descendants() is true.

Comment on attachment 8849726 [details] Bug 1344966 - Post eRestyle_CSSAnimations instead of eRestyle_Self and eRestyle_Subtree. https://reviewboard.mozilla.org/r/122512/#review125818

Comment on attachment 8849720 [details] Bug 1344966 - Add NODE_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO to represent that an element's descendant has animation. https://reviewboard.mozilla.org/r/122500/#review125772 > If you specifically want to re-use ELEMENT_HAS_PENDING_ANIMATION_ONLY_RESTYLE, then I think we should define it in nsINode.h as NODE_SHARED_RESTYLE_BIT_3, like we do with NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO. But I think it would be fine just to use NODE_SHARED_RESTYLE_BIT_2. > > Regarding the name: ELEMENT_HAS_ANIMATING_DESCENDANTS_FOR_SERVO sounds like a bit that would be set if an animation is running, rather than if we've posted an animation-only restyle. But NODE_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO seems a bit long. Not sure I have a better idea. Thanks. I use NODE_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO. Even though it's a bit long, we do scarcely use it directly.

Comment on attachment 8849723 [details] Bug 1344966 - Introduce a closure to replace rule nodes. https://reviewboard.mozilla.org/r/122506/#review125804 > Do we need to pass in guards? Can we just refer to |&context.shared.guards| from inside the closer instead? Good catch!

Comment on attachment 8849724 [details] Bug 1344966 - Introduce eRestyle_CSSAnimations. https://reviewboard.mozilla.org/r/122508/#review125808 > Can we assert in here that the animation_only_restyle flag on the context is true? Thanks for this advice. This assertion was super useful. This noticed me that a check in node_needs_traversal was wrong. Thanks!

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review125810 > If animation_only is true and has_animating_descendants() is false, we should return false. Otherwise we will return true if has_dirty_descendants() is true. Also we should return true in the case where the element has animation restyle hints. Thanks for making me realize it.

Comment on attachment 8849722 [details] Bug 1344966 - Add flag that represents the traversal is only for animation-only restyle. https://reviewboard.mozilla.org/r/122504/#review125796 > Will we ever have unstyled_children_only and animation_only both true? If not, then maybe we can assert or warn in that case, if it's unexpected. After some thought about this. We don't need the flag in PreTraverseToken. We just need the flag in SharedStyleContext. > It's not obvious to me why we should pass in true here. Shouldn't we be doing a normal restyle here? Can you explain? This was wrong. We should pass a boolean value whether the element has animation restyle hint or not.

Comment on attachment 8849722 [details] Bug 1344966 - Add flag that represents the traversal is only for animation-only restyle. https://reviewboard.mozilla.org/r/122504/#review126066 ::: servo/ports/geckolib/glue.rs:170 (Diff revision 3) > - let shared_style_context = create_shared_context(&guard, &per_doc_data); > + let shared_style_context = create_shared_context(&guard, &per_doc_data, > + element.has_animation_only_dirty_descendants()); I think it's a bit strange to be making the decision here, about whether to perform an animation-only restyle, rather than by the caller of traverse_subtree, especially since Servo_TraverseSubtree has just checked this. Do you think it might be better to have an argument to traverse_subtree, and then just pass that argument into create_shared_content? ::: servo/ports/geckolib/glue.rs:1509 (Diff revision 3) > - let shared = create_shared_context(&guard, &mut doc_data.borrow_mut()); > + let shared = create_shared_context(&guard, &mut doc_data.borrow_mut(), > + element.has_animation_restyle_hints()); I still don't understand this. Callers to Servo_ResolveStyleLazily are expecting to get up-to-date styles back from this function. If we happen to have animation restyles on the element, we won't get that, since we'll only perform an animation-only restyle (i.e., without a subsequent full restyle, like Servo_TraverseSubtree does). Do we really expect to have animation restyles around when we cal Servo_ResolveStyleLazily?

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review126068 ::: servo/components/style/traversal.rs:138 (Diff revision 4) > let _later_siblings = r.compute_final_hint(root, stylist); > } > } > > PreTraverseToken { > - traverse: Self::node_needs_traversal(root.as_node()), > + traverse: Self::node_needs_traversal(root.as_node(), root.has_animation_only_dirty_descendants()), If we give traverse_subtree an argument to indicate whether we should perform an animation-only restyle, then we should pass that in here, rather than checking has_animation_only_dirty_descendants(). In general, I think we should check once, up in Servo_TraverseSubtree, whether we have animation only restyles, and from then on, pass this boolean result to the functions that need to know whether we're doing an animation-only restyle or not. (And then use the flag on the SharedStyleContext to avoid explicit arguments as much as possible.) ::: servo/components/style/traversal.rs:152 (Diff revision 4) > debug_assert!(node.is_text_node()); > false > } > > /// Returns true if traversal is needed for the given node and subtree. > - fn node_needs_traversal(node: E::ConcreteNode) -> bool { > + fn node_needs_traversal(node: E::ConcreteNode, animation_only: bool) -> bool { Would it be better to remove the animation_only argument, and make node_needs_traversal() take a &self argument, and then just look up self.shared_context().animation_only_restyle? ::: servo/components/style/traversal.rs:268 (Diff revision 4) > - fn traverse_children<F>(&self, thread_local: &mut Self::ThreadLocalContext, parent: E, mut f: F) > + fn traverse_children<F>(&self, thread_local: > + &mut Self::ThreadLocalContext, > + parent: E, > + mut f: F) This wrapping looks strange. I think we can just leave this line as it is. ::: servo/components/style/traversal.rs:462 (Diff revision 4) > + if r.hint.has_animation_hint() { > + // Drop animation restyle hint. > + let propagated_hint = r.hint.propagate(); > + r.hint.remove_animation_hint(); > + propagated_hint > + } else { > - mem::replace(&mut r.hint, empty_hint).propagate() > + mem::replace(&mut r.hint, empty_hint).propagate() > + } Again, rather than checking has_animation_hint(), I think we should check context.shared.animation_only_restyle instead. ::: servo/components/style/traversal.rs:478 (Diff revision 4) > + "Should have computed style or haven't yet valid computed style in case of animation-only restyle"); > trace!("propagated_hint={:?}, inherited_style_changed={:?}", propagated_hint, inherited_style_changed); > > // Preprocess children, propagating restyle hints and handling sibling relationships. > if traversal.should_traverse_children(&mut context.thread_local, element, &data, DontLog) && > - (element.has_dirty_descendants() || !propagated_hint.is_empty() || inherited_style_changed) { > + (element.has_dirty_descendants() || element.has_animation_only_dirty_descendants() || Hmm, shouldn't we be only checking has_dirty_descendants() if we're in a regular restyle, and has_animation_only_dirty_descendants() if we're in an animation-only restyle?

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review126068 > If we give traverse_subtree an argument to indicate whether we should perform an animation-only restyle, then we should pass that in here, rather than checking has_animation_only_dirty_descendants(). > > In general, I think we should check once, up in Servo_TraverseSubtree, whether we have animation only restyles, and from then on, pass this boolean result to the functions that need to know whether we're doing an animation-only restyle or not. (And then use the flag on the SharedStyleContext to avoid explicit arguments as much as possible.) This node_needs_traversal is called before creating Traversal (here) and after creating Traversal (traversal_children), so we can't use SharedStyleContext for the former case. But yes, checking has_animation_only_dirty_descendants() here is somewhwat error prone. So I did introduce TraversalFlags, set the flags in Servo_TraverseSubtree() and pass the flag to pre_traverse(). > Again, rather than checking has_animation_hint(), I think we should check context.shared.animation_only_restyle instead. Here we should check the the element has animation hint instead of that element has animation-only-ditry-descendants. I added an assertion to check context.shared.animation_only_restyle in the if block.

Comment on attachment 8849722 [details] Bug 1344966 - Add flag that represents the traversal is only for animation-only restyle. https://reviewboard.mozilla.org/r/122504/#review126066 > I still don't understand this. Callers to Servo_ResolveStyleLazily are expecting to get up-to-date styles back from this function. If we happen to have animation restyles on the element, we won't get that, since we'll only perform an animation-only restyle (i.e., without a subsequent full restyle, like Servo_TraverseSubtree does). > > Do we really expect to have animation restyles around when we cal Servo_ResolveStyleLazily? I thought below case does call Servo_ResolveStyleLazily and needs animation-only restyle, but actually Servo_ResolveStyleLazily is not called at all. Sorry for the confusion. var div = document.createElement('div'); document.body.appendChild(div); div.style.transition = 'background-color 1s'; div.animate({ backgroundColor: ['red', 'blue'] }, 1000); getComputedStyle(div).backgroundColor;

(In reply to Hiroyuki Ikezoe (:hiro) from comment #48) > > If animation_only is true and has_animating_descendants() is false, we should return false. Otherwise we will return true if has_dirty_descendants() is true. > > Also we should return true in the case where the element has animation > restyle hints. Also we should return true in the case recascade flag is true, otherwise descendant elements are not correctly painted if parent animation property is inherited to the descendants.

Comment on attachment 8851211 [details] Bug 1344966 - Introduce TraversalFlags to represents target elements of the traversal we are about to do. https://reviewboard.mozilla.org/r/123576/#review126108 ::: servo/components/style/traversal.rs:51 (Diff revision 1) > + pub fn for_animation_only(&self) -> bool { > + self.contains(ANIMATION_ONLY) > + } > + > + /// Returns true if the traversal is for unstyled children. > + pub fn for_unstyled_children(&self) -> bool { Nit: for consistency with for_animation_only, I would name this for_unstyled_children_only. ::: servo/components/style/traversal.rs:134 (Diff revision 1) > /// The unstyled_children_only parameter is used in Gecko to style newly- > /// appended children without restyling the parent. Please update this comment to refer to traversal_flags/UNSTYLED_CHILDREN_ONLY. (And also mention what ANIMATION_ONLY is used for, in the "Process animation-only traversal." patch.) ::: servo/components/style/traversal.rs:136 (Diff revision 1) > /// a traversal is needed. Returns a token that allows the caller to prove > /// that the call happened. > /// > /// The unstyled_children_only parameter is used in Gecko to style newly- > /// appended children without restyling the parent. > - fn pre_traverse(root: E, stylist: &Stylist, unstyled_children_only: bool) > + fn pre_traverse(root: E, stylist: &Stylist, traversal_flags: &TraversalFlags) The bitflags!-generated type implements Copy, so I think we should just make this argument's type |TraversalFlags| instead of |&TraversalFlags|. ::: servo/ports/geckolib/glue.rs:147 (Diff revision 1) > quirks_mode: QuirksMode::NoQuirks, > } > } > > fn traverse_subtree(element: GeckoElement, raw_data: RawServoStyleSetBorrowed, > - unstyled_children_only: bool) { > + traversal_flags: &TraversalFlags) { Here too.

Comment on attachment 8849722 [details] Bug 1344966 - Add flag that represents the traversal is only for animation-only restyle. https://reviewboard.mozilla.org/r/122504/#review126112

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review126110 ::: servo/ports/geckolib/glue.rs:200 (Diff revision 6) > + let mut traversal_flags = TraversalFlags::empty(); > + if behavior == structs::TraversalRootBehavior::UnstyledChildrenOnly { > + traversal_flags.insert(UNSTYLED_CHILDREN_ONLY); > + } > + > + if element.has_animation_only_dirty_descendants() || > + element.has_animation_restyle_hints() { > + traversal_flags.insert(ANIMATION_ONLY); > + traverse_subtree(element, raw_data, &traversal_flags); > + traversal_flags.remove(ANIMATION_ONLY); > + } > + > + traverse_subtree(element, raw_data, &traversal_flags); Nit: rather than have a mut variable, I think it would be cleaner to write: let traversal_flags = match behavior { structs::TraversalRootBehavior::UnstyledChildrenOnly => UNSTYLED_CHILDREN_ONLY, _ => TraversalFlags::empty(), }; if ... { traverse_subtree(element, raw_data, traversal_flags | ANIMATION_ONLY); } traverse_subtree(element, raw_data, traversal_flags);

Comment on attachment 8851298 [details] Bug 1344966 - Post eRestyle_CSSAnimations instead of eRestyle_Self and eRestyle_Subtree. https://reviewboard.mozilla.org/r/123644/#review126114

Comment on attachment 8849725 [details] Bug 1344966 - Process animation-only traversal. https://reviewboard.mozilla.org/r/122510/#review126110 > Nit: rather than have a mut variable, I think it would be cleaner to write: > > let traversal_flags = match behavior { > structs::TraversalRootBehavior::UnstyledChildrenOnly => UNSTYLED_CHILDREN_ONLY, > _ => TraversalFlags::empty(), > }; > > if ... { > traverse_subtree(element, raw_data, traversal_flags | ANIMATION_ONLY); > } > > traverse_subtree(element, raw_data, traversal_flags); Impressive! Thanks!

A final try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2458860b4c70b00163d04f6afdfe93a6d1cfdb I will land these patches send PR to servo tomorrow.

Hiro, would you mind noting down the restyle flow for current stylo animation, just like what you did in Bug 1341985 Comment 106 into [1]? So it would be easier to recall that. [1] https://public.etherpad-mozilla.org/p/stylo-animation

(In reply to Boris Chiou [:boris] from comment #78) > Hiro, would you mind noting down the restyle flow for current stylo > animation, just like what you did in Bug 1341985 Comment 106 into [1]? So it > would be easier to recall that. > > [1] https://public.etherpad-mozilla.org/p/stylo-animation https://public.etherpad-mozilla.org/p/stylo-animation#lineNumber=35

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1552119b94a1 Add NODE_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO to represent that an element's descendant has animation. r=heycam https://hg.mozilla.org/integration/autoland/rev/43143f8b9fa1 Post eRestyle_CSSAnimations instead of eRestyle_Self and eRestyle_Subtree. r=heycam

https://hg.mozilla.org/mozilla-central/rev/1552119b94a1 https://hg.mozilla.org/mozilla-central/rev/43143f8b9fa1