| Summary: | Hitch (due to style recalc?) when starting CSS3 animation | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Rob Arnold <robarnold> |
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED CONFIGURATION CHANGED | ||
| Severity: | Normal | CC: | ap, bfulgham, browser-bugs, crizweather21, dbates, dino, esprehn, igor.oliveira, jamesr, jmkaldor, loic.yhuel, nduca, rniwa, shanestephens, simon.fraser, tony, webkit-bug-importer, webkit.review.bot |
| Priority: | P2 | Keywords: | FacebookBug, InRadar |
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | |||
Pretty sure that touching animRule will cause full style recalc. Created attachment 129074 [details]
Revised testcase which does not modify the animation rule and removes the change in background color, still exhibits hitch
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. 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
Created attachment 132412 [details]
Patch
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.
Interesting. Do you have a trac link for when that was added? 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. (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. 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. Comment on attachment 132412 [details]
Patch
Lets put in the commit queue and hope for the best.
Comment on attachment 132412 [details] Patch Clearing flags on attachment: 132412 Committed r112155: <http://trac.webkit.org/changeset/112155> All reviewed patches have been landed. Closing bug. 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. 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. (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 This also caused bug 87146. We've received multiple reports internally and externally that this is breaking things. Sorry, I'm going to revert. Reverted in http://trac.webkit.org/changeset/118553 (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 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. 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. |
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.