The default bug view has changed. See this FAQ.

stylo: Implement eRestyle_CSSAnimations and eRestyle_CSSTransitions to avoid triggering CSS transitions due to style changes caused by animations

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
16 days ago
18 hours 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

(7 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
59 bytes, text/x-review-board-request
hiro
: review?
heycam
Details | Review
(Assignee)

Description

16 days ago
We need eRestyle_CSSAnimations and eRestyle_CSSTransitions to know whether the style changes are caused by normal style change or animation style change.
(Assignee)

Comment 1

15 days ago
Created attachment 8844720 [details] [diff] [review]
Implement eRestyle_CSSAnimations (very early stage)

I am totally unfamiliar with this area, so I don't know how much work is required for completion. 

A thing what I realize that we should do is:

* propagate RESTYLE_CSS_ANIMATIONS even if the element has other restyle hints

A good news with this patch is that I am feeling animation moving smoother. yay!
(Assignee)

Comment 2

10 days ago
For reference, I just pushed a try with WIP patches: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3a66a8fe16aeec185a26afecc811448239cf1d

Actually this does not work well yet, with these patches some tests in test_animations.html fails.  There must be something that I am missing.  But anyway, I don't want to block Boris's work for transitions by this bug.

Gosh! I did forget to add try syntax, but it doesn't matter since it won't work. :-p
I only had a glancing look at the patches, but don't we need something like the logic in GeckoRestyleManager::ProcessPendingRestyles() in the ServoRestyleManager for this to work?

That is we do the following:

If we have any restyles that are *not* animation restyles:

 * Make a separate RestyleTracker
 * Get EffectCompositor to add to the RestyleTracker restyles for all elements that have animation restyles
   (including throttled restyles)
 * Set a flag on the transition manager to say "animation-only" restyle so it knows not to generate transitions
 * Process the restyles in the separate restyle tracker (i.e. the animation restyles)
 * Clear the flag on the transition manager
 * Process the remaining restyles

If we *only* have animation restyles:

 * Set the flag on the transition manager to say "animation-only" restyle
 * Process the restyles
 * Clear the flag on the transition manager
(Assignee)

Comment 4

10 days ago
Thanks.  I should note that the failure I mentioned in comment 2 is not related to the animation-only restyles because we have no transitions on stylo yet.

For the animation-only restyles I think we can re-use the second traversal for CSS animation for animation-only restyles, I am not 100% sure though.

Updated

10 days ago
Blocks: 1341372
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> For the animation-only restyles I think we can re-use the second traversal
> for CSS animation for animation-only restyles, I am not 100% sure though.

The animation restyle needs to happen first so that during the main traversal (i.e. the current "first" traversal) we can produce the correct before and after change styles for generating transitions.

So, for the case where, say, an animation-name property changes and we have animation restyles pending, we will have three restyles:

* animation-only restyle
* main restyle where we generate the animation changes
* "second" restyle where we apply the animation changes
(Assignee)

Comment 6

10 days ago
(In reply to Brian Birtles (:birtles) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > For the animation-only restyles I think we can re-use the second traversal
> > for CSS animation for animation-only restyles, I am not 100% sure though.
> 
> The animation restyle needs to happen first so that during the main
> traversal (i.e. the current "first" traversal) we can produce the correct
> before and after change styles for generating transitions.

Gah. CSS transitions need animation styles for the before and after changes?  Can't we somehow the animation-only restyles in the first normal traversal?  If we really need the animation-only restyles as the zero-th traversal, I am yet too unfamiliar with servo's traversal to introduce the zero-th traversal.
The reason we do it that way is that we need to distinguish between changes due to animation and other changes. So, for example, if we just apply the animation change and the other changes at once, the before and after change styles won't match but we won't know if that's due to animations ticking (which we should ignore) or due to real changes that should generate transitions.

We used to do it differently by using a cover rule that--if my memory is correct--we applied to the element to hide animation styles. However, we changed that to use two phases instead. Bug 960465 (and bug 996796) has some description about why this change was made. We probably need to look into that bug a bit more.
(Assignee)

Comment 8

10 days ago
OK. I found the reason of the failure.  The reason is bug 1339704.  In RESTYLE_CSS_ANIMATIONS restyle we update only CSS animation level rule. Bug unfortunately the wrong transition level rule still persists there. So the wrong style overrides the updated CSS animation level style.  I am inclined to return None for transition level for now in get_animation_rules().

As for the animation-only restyle I am guessing it in the first traversal.
Comment hidden (obsolete)
Comment hidden (obsolete)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> OK. I found the reason of the failure.  The reason is bug 1339704.  In
> RESTYLE_CSS_ANIMATIONS restyle we update only CSS animation level rule. Bug
> unfortunately the wrong transition level rule still persists there. So the
> wrong style overrides the updated CSS animation level style.  I am inclined
> to return None for transition level for now in get_animation_rules().
> 
> As for the animation-only restyle I am guessing it in the first traversal.

I just updated Bug 1339704, and verified it by Bug 1339704 Comment 0. We don't trigger transition yet, but I think it will work while both transition and animations happen.
Assignee: nobody → hikezoe
Priority: -- → P1

Updated

6 days ago
Blocks: 1346663
(Assignee)

Comment 12

a day ago
Summary of what I am going to do in this bug:

* Add a new bit to element to represents that a descendant has animations.
  We can traverse down dom tree with this bit to find elements that have
  animations.
* When eRestyle_CSSAnimations is posted to an element, set this bit to
  ancestors of the element and set RESTYLE_CSS_ANIMATIONS hint to the element.
* If the root node has this bit, we treat the traversal as *animation-only*
  traversal.
* In the animation-only traversal restyle elements that has
  RESTYLE_CSS_ANIMATIONS hint and remove the hint.
  (The new bit is also cleared while traversing down dom tree)
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 20

a day ago
mozreview-review
Comment on attachment 8849725 [details]
Bug 1344966 - Process animation-only traversal.

https://reviewboard.mozilla.org/r/122510/#review124662

::: servo/components/style/traversal.rs:285
(Diff revision 1)
> -                              .map_or(false, |d| d.has_styles())
> -                {
> +                              .map_or(false, |d| d.has_styles()) {
> +                    // XXX: Set the dirty descendtants bit if this is not animation-only restyle?
>                      unsafe { parent.set_dirty_descendants(); }
>                  }

One point I don't quite understand is here.
I think we don't need to set the dirtry descendants bit in case of animation-only traversal. But without setting the dirty bit here, no visually change happens on animating element (whereas animation value is surely computed).
(Assignee)

Comment 21

a day ago
These patch set actually works fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c18b8f8edd4f7a6565faab73476e377e67aafb05

But I'd like to know the purpose of the set_dirty_descendants() in comment 20.
Bobby, could you please tell me about the purpose of the set_dirty_descendants() in traverse_children()?
Maybe I am missing an important thing about the dirty bit.

Note that I did introduce another bit for animation-only restyle in the first patch.
Flags: needinfo?(bobbyholley)
A general comment here (not in response to the last 2 comments):

I think the key thing here is that transitions depend on is that the style changes that result from the refresh driver's time advancing (which do not trigger transitions) need to be separated from other style changes (which do trigger transitions).  Gecko handles this by calling UpdateOnlyAnimationStyles and recording the state with nsTransitionManager::SetInAnimationOnlyStyleUpdate.

Gecko currently (in GeckoRestyleManager::PostRestyleEvent) uses the somewhat fragile mechanism that the set of restyle hints posted by animation ticking as the result of refresh driver time advance, and the set of restyle hints posted for other things, are disjoint.  This is (as the comment there says) somewhat fragile.

If you want to depend on that mechanism, you similarly need to ensure that the sets are disjoint.
(Assignee)

Comment 23

a day ago
Thank you David for your quick useful response.

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #22)
> I think the key thing here is that transitions depend on is that the style
> changes that result from the refresh driver's time advancing (which do not
> trigger transitions) need to be separated from other style changes (which do
> trigger transitions).  Gecko handles this by calling
> UpdateOnlyAnimationStyles and recording the state with
> nsTransitionManager::SetInAnimationOnlyStyleUpdate.

I think I did these in the patch set.  Animation-only traversal before normal traversal and
added animation_only_restyle flag to SharedStyleContext.

> Gecko currently (in GeckoRestyleManager::PostRestyleEvent) uses the somewhat
> fragile mechanism that the set of restyle hints posted by animation ticking
> as the result of refresh driver time advance, and the set of restyle hints
> posted for other things, are disjoint.  This is (as the comment there says)
> somewhat fragile.

I have not read the comment carefully.  I will check it. Thank you!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> These patch set actually works fine:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c18b8f8edd4f7a6565faab73476e377e67aafb05
> 
> But I'd like to know the purpose of the set_dirty_descendants() in comment
> 20.
> Bobby, could you please tell me about the purpose of the
> set_dirty_descendants() in traverse_children()?
> Maybe I am missing an important thing about the dirty bit.

That logic exists to be sure that we can actually find and update restyled elements during the post-traversal. Before the traversal, we propagate the bit up from any "restyle roots" (elements for which we've called Servo_NoteExplicitHints). However, it's possible for restyling at such a root to trigger more restyling in the siblings and subtree, and so we need to set the bit to make sure that we can use it in the post-traversal to find any elements that (a) have updated style or (b) have change hints to apply.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 25

a day ago
Thank you Bobby.  Now I think I understand why the set_dirty_descendants() call is necessary for animation-only restyle, animation-only restyle needs to update style context.
(Assignee)

Updated

a day ago
Attachment #8844720 - 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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

20 hours ago
The previous patch did traverse for non-animating element during animation-only traversal.
(Assignee)

Updated

18 hours ago
No longer blocks: 1346663
You need to log in before you can comment on or make changes to this bug.