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

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

3 months ago
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.
Assignee: nobody → boris.chiou
Depends on: 1344966
Assignee: boris.chiou → hikezoe
(Assignee)

Updated

3 months ago
No longer depends on: 1344966
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

3 months ago
mozreview-review
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
Attachment #8850420 - Flags: review?(emilio+bugs) → review+

Comment 13

3 months ago
mozreview-review
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.
Attachment #8850421 - Flags: review?(emilio+bugs) → review+

Comment 14

3 months ago
mozreview-review
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?
Attachment #8850422 - Flags: review?(emilio+bugs) → review+

Comment 15

3 months ago
mozreview-review
Comment on attachment 8850423 [details]
Bug 1346663 - Make cascade_internal() reusable with rule nodes.

https://reviewboard.mozilla.org/r/123016/#review125438
Attachment #8850423 - Flags: review?(emilio+bugs) → review+

Comment 16

3 months ago
mozreview-review
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?
Attachment #8850424 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 17

3 months ago
(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!
(Assignee)

Comment 18

3 months ago
(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!
(Assignee)

Comment 19

3 months ago
https://github.com/servo/servo/pull/16111
(Assignee)

Comment 20

3 months ago
Fixed.
https://hg.mozilla.org/integration/autoland/rev/6b9bbd396faca96f2e4549dab53b82557fe22403
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> Fixed.
> https://hg.mozilla.org/integration/autoland/rev/
> 6b9bbd396faca96f2e4549dab53b82557fe22403

Cool!!
You need to log in before you can comment on or make changes to this bug.