stylo: Inserting new @keyframes rule doesn't trigger restyle

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: xidorn, Assigned: hiro)

Tracking

(Blocks: 3 bugs)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(URL)

MozReview Requests

()

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 months ago
Created attachment 8867580 [details]
testcase

See the attached testcase. Inserting a new @keyframes doesn't trigger restyle as expected on Stylo.
(Assignee)

Comment 1

2 months ago
I am not yet sure this bug should depend on bug 1344581 or not.
Blocks: 1288269
URL: 1344581
(Reporter)

Comment 2

2 months ago
May even just a dupe. I'm not sure.
(Assignee)

Comment 3

2 months ago
Maybe, but it depends on how we fix bug 1344581. Actually I did miss the case in comment 0. So, I'd like to keep this bug open. Thanks for filing!
Assignee: nobody → hikezoe
Priority: -- → P2
(Assignee)

Comment 4

2 months ago
I've been thinking how to update CSS animations in this case efficiently, but don't come up with any good idea yet.

What gecko does in this case is that:

1. Post eRestyle_Subtree to the root element in nsIPresShell::RestyleForCSSRuleChanges()
2. Call UpdateAnimations for *all* descendant elements in nsStyleSet::GetContext()

If we try to do the same way in stylo, we need to create lots of SequentialTasks even if it's not the case that style sheet is changed. It's really horrible.

A solution what I can think of to reduce the number of SequentialTask is:

1) Add a new restyle hint that represents that it's caused by RestyleForCSSRuleChanges(). (or add another argument to PostRestyleEvent?)
2) If the flag added 1) is true and the target element has an animation style, create a SequentialTask.
3) For further optimization in a later bug, we store newly added keyframes' names in stylist.rebuild and check the names before creating the SequentialTask.

In 3), I think we can also store updated/removed keyframes.

Brian, does this way sound reasonable?
Flags: needinfo?(bbirtles)
That makes sense to me.

I guess we can't do it the other direction, i.e. iterate from the nsAnimationManager, because if an element has animation styles that refer to a previously absent @keyframes rule, they won't be registered with nsAnimationManager.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 6

2 months ago
(In reply to Brian Birtles (:birtles) from comment #5)
> That makes sense to me.
> 
> I guess we can't do it the other direction, i.e. iterate from the
> nsAnimationManager, because if an element has animation styles that refer to
> a previously absent @keyframes rule, they won't be registered with
> nsAnimationManager.

Thank you!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> 1) Add a new restyle hint that represents that it's caused by
> RestyleForCSSRuleChanges(). (or add another argument to PostRestyleEvent?)

I am now thinking whether we should add an argument to RestyleManager::PostRestyleEvent() or add a new function to set the state that a subsequent ProcessPendingRestyles() was caused by RestyleForCSSRuleChange().  I am now thinking using restyle hint for the purpose does not match restyle hints.
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1344581
(Assignee)

Comment 8

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b1d356eb1acfce25bc1c6333ef28d659f1d99b5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8868969 [details]
Bug 1364799 - Add PostRestyleEventForCSSRuleChanges to distinguish PostRestyleEvent.

https://reviewboard.mozilla.org/r/140638/#review144028

::: layout/base/RestyleManagerInlines.h:31
(Diff revision 1)
>  
>  void
> +RestyleManager::PostRestyleEventForCSSRulesChanges(dom::Element* aElement,
> +                                                   nsRestyleHint aRestyleHint,
> +                                                   nsChangeHint aMinChangeHint)
> +{

I did add an assertion [1] here initially. But unfortunately it will not work until bug 1345702 is fixed.

[1] https://hg.mozilla.org/try/rev/ef93229718a6b24c16ecbc95a7d7288a596362be#l4.16

Comment 14

2 months ago
mozreview-review
Comment on attachment 8868969 [details]
Bug 1364799 - Add PostRestyleEventForCSSRuleChanges to distinguish PostRestyleEvent.

https://reviewboard.mozilla.org/r/140638/#review144370

This seems fine to me but I'd like Cameron to have a look. He'll know if there is a better way of doing this (e.g. if we can overload restyle hints for this).

::: layout/base/PresShell.cpp:4587
(Diff revision 1)
>  
>    if (scopeRoots.IsEmpty()) {
>      // If scopeRoots is empty, we know that mStylesHaveChanged was true at
>      // the beginning of this function, and that we need to restyle the whole
>      // document.
> -    restyleManager->PostRestyleEvent(root, eRestyle_Subtree,
> +    restyleManager->PostRestyleEventForCSSRulesChanges(root,

This should be ...ForCSSRuleChanges, i.e. no 's' after 'Rule' in order to match nsIPresShell::RestyleForCSSRuleChanges.

::: layout/base/ServoRestyleManager.h:165
(Diff revision 1)
>    // increase mAnimationGeneration before creating new transitions, so their
>    // creation sequence will be correct.
>    bool mHaveNonAnimationRestyles = false;
>  
> +  // Set to true when posting restyle events triggered by CSS rules changes.
> +  // This flag is cleared once ProcessPendingRestyles has done.

(Nit: either 'is done' or 'has completed')

::: layout/base/ServoRestyleManager.h:166
(Diff revision 1)
>    // creation sequence will be correct.
>    bool mHaveNonAnimationRestyles = false;
>  
> +  // Set to true when posting restyle events triggered by CSS rules changes.
> +  // This flag is cleared once ProcessPendingRestyles has done.
> +  bool mRestyleForCSSRulesChanges = false;

Likewise 'mRestyleForCSSRuleChanges' (no 's' after 'Rule')
Attachment #8868969 - Flags: review?(bbirtles) → review+
Attachment #8868969 - Flags: review?(cam)

Comment 15

2 months ago
mozreview-review
Comment on attachment 8868970 [details]
Bug 1364799 - Add a new TraversalRestyleBehavior that represents the traversal is triggered by CSS rule changes.

https://reviewboard.mozilla.org/r/140640/#review144374

This seems fine to me but will probably depend on Cameron's feedback on the first patch.

::: layout/style/ServoStyleSet.h:238
(Diff revision 1)
>     * Performs a Servo traversal to compute style for all dirty nodes in the
>     * document.
>     *
>     * This will traverse all of the document's style roots (that is, its document
>     * element, and the roots of the document-level native anonymous content).
>     *
>     * Returns true if a post-traversal is required.
>     */
> -  bool StyleDocument();
> +  bool StyleDocument(TraversalRestyleBehavior aRestyleBehavior);

We should probably document what the aRestyleBehavior flag does (including mentioning the range of expected values).

::: layout/style/ServoTypes.h:64
(Diff revision 1)
>  // hints in this case are unneeded, since the old frames have already been
>  // destroyed.
>  enum class TraversalRestyleBehavior {
>    Normal,
>    ForReconstruct,
> +  ForCSSRulesChanges,

ForCSSRuleChanges

::: servo/components/style/traversal.rs:39
(Diff revision 1)
>          const UNSTYLED_CHILDREN_ONLY = 0x01,
>          /// Traverse only elements for animation restyles.
>          const ANIMATION_ONLY = 0x02,
>          /// Traverse without generating any change hints.
>          const FOR_RECONSTRUCT = 0x04,
> +        /// Traverse triggered by CSS rules changes.

"CSS rule changes"

Can we briefly mention what the significance of this is? e.g.

"Traverse and update all elements with CSS animations since @keyframes rules may have changed" ?

::: servo/components/style/traversal.rs:60
(Diff revision 1)
>      /// Returns true if the traversal is for a frame reconstruction.
>      pub fn for_reconstruct(&self) -> bool {
>          self.contains(FOR_RECONSTRUCT)
>      }
> +
> +    /// Returns true if the traversal is triggered by CSS rules changes.

CSS rule changes
Attachment #8868970 - Flags: review?(bbirtles) → review+

Comment 16

2 months ago
mozreview-review
Comment on attachment 8868971 [details]
Bug 1364799 - Create a SequentialTask for updating CSS animations in the case where the traversal is triggered by CSS rule changes.

https://reviewboard.mozilla.org/r/140642/#review144380
Attachment #8868971 - Flags: review?(bbirtles) → review+

Comment 17

2 months ago
mozreview-review
Comment on attachment 8868972 [details]
Bug 1364799 - Update mochitest expectations.

https://reviewboard.mozilla.org/r/140644/#review144376
Attachment #8868972 - Flags: review?(bbirtles) → review+

Comment 18

2 months ago
mozreview-review
Comment on attachment 8868970 [details]
Bug 1364799 - Add a new TraversalRestyleBehavior that represents the traversal is triggered by CSS rule changes.

https://reviewboard.mozilla.org/r/140640/#review144382

::: layout/style/ServoTypes.h:56
(Diff revision 1)
>  // Indicates whether the Servo style system should perform normal processing or
>  // whether it should traverse in a mode that doesn't generate any change hints,
>  // which is what's required when handling frame reconstruction.  The change
>  // hints in this case are unneeded, since the old frames have already been
>  // destroyed.

Please document the new value in here.

Comment 19

2 months ago
mozreview-review
Comment on attachment 8868969 [details]
Bug 1364799 - Add PostRestyleEventForCSSRuleChanges to distinguish PostRestyleEvent.

https://reviewboard.mozilla.org/r/140638/#review144378

I think it's right to new a traversal mode flag rather than a restyle hint.

But I wonder if we should be trying setting this flag only if we know that @keyframes rules are affected?  Maybe it doesn't matter so much, since we do a lot of other work when restyling after rules have changed anyway.

::: layout/base/ServoRestyleManager.h:164
(Diff revision 1)
> +  // Set to true when posting restyle events triggered by CSS rules changes.
> +  // This flag is cleared once ProcessPendingRestyles has done.

Can you please document in here why we need to track this state.
Attachment #8868969 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 months ago
Thanks for the review!
A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28a5d4bf3f96e8d65b95a33949b860722fc686ea
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8868971 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 months ago
Now servo PR gets merged. Unfortunately autoland is still closed. I had to land gecko side patches blindly.

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

Comment 36

2 months ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3bce1540107c
Add PostRestyleEventForCSSRuleChanges to distinguish PostRestyleEvent. r=birtles,heycam
https://hg.mozilla.org/integration/autoland/rev/50ede6235240
Add a new TraversalRestyleBehavior that represents the traversal is triggered by CSS rule changes. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f5d0213a9d16
Update mochitest expectations. r=birtles

Comment 37

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3bce1540107c
https://hg.mozilla.org/mozilla-central/rev/50ede6235240
https://hg.mozilla.org/mozilla-central/rev/f5d0213a9d16
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.