1375787 - stylo: Need to check has_new_animation_style flag along with traversal_flags.for_css_rule_changes() in needs_animations_update()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Hiroyuki Ikezoe (:hiro)]

If there is no CSS animation on the element in the case the traversal is invoked by FOR_CSS_RULE_CHANGES, we don't need to call update_animations() at all.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67b92d51d7c5ce7d7e87271c1dd79ef07addd279

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The title was not quite right. We need to check has_new_animation_style instead.  For example, an element has an animation style but there is no @keyframes rules corresponding to the animation style.  In this case the element has no running animation, jut has the animation style.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=785dde833f04f5a749ac83ce71380d43dd11493e

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1375787 - Check has_new_animation_style along with for_css_rule_changes() in needs_animations_update().

Review commit: https://reviewboard.mozilla.org/r/152336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152336/

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8880977 [details]
Bug 1375787 - Check has_new_animation_style along with for_css_rule_changes() in needs_animations_update().

https://reviewboard.mozilla.org/r/152336/#review157588

::: servo/components/style/matching.rs:656
(Diff revision 1)
>          old_values.map_or(has_new_animation_style, |old| {
>              let old_box_style = old.get_box();
>              let old_display_style = old_box_style.clone_display();
>              let new_display_style = new_box_style.clone_display();
>  
> -            // If the traverse is triggered by CSS rule changes,
> +            // In case the traverse is triggered by CSS rule changes,

"If the traverse..." is fine

::: servo/components/style/matching.rs:661
(Diff revision 1)
> -            // If the traverse is triggered by CSS rule changes,
> -            // we need to try to update all CSS animations.
> -            context.shared.traversal_flags.for_css_rule_changes() ||
> +            // In case the traverse is triggered by CSS rule changes,
> +            // we need to try to update all CSS animations on the element
> +            // if the element has CSS animation style regardless of whether
> +            // the animation is running or not.
> +            // TODO: We should check which @keyframes changed/added/deleted
> +            // and update only animations corresponding to the @keyframes.

Nit: ...corresponding to those @keyframes.

::: servo/components/style/matching.rs:663
(Diff revision 1)
> -            context.shared.traversal_flags.for_css_rule_changes() ||
> +            // if the element has CSS animation style regardless of whether
> +            // the animation is running or not.
> +            // TODO: We should check which @keyframes changed/added/deleted
> +            // and update only animations corresponding to the @keyframes.
> +            (context.shared.traversal_flags.for_css_rule_changes() &&
> +             (has_new_animation_style)) ||

I don't think we need the () around has_new_animation_style here

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Pushed a try based on current autoland just in case.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b4476ff869c9b1c32085ea2c5bb7078306beb82

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://github.com/servo/servo/pull/17526

Comment 8

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/autoland/rev/ab348cba6c56cf0dfb4acaccee1bfa262ef3b2d8