1346663 - stylo: Add a function that returns computed values without animation/transition rules

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

This is needed for CSS transition and additive or accumulative animations.
I guess this is also needed for SMIL, but for SMIL, the function might be slightly different, I am not sure.

Also I am guessing we might want computed values without animation rules for *a property* in future.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1346663 - Drop mut from pseudo_style for the argument of cascade_internal.

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1346663 - Add a function that returns new rules by removing transition level rule.

This will be used for after-change-style of CSS Transition.

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1346663 - Add a function that returns new rules by removing transition and animation level rules.

This will be used for additive animations and SMIL.

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1346663 - Make cascade_internal() reusable with rule nodes.

A new function, cascade_with_rules, takes StrongRuleNode
and computes cascading result values with the StrongRuleNode.
This new function will be used for getting after-change-style
for CSS Transitions.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1346663 - Add a function to get after-change style for CSS Transition.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

The patch set adds two function to RuleTree:
 1. a function that returns StrongRuleNode after removing transition level rule from a given StrongRuleNode
 2. a function that returns StrongRuleNode after removing transition and animation level rules from a given StrongRuleNode

1 is used for creating after-change style which is defined in CSS Transition spec.
2 is used for getting base style values for additive animation or SMIL animation.

I did confirm both of functions remove subject level rules from a given computed values with some tweaks (Actually we don't support CSS transitions yet, so I did inject CSS animations style values into transition level).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8850420 [details]
Bug 1346663 - Drop mut from pseudo_style for the argument of cascade_internal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123010/diff/1-2/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8850421 [details]
Bug 1346663 - Add a function that returns new rules by removing transition level rule.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123012/diff/1-2/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8850422 [details]
Bug 1346663 - Add a function that returns new rules by removing transition and animation level rules.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123014/diff/1-2/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8850423 [details]
Bug 1346663 - Make cascade_internal() reusable with rule nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123016/diff/1-2/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8850424 [details]
Bug 1346663 - Add a function to get after-change style for CSS Transition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123018/diff/1-2/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850420 [details]
Bug 1346663 - Drop mut from pseudo_style for the argument of cascade_internal.

https://reviewboard.mozilla.org/r/123010/#review125430

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850421 [details]
Bug 1346663 - Add a function that returns new rules by removing transition level rule.

https://reviewboard.mozilla.org/r/123012/#review125432

::: servo/components/style/rule_tree/mod.rs:244
(Diff revision 2)
>          // necessary.
>          Some(self.insert_ordered_rules_from(current, children.into_iter().rev()))
>      }
> +
> +    /// Returns new rule nodes without Transitions level rule.
> +    /// Note: this funciton supposes that rule nodes have been already ordered.

nit: The rule nodes are always sorted, so no need for this last comment.

::: servo/components/style/rule_tree/mod.rs:245
(Diff revision 2)
>          Some(self.insert_ordered_rules_from(current, children.into_iter().rev()))
>      }
> +
> +    /// Returns new rule nodes without Transitions level rule.
> +    /// Note: this funciton supposes that rule nodes have been already ordered.
> +    pub fn remove_transition_rule(&self, path: &StrongRuleNode) -> StrongRuleNode {

let's rename it to `remove_transition_rule_if_applicable`, so it's clear that it handles not having a transition rule.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850422 [details]
Bug 1346663 - Add a function that returns new rules by removing transition and animation level rules.

https://reviewboard.mozilla.org/r/123014/#review125434

::: servo/components/style/rule_tree/mod.rs:255
(Diff revision 2)
>  
>          path.parent().unwrap().clone()
>      }
> +
> +    /// Returns new rule node without Animations and Transitions level rules.
> +    /// Note: this funciton supposes that rule nodes have been already ordered.

nit: Ditto about the sorting.

::: servo/components/style/rule_tree/mod.rs:256
(Diff revision 2)
>          path.parent().unwrap().clone()
>      }
> +
> +    /// Returns new rule node without Animations and Transitions level rules.
> +    /// Note: this funciton supposes that rule nodes have been already ordered.
> +    pub fn remove_animation_rules(&self, path: &StrongRuleNode) -> StrongRuleNode {

nit: I'd prefer to mention also `transition` in the function name, so it's a bit clearer. Feel free to drop it if you think it's too verbose though.

::: servo/components/style/rule_tree/mod.rs:258
(Diff revision 2)
> +
> +    /// Returns new rule node without Animations and Transitions level rules.
> +    /// Note: this funciton supposes that rule nodes have been already ordered.
> +    pub fn remove_animation_rules(&self, path: &StrongRuleNode) -> StrongRuleNode {
> +        // Return a clone if there is neither animation nor transition level.
> +        if !path.self_and_ancestors().take_while(|node| node.cascade_level() >= CascadeLevel::Animations)

Perhaps extract this condition to a method in `StrongRuleNode`?

`StrongRuleNode::has_animation_or_transition_rules` maybe?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850423 [details]
Bug 1346663 - Make cascade_internal() reusable with rule nodes.

https://reviewboard.mozilla.org/r/123016/#review125438

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8850424 [details]
Bug 1346663 - Add a function to get after-change style for CSS Transition.

https://reviewboard.mozilla.org/r/123018/#review125442

::: servo/components/style/matching.rs:616
(Diff revision 2)
> +                              pseudo_style: &Option<(&PseudoElement, &mut ComputedStyle)>)
> +                              -> Arc<ComputedValues> {
> +        let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules;
> +        let without_transition_rules =
> +            context.shared.stylist.rule_tree.remove_transition_rule(rule_node);
> +        let cascade_flags = CascadeFlags::empty();

We still may need the "ignore fixup" flag right?

::: servo/components/style/matching.rs:617
(Diff revision 2)
> +                              -> Arc<ComputedValues> {
> +        let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules;
> +        let without_transition_rules =
> +            context.shared.stylist.rule_tree.remove_transition_rule(rule_node);
> +        let cascade_flags = CascadeFlags::empty();
> +        self.cascade_with_rules(context,

This is fine, but we can optimize this in the (common? I guess that depends on when do you plan to call this) case there are no transition or animation rules, to avoid doing the cascade:

```
let style = pseudo_style.as_ref().map_or(primary_style, |p| &*p.1);
let rule_node = &style.rules;

let without_transition_rules =
        context.shared.stylist.rule_tree.remove_transition_rule(rule_node);
if without_transition_rules == rule_node {
    // Note that unwrapping here is fine, because the style is
    // only incomplete during the styling process.
    return style.values.as_ref().unwrap().clone();
}

self.cascade_with_rules(context, ...)
```

Seems like a pretty easy optimization, what do you think?

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> ::: servo/components/style/rule_tree/mod.rs:256
> (Diff revision 2)
> >          path.parent().unwrap().clone()
> >      }
> > +
> > +    /// Returns new rule node without Animations and Transitions level rules.
> > +    /// Note: this funciton supposes that rule nodes have been already ordered.
> > +    pub fn remove_animation_rules(&self, path: &StrongRuleNode) -> StrongRuleNode {
> 
> nit: I'd prefer to mention also `transition` in the function name, so it's a
> bit clearer. Feel free to drop it if you think it's too verbose though.

I will add 'transition' there.

> ::: servo/components/style/rule_tree/mod.rs:258
> (Diff revision 2)
> > +
> > +    /// Returns new rule node without Animations and Transitions level rules.
> > +    /// Note: this funciton supposes that rule nodes have been already ordered.
> > +    pub fn remove_animation_rules(&self, path: &StrongRuleNode) -> StrongRuleNode {
> > +        // Return a clone if there is neither animation nor transition level.
> > +        if !path.self_and_ancestors().take_while(|node| node.cascade_level() >= CascadeLevel::Animations)
> 
> Perhaps extract this condition to a method in `StrongRuleNode`?
> 
> `StrongRuleNode::has_animation_or_transition_rules` maybe?

Sounds nice idea. I will do it. Thanks!

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> ::: servo/components/style/matching.rs:617
> (Diff revision 2)
> > +                              -> Arc<ComputedValues> {
> > +        let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules;
> > +        let without_transition_rules =
> > +            context.shared.stylist.rule_tree.remove_transition_rule(rule_node);
> > +        let cascade_flags = CascadeFlags::empty();
> > +        self.cascade_with_rules(context,
> 
> This is fine, but we can optimize this in the (common? I guess that depends
> on when do you plan to call this) case there are no transition or animation
> rules, to avoid doing the cascade:
> 
> ```
> let style = pseudo_style.as_ref().map_or(primary_style, |p| &*p.1);
> let rule_node = &style.rules;
> 
> let without_transition_rules =
>         context.shared.stylist.rule_tree.remove_transition_rule(rule_node);
> if without_transition_rules == rule_node {
>     // Note that unwrapping here is fine, because the style is
>     // only incomplete during the styling process.
>     return style.values.as_ref().unwrap().clone();
> }
> 
> self.cascade_with_rules(context, ...)
> ```
> 
> Seems like a pretty easy optimization, what do you think?

Oh neat! I will do this as well. Thanks!

Comment 19

[Hiroyuki Ikezoe (:hiro)]

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

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Fixed.
https://hg.mozilla.org/integration/autoland/rev/6b9bbd396faca96f2e4549dab53b82557fe22403

Comment 21

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> Fixed.
> https://hg.mozilla.org/integration/autoland/rev/
> 6b9bbd396faca96f2e4549dab53b82557fe22403

Cool!!