Bug 79389 – Hitch (due to style recalc?) when starting CSS3 animation

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Rob Arnold]

Created attachment 128532 [details]
Testcase

See the attached testcase for an example. In it, we begin animating a div smoothly using JS to modify the transform property but when we switch to using the CSS3 animation, there is a noticeable visual hitch. We've observed this on the latest versions of Safari, Mobile Safari and Chrome (which most clearly shows the problem). The inspector seems to attribute this hitch to a style recalc.

On mobile safari, NUMBER_OF_LAYERS can be set lower (say, 256). You may need to tune this number depending on your machine to observe the problem.

[Simon Fraser (smfr)]

Pretty sure that touching animRule will cause full style recalc.

[Jonathan Kaldor]

Created attachment 129074 [details]
Revised testcase which does not modify the animation rule and removes the change in background color, still exhibits hitch

[Jonathan Kaldor]

Regarding modifying animRule, the hitch occurs whether or not lines 70-73 and 97-101 are commented out; the hitch seems to occur because of line 102, e.g. the setting of the animation property on container.  I just uploaded a newer version of the testcase which does not modify the animation rule and which also removes the change in background color.

[Jonathan Kaldor]

Created attachment 132179 [details]
Test case without overlap

In profiling, we noticed that there was a significant penalty to all the overlap testing.  We still see the hitch without the overlap, though, so this revised test case separates the child layers so the cost of style recalc is more apparent

[Rob Arnold]

Created attachment 132412 [details]
Patch

[Rob Arnold]

Comment on attachment 132412 [details]
Patch

This seems too simple to be true but I did not see any of the existing tests on the Mac port fail. I am undoing a change that was introduced when CSS3 animations were added to WebKit; it's not clear to me why the children also need to have a forced style recalc though I suspect it may be related to moving elements in and out of RenderLayers (animations are a trigger for compositing). Mostly I'm looking for a smoke check here.

[Simon Fraser (smfr)]

Interesting. Do you have a trac link for when that was added?

[Igor Trindade Oliveira]

In the code: 

// SyntheticStyleChange means that we need to go through the entire style change logic even though
// no style property has actually changed. It is used to restructure the tree when, for instance,
// RenderLayers are created or destroyed due to animation changes.

So would be interesting test sites using complex animations.

[Rob Arnold]

(In reply to comment #7)
> Interesting. Do you have a trac link for when that was added?

I misread the patch - it was https://bugs.webkit.org/show_bug.cgi?id=19938 which added the webkitAnimation* event and AnimationStyleChange (later renamed for WebGL).

(In reply to comment #8)
> In the code: 
> 
> // SyntheticStyleChange means that we need to go through the entire style change logic even though
> // no style property has actually changed. It is used to restructure the tree when, for instance,
> // RenderLayers are created or destroyed due to animation changes.
> 
> So would be interesting test sites using complex animations.

I was hoping that this would be covered by existing tests but I can start looking for some. Our internal prototypes appear to be working at least though I don't think they are cover enough of the edge cases.

[Rob Arnold]

I tried a couple of sites that use animations (some css3 filters, morphing power cubes) and wrote some test cases that (I hope) targeted the now non-recursive style recalculation (rediscovering bug 46041 in the process) but I was unable to find a test case where the behavior different from Chrome 17. Simon, do you have any ideas for test cases that might spot a bug? I noticed that you had some comments about this line of code in bug 19938 comment 5.

[Igor Trindade Oliveira]

Comment on attachment 132412 [details]
Patch

Lets put in the commit queue and hope for the best.

[WebKit Review Bot]

Comment on attachment 132412 [details]
Patch

Clearing flags on attachment: 132412

Committed r112155: <http://trac.webkit.org/changeset/112155>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Loïc Yhuel]

Created attachment 137588 [details]
regression : blinking text animation

This animation no longer works after this commit.

In this case, when the text color should be changed to white :
 - styleChangeType() is SyntheticStyleChange, the animated style (color: white) is set on the Element in setRenderStyle(newStyle)
 - currentStyle and newStyle are the same (color: black), so ch is NoChange
 - After r112155, change is no more set to Force, so Text::recalcTextStyle is called with NoChange as argument, the style of the text is not changed, so the text color is still black.

[Alexey Proskuryakov]

Could you please file a new bug about the regression? It's helpful to mention the problem in a bug that introduced it, but actual discussion should happen in a new one.

[Loïc Yhuel]

(In reply to comment #15)
> Could you please file a new bug about the regression? It's helpful to mention the problem in a bug that introduced it, but actual discussion should happen in a new one.

https://bugs.webkit.org/show_bug.cgi?id=84194

[Alexey Proskuryakov]

This also caused bug 87146.

[Dean Jackson]

We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert.

[Dean Jackson]

Reverted in http://trac.webkit.org/changeset/118553

[Elliott Sprehn]

(In reply to comment #18)
> We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert.

Can you explain what was broken? It's not clear in the bug.

[Rob Arnold]

(In reply to comment #20)
> (In reply to comment #18)
> > We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert.
> 
> Can you explain what was broken? It's not clear in the bug.

In the blinking text example (bug 84194), the text would not blink. My guess was that any inherited values would not be animated as a side effect of my change. The use case I'm interested in is animating transforms which cannot (I think) be inherited.

[Brent Fulgham]

This seems to work properly (at least I don't see any meaningful animation hitches). Please REOPEN if you believe there is still an issue here.

[Radar WebKit Bug Importer]

<rdar://problem/96918334>