Closed Bug 1364799 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: hiro)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(4 files, 1 obsolete file)

testcase
326 bytes, text/html
Details
Bug 1364799 - Add PostRestyleEventForCSSRuleChanges to distinguish PostRestyleEvent.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1364799 - Add a new TraversalRestyleBehavior that represents the traversal is triggered by CSS rule changes.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1364799 - Update mochitest expectations.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file testcase — 
See the attached testcase. Inserting a new @keyframes doesn't trigger restyle as expected on Stylo.
I am not yet sure this bug should depend on bug 1344581 or not.
Blocks: stylo-css-animations
URL: 1344581
May even just a dupe. I'm not sure.
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
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)
(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.
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 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+
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 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 on attachment 8868972 [details]
Bug 1364799 - Update mochitest expectations.

https://reviewboard.mozilla.org/r/140644/#review144376
Attachment #8868972 - Flags: review?(bbirtles) → 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 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+
Attachment #8868971 - Attachment is obsolete: true
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: