Closed Bug 1190881 Opened 9 years ago Closed 8 years ago

SVG CSS animation not working through img tag

Categories

(Core :: SVG, defect, P1)

Product:
Core
Component:
SVG
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- verified

People

(Reporter: acterhd, Assigned: u459114)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, regression, Whiteboard: tlt-done)

Attachments

(6 files, 4 obsolete files)

clock.svg
293.07 KB, image/svg+xml
Details
reporter's testcase without all the javascript creation
129 bytes, text/html
Details
Bug 1190881 - Part 3. mochitest for svg css animation.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1190881 - Part 4. Use infallible PresShell getter in PaintRoot, and remove unnecessary null-check on it result.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1190881 - Part 1. Create and pass a navigation timing object to the wrapped SVG document.
58 bytes, text/x-review-board-request
Details
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2467.2 Safari/537.36 Steps to reproduce: I created SVG with CSS animation and pasted to IMG tag. Result: animation not working. In chrome <animate> deprecated and uses CSS animation, even if IMG tag. I'm use chrome browser, but found bug in Firefox. Must working right image https://acterhd.github.io/apng-decoder/ Actual results: Not working SVG animation Expected results: Animation should working
Component: Untriaged → SVG
Product: Firefox → Core
Fixed test case. I working on.
Attached image clock.svg —
Paste into "img" tag.
Another testcase: http://xn--dahlstrm-t4a.net/svg/css/animations/img-animation.html CSS animations aren't working in background images either, see http://xn--dahlstrm-t4a.net/svg/css/animations/background-animation.html.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Is this a regression?
Yes it is. This works in Firefox 16. I'll try to find a smaller regression range.
We now rely on SVGDocumentWrapper::TickRefreshDriver being called under: SVGDocumentWrapper::TickRefreshDriver VectorImage::RequestRefresh nsRefreshDriver::Tick That doesn't happen in this case because in nsRefreshDriver::Tick, mRequests does not include the VectorImage. We should add the VectorImage when nsImageLoadingContent::FrameCreated is called (for the embedding <img>) and calls nsLayoutUtils::RegisterImageRequestIfAnimated. We don't though because SVGDocumentWrapper::IsAnimated doesn't return true when called under: SVGDocumentWrapper::IsAnimated VectorImage::GetAnimated nsLayoutUtils::RegisterImageRequestIfAnimated nsImageLoadingContent::FrameCreated because it only checks for SMIL animations.
Flags: needinfo?(dholbert)
I guess that makes this a duplicate of bug 763784.
Blocks: 1242256
(In reply to Jonathan Watt [:jwatt] from comment #10) > I guess that makes this a duplicate of bug 763784. Actually that's only part of the puzzle. It looks like there are at least a couple other issues that I'm working on.
Assignee: nobody → jwatt
No longer blocks: 1242256
The flickering in the test case might be bug 1223658.
Depends on: 763784
Jonathan, any update on this? Or more to the point, are you likely to be able to work on this in the near future? I'd like to get this fixed since it's a common complaint from authors and other browser vendors (who want to recommend people use CSS animations in SVG but can't because it doesn't work in Firefox; specifically Opera and Google have brought this up) and it's now getting in our way too in bug 759252. If you're not available let me know and I'll find someone else to look into it.
Flags: needinfo?(jwatt)
I won't be able to look further at this in the near future. If you can find someone else to look at it that would be awesome, Brian.
Flags: needinfo?(jwatt)
Assignee: jwatt → nobody
Can you at least give us a hint about what the couple of issues were that you mentioned in commment 11?
Flags: needinfo?(jwatt)
Unfortunately not. :/ I usually keep notes on this sort of thing somewhere, but I can't find any for this bug.
Flags: needinfo?(jwatt)
Astley, is this something that your team can help?
Flags: needinfo?(aschen)
Ting-Yu, yes. For SVG relevant bugs tracked by svg-enhance (bug 1262352), we will try to fix them one by one based on priority and present resource allocation. Thanks.
Flags: needinfo?(aschen)
Blocks: 759252
Flags: needinfo?(aschen)
Hi CJ, let's figure out the root cause here and explore any knotty issues mentioned in comment 11. Thanks.
Assignee: nobody → cku
Status: NEW → ASSIGNED
Flags: needinfo?(aschen)
bug 1262352 comment 6
No luck, simply rollback patches in bug 1002632 and bug 763784 does not work. I will look deeper into this bug.
Flags: needinfo?(acterhd)
See Also: → 908634
Flags: needinfo?(acterhd)
Attached patch WIP (obsolete) — — Splinter Review
A prototype to point out where is the problem. You may apply this patch and browse this test cases (this bug) http://xn--dahlstrm-t4a.net/svg/css/animations/img-animation.html (bug 908634) http://jsfiddle.net/zBudu/
and (Bug 1121478) https://jsfiddle.net/evg46ns2/ The root cause of these three bug is the same
See Also: → 1121478
See Also: → 1184530
This bug is different: Bug 1118710 - http://jsfiddle.net/wojtekjodel/kq3jpksj/14/
I only created one test case here. This one is to guarantee css-animation works correctly for SVG-as-image. We still need a test case for SVG-as-backgound-image, will create one in bug 912449.
See Also: 1184530 →
Attachment #8783957 - Attachment is obsolete: true
Comment on attachment 8784318 [details] Bug 1190881 - Part 1. Create and pass a navigation timing object to the wrapped SVG document. https://reviewboard.mozilla.org/r/73820/#review71754 ::: image/SVGDocumentWrapper.cpp:355 (Diff revision 2) > > NS_ENSURE_TRUE(viewer, NS_ERROR_UNEXPECTED); > > + RefPtr<nsDOMNavigationTiming> timing = new nsDOMNavigationTiming(); > + timing->NotifyNavigationStart(); > + viewer->SetNavigationTiming(timing); Can you explain why this |timing| object is needed here? It's not clear to me where it ends up getting used / needed. (I can imagine that the PendingAnimationTracker in the next patch might depend on it somehow, but I don't immediately see where that dependency is. Both of these objects -- PendingAnimationTracker and nsDOMNavigationTiming -- are new to me.)
Comment on attachment 8784379 [details] Bug 1190881 - Part 3. mochitest for svg css animation. https://reviewboard.mozilla.org/r/73840/#review71764 The mochitest changes look good! Thanks for updating it to cover this case. r=me, two nits below. ::: image/test/mochitest/mochitest.ini:83 (Diff revision 1) > transparent.png > over.png > 6M-pixels.png > 12M-pixels-1.png > 12M-pixels-2.png > + lime-css-anim-100x100.svg This list is (mostly) in alphabetical order -- please insert this new file in the correct sorted location. ::: image/test/mochitest/test_animSVGImage.html:73 (Diff revision 1) > + > + if (++gSVGCurrentImage > gSVGImages.length) { > - cleanUpAndFinish(); > + cleanUpAndFinish(); > + } else { > + setTimeout(myPoll, 1); > + gImg.setAttribute("src", gSVGImages[gSVGCurrentImage]); These two lines are duplicated here and in main() -- they might want to be refactored out into a helper-function, e.g. "loadNextImageAndPoll()", but it's fine like this too.
Attachment #8784379 - Flags: review?(dholbert) → review+
Attachment #8784319 - Attachment is obsolete: true
Attachment #8784319 - Flags: review?(dholbert)
Attachment #8784895 - Flags: review?(dholbert)
Attachment #8784896 - Flags: review?(dholbert)
Attachment #8784897 - Flags: review?(dholbert)
Comment on attachment 8784897 [details] Bug 1190881 - Part 4. Use infallible PresShell getter in PaintRoot, and remove unnecessary null-check on it result. https://reviewboard.mozilla.org/r/74214/#review72064 ::: layout/base/nsDisplayList.cpp (Diff revision 1) > RefPtr<ContainerLayer> root = layerBuilder-> > BuildContainerLayerFor(aBuilder, layerManager, frame, nullptr, this, > containerParameters, nullptr); > > - nsIDocument* document = nullptr; > + nsIDocument* document = presShell->GetDocument(); > - if (presShell) { If presShell == nullptr is possible, we should already crash at http://searchfox.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1801 many times. So this check is useless
Comment on attachment 8784318 [details] Bug 1190881 - Part 1. Create and pass a navigation timing object to the wrapped SVG document. https://reviewboard.mozilla.org/r/73820/#review72066 ::: image/SVGDocumentWrapper.cpp:365 (Diff revision 3) > + // automatically. Since there is no DocShell for this wrapped SVG document, > + // we must set it up manually. > + RefPtr<nsDOMNavigationTiming> timing = new nsDOMNavigationTiming(); > + timing->NotifyNavigationStart(); > + viewer->SetNavigationTiming(timing); > + Where DocShell pass navigation timing object into viewer: http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9360
Comment on attachment 8784318 [details] Bug 1190881 - Part 1. Create and pass a navigation timing object to the wrapped SVG document. https://reviewboard.mozilla.org/r/73820/#review72230 Thanks for the nice documentation here. r=me, just a few grammar/clarification nits: ::: image/SVGDocumentWrapper.cpp:356 (Diff revision 3) > NS_ENSURE_TRUE(viewer, NS_ERROR_UNEXPECTED); > > + // Create a navigation time object and pass it to the SVG document through > + // the viewer. > + // The timeline(DocumentTimeline) of this SVG document needs this navigation > + // timing object for time computation, such as calculate current time stamp s/such as calculate/such as to calculate/ (or "e.g. to calculate" if you want to avoid rewrapping this line :)) ::: image/SVGDocumentWrapper.cpp:357 (Diff revision 3) > > + // Create a navigation time object and pass it to the SVG document through > + // the viewer. > + // The timeline(DocumentTimeline) of this SVG document needs this navigation > + // timing object for time computation, such as calculate current time stamp > + // base on the start time of navigation time object. Three nits: (1) s/such as calculate/such as to calculate/ (2) s/base on/based on/ (3) Please mention animations somewhere here (since that's the reason we care about this DOM object -- not actually for any DOM timestamp apis, since scripting is disabled). e.g. maybe add "(for animations)" at the end of this sentence. ::: image/SVGDocumentWrapper.cpp:359 (Diff revision 3) > + // the viewer. > + // The timeline(DocumentTimeline) of this SVG document needs this navigation > + // timing object for time computation, such as calculate current time stamp > + // base on the start time of navigation time object. > + // > + // For a root document, DocShell will do these sort of things s/will/would/ (to make it clearer that "a root document" is *not* what we're dealing with here.)
Attachment #8784318 - Flags: review?(dholbert) → review+
Comment on attachment 8784896 [details] Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs. https://reviewboard.mozilla.org/r/74212/#review72446 ::: layout/base/nsDisplayList.cpp:1684 (Diff revision 3) > PendingAnimationTracker* tracker = aDocument->GetPendingAnimationTracker(); > if (tracker) { > nsIPresShell* shell = aDocument->GetShell(); > // If paint-suppression is in effect then we haven't finished painting > - // this document yet so we shouldn't start animations > - if (!shell || !shell->IsPaintingSuppressed()) { > + // this document yet so we shouldn't start animations. SVG-as-an-image > + // docuemnt is an excption since it's painted indirectly. document is not spelled correctly here. ::: layout/base/nsDisplayList.cpp:1940 (Diff revision 3) > aBuilder, flags); > aBuilder->SetIsCompositingCheap(temp); > layerBuilder->DidEndTransaction(); > > - if (document && widgetTransaction) { > + // Trigger pending animation for SVG-as-an-image doc, since it may also > + // contian CSS animation. contain is not spelled correctly here.
Comment on attachment 8784895 [details] Bug 1190881 - Part 2. Assert the document of a DocumentTimeline contains a navigation timing. https://reviewboard.mozilla.org/r/74210/#review74190 ::: dom/animation/DocumentTimeline.cpp:95 (Diff revision 2) > > // If we don't have a refresh driver and we've never had one use the > // timeline's zero time. > if (result.IsNull()) { > RefPtr<nsDOMNavigationTiming> timing = mDocument->GetNavigationTiming(); > + MOZ_ASSERT(timing); // The computing result of this function is mostly A few things, which apply to each of this patch's assertions: (1) This should be a non-fatal assertion, NS_ASSERTION. Assertions should only be fatal if we're about to crash anyway when the assertion fails [and hence the assertion makes us abort more helpfully in debug builds], or if the assertion indicates something is really horrendously wrong. In this case, I don't think a fatal assertion is merited; we can proceed just fine (though without animations) if the assertion fails. (Superficially, it seems like it would good to always use stricter assertions & catch bugs more aggressively. But unnecessarily fatal assertions can cause more trouble than they're worth. e.g. dbaron used to run debug builds as his main browser, but I think he recently stopped because he was aborting on too many unnecessarily-fatal-assertions for stuff like this -- the assertion-failures all probably indicated bugs, but it probably wasn't worth forcing his browsing session to be aborted every time he tripped over one. Fuzzers & automated tests can still trigger & report nonfatal assertion-failures, too.) (2) Please include a helpful message with the assertion. (The compiler will actually force you to do so, I think, when you convert it to nonfatal NS_ASSERTION -- that version requires a message.) The code-comment you've got there is nice, but it'd be even nicer if we had a form of that comment as the assertion-message, so that something more informative/helpful than e.g. "ASSERTION FAILED: timing" will appear in the terminal when this fails. (3) Someone who's actually touched this DocumentTimeline code (and the "timing" null-checks around here in particular) should review this patch; they'll be more likely to know whether or not we're justified in asserting that this "timing" object must necessarily be non-null here. (And they might have thoughts on whether we should keep the existing null-checks around, if these new assertions are really justified.) Could you look at hg blame for these lines / file to find a reviewer?
Attachment #8784895 - Flags: review?(dholbert)
Attachment #8784896 - Flags: review?(dholbert) → review?(bbirtles)
Comment on attachment 8784896 [details] Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs. https://reviewboard.mozilla.org/r/74212/#review74194 I don't really know the context around / background behind either of the bits of logic that you're changing here. It looks like birtles has "hg blame" for both conditions that you're modifying, so I'm hoping he might understand the change better & be a better reviewer -- transferring review to him. I've included a few nits, though: ::: layout/base/nsDisplayList.cpp:1684 (Diff revision 4) > PendingAnimationTracker* tracker = aDocument->GetPendingAnimationTracker(); > if (tracker) { > nsIPresShell* shell = aDocument->GetShell(); > // If paint-suppression is in effect then we haven't finished painting > - // this document yet so we shouldn't start animations > - if (!shell || !shell->IsPaintingSuppressed()) { > + // this document yet so we shouldn't start animations. SVG-as-an-image > + // document is an excption since it's painted indirectly. typo: s/excption/exception/ (missing an "e") ::: layout/base/nsDisplayList.cpp:1686 (Diff revision 4) > nsIPresShell* shell = aDocument->GetShell(); > // If paint-suppression is in effect then we haven't finished painting > - // this document yet so we shouldn't start animations > - if (!shell || !shell->IsPaintingSuppressed()) { > + // this document yet so we shouldn't start animations. SVG-as-an-image > + // document is an excption since it's painted indirectly. > + if (!shell || (!shell->IsPaintingSuppressed() || > + aDocument->IsBeingUsedAsImage())) { Drop the extra layers of parens here -- they're not useful, and they make the logic seem a bit more complex than it really is. Logically, (a || (b || c)) ...is equivalent to just... (a || b || c) ::: layout/base/nsDisplayList.cpp:1941 (Diff revision 4) > aBuilder->SetIsCompositingCheap(temp); > layerBuilder->DidEndTransaction(); > > - if (document && widgetTransaction) { > + // Trigger pending animation for SVG-as-an-image doc, since it may also > + // contain CSS animation. > + if (document && (widgetTransaction || document->IsBeingUsedAsImage())) { (As noted above, I don't really understand what this check is currently doing, so I'm not comfortable r+'ing changes to it at this point. BUT, I did notice one thing when doing a little research: the last modification to this line (from birtles) was adding this "widgetTransaction" check, and the commit message[1] mentioned that it was trying to avoid nested PaintFrame calls during -moz-element() resolution. So: now that you're letting IsBeingUsedAsImage() bypass the "widgetTransaction" check that birtles added, I wonder if that might make us end up being able to produce the nested paints that birtles was trying to avoid, if we're inside of an SVG-as-an-image doc? (not sure; hopefully birtles knows)) [1] https://hg.mozilla.org/mozilla-central/rev/64f3413693b9
Attachment #8784896 - Flags: review?(dholbert)
Attachment #8784896 - Flags: review?(dholbert)
Comment on attachment 8784897 [details] Bug 1190881 - Part 4. Use infallible PresShell getter in PaintRoot, and remove unnecessary null-check on it result. https://reviewboard.mozilla.org/r/74214/#review74230 Please reword the commit message: > Part 5. Correct logic in PaintRoot. Right now this sounds like you're *changing the control-flow* -- but you're not. You're just removing an unnecessary null-check (removing dead code, basically). So, please rephrase as: "Part 5. Use infallible PresShell getter in PaintRoot, and remove unnecessary null-check on its result" (The first half might not make sense until you see the one "issue" I filed below). ::: layout/base/nsDisplayList.cpp:1781 (Diff revision 4) > layerBuilder->DidBeginRetainedLayerTransaction(layerManager); > } > > nsIFrame* frame = aBuilder->RootReferenceFrame(); > nsPresContext* presContext = frame->PresContext(); > nsIPresShell* presShell = presContext->GetPresShell(); Please change this to call "PresShell()" instead of "GetPresShell()". (The two accessors have the same behavior, but the one without "get" implies by its name that it's guaranteed to return non-null -- and it asserts that it's holding up to that promise, too, so we don't have to bother asserting here.) That makes the existing code's implicit assumption (that presShell is non-null) a bit clearer & more understandable. And it makes your new reliance on that assumption more justifiable as well.
Attachment #8784897 - Flags: review?(dholbert) → review+
Also, one more nit on Part 3's commit message that I just noticed: > Bug 1190881 - Part 3. Tigger pending animation for SVG-as-image docs. You're missing an "r" in the first word there -- should be "Trigger".
I'm going to pass part 3 onto Matt. In bug 1123196 comment 16 / attachment 8556855 [details] [diff] [review] I actually had the document->IsBeingUsedAsImage() check but Matt suggested in bug 1123196 comment 21 that we shouldn't need it because "Rendering an image document should still be nested within the chrome document, and will have a widget transaction there." So I guess I'd like to know if something changed that means that's no longer true. In any case, Matt understands this much better than I.
Attachment #8784896 - Flags: review?(bbirtles) → review?(matt.woodrow)
Comment on attachment 8784896 [details] Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs. https://reviewboard.mozilla.org/r/74212/#review74294 ::: layout/base/nsDisplayList.cpp:1685 (Diff revision 4) > if (tracker) { > nsIPresShell* shell = aDocument->GetShell(); > // If paint-suppression is in effect then we haven't finished painting > - // this document yet so we shouldn't start animations > - if (!shell || !shell->IsPaintingSuppressed()) { > + // this document yet so we shouldn't start animations. SVG-as-an-image > + // document is an excption since it's painted indirectly. > + if (!shell || (!shell->IsPaintingSuppressed() || exception is not spelled correctly here.
Comment on attachment 8784896 [details] Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs. https://reviewboard.mozilla.org/r/74212/#review74304 I don't know this code well enough, sorry.
Attachment #8784896 - Flags: review?(matt.woodrow)
Matt, did you see comment 60? You suggested we don't need the "document->IsBeingUsedAsImage()" in the first place and now this patch adds it back. Is that ok?
Flags: needinfo?(matt.woodrow)
(In reply to Brian Birtles (:birtles) from comment #63) > Matt, did you see comment 60? You suggested we don't need the > "document->IsBeingUsedAsImage()" in the first place and now this patch adds > it back. Is that ok? Hmm, good point, I did too. I think it's still incorrect, for the same reason. When painting the top level document we'll end up taking that path twice (once for the top-level, and once for the SVG document) and run into the problems we had in bug 1123196.
Flags: needinfo?(matt.woodrow)
We can find another place to trigger animation: https://reviewboard.mozilla.org/r/74212/diff/2/ Let me ask help from web animation engineer.
Comment on attachment 8784895 [details] Bug 1190881 - Part 2. Assert the document of a DocumentTimeline contains a navigation timing. https://reviewboard.mozilla.org/r/74210/#review74190 > A few things, which apply to each of this patch's assertions: > (1) This should be a non-fatal assertion, NS_ASSERTION. Assertions should only be fatal if we're about to crash anyway when the assertion fails [and hence the assertion makes us abort more helpfully in debug builds], or if the assertion indicates something is really horrendously wrong. In this case, I don't think a fatal assertion is merited; we can proceed just fine (though without animations) if the assertion fails. > > (Superficially, it seems like it would good to always use stricter assertions & catch bugs more aggressively. But unnecessarily fatal assertions can cause more trouble than they're worth. e.g. dbaron used to run debug builds as his main browser, but I think he recently stopped because he was aborting on too many unnecessarily-fatal-assertions for stuff like this -- the assertion-failures all probably indicated bugs, but it probably wasn't worth forcing his browsing session to be aborted every time he tripped over one. Fuzzers & automated tests can still trigger & report nonfatal assertion-failures, too.) > > (2) Please include a helpful message with the assertion. (The compiler will actually force you to do so, I think, when you convert it to nonfatal NS_ASSERTION -- that version requires a message.) The code-comment you've got there is nice, but it'd be even nicer if we had a form of that comment as the assertion-message, so that something more informative/helpful than e.g. "ASSERTION FAILED: timing" will appear in the terminal when this fails. > > (3) Someone who's actually touched this DocumentTimeline code (and the "timing" null-checks around here in particular) should review this patch; they'll be more likely to know whether or not we're justified in asserting that this "timing" object must necessarily be non-null here. (And they might have thoughts on whether we should keep the existing null-checks around, if these new assertions are really justified.) Could you look at hg blame for these lines / file to find a reviewer? I think　these functions should not be used if mDocument->GetNavigationTiming() return null. birtles, may I have your opinion here?
Attachment #8784895 - Flags: review?(bbirtles)
Hi Brain, In Part 3, I tried to find a place to trigger pending animation. Two places that I found. One is from nsDisplayList::PaintRoot[1], which Matt thought it's not correct in comment 64. Anther place is from VectorImage::RequestRefresh[1]. We trigger SMIL or CSS animation of this wrapped SVG document from here. How do you think? [1]https://reviewboard.mozilla.org/r/74212/diff/4#index_header [2]https://reviewboard.mozilla.org/r/74212/diff/2/
Flags: needinfo?(bbirtles)
Attachment #8784318 - Attachment is obsolete: true
(In reply to C.J. Ku[:cjku](UTC+8) from comment #66) > I think　these functions should not be used if > mDocument->GetNavigationTiming() return null. birtles, may I have your > opinion here? I'm pretty sure there are some odd cases with using document.open or document.write where we can end up with a null timing object. I can dig up the bug(s) where we've hit this if needed, but it does happen so I don't think we want part 2 in this patch series.
(In reply to Brian Birtles (:birtles) from comment #77) > (In reply to C.J. Ku[:cjku](UTC+8) from comment #66) > > I think　these functions should not be used if > > mDocument->GetNavigationTiming() return null. birtles, may I have your > > opinion here? > > I'm pretty sure there are some odd cases with using document.open or > document.write where we can end up with a null timing object. I can dig up > the bug(s) where we've hit this if needed, but it does happen so I don't > think we want part 2 in this patch series. OK, then I think we should probably do this in another bug, so that we can backout part 2 independent if these assertion hit.
Attachment #8784895 - Attachment is obsolete: true
Attachment #8784895 - Flags: review?(bbirtles)
Blocks: 1300481
I'm a bit confused. I assume that if we don't have part 2 the animations don't start, but Matt seems to be saying we shouldn't need it. Do we not get a widget transaction in nsDisplayList::PaintRoot for SVG-as-image? I'm a bit concerned that with the change to VectorImage::RequestRefresh we might end up starting animations one tick too late. But maybe that's ok.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #83) > I'm a bit confused. I assume that if we don't have part 2 the animations > don't start, but Matt seems to be saying we shouldn't need it. > > Do we not get a widget transaction in nsDisplayList::PaintRoot for > SVG-as-image? We do, but: While painting SVG-as-image, we always use basic layer manager create at [2], that means widgetTransaction is always false. Look back to the if condition at [1], if widgetTransaction is false, TriggerPendingAnimationsOnNextTick will not be called. [1] http://searchfox.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1937 [2] http://searchfox.org/mozilla-central/source/layout/base/nsDisplayList.cpp#1748 > I'm a bit concerned that with the change to VectorImage::RequestRefresh we > might end up starting animations one tick too late. But maybe that's ok. Even we start animations in nsDisplayList::PaintRoot, we still end up one tick late, isn't it?
(In reply to C.J. Ku[:cjku](UTC+8) from comment #84) > (In reply to Brian Birtles (:birtles) from comment #83) > > I'm a bit concerned that with the change to VectorImage::RequestRefresh we > > might end up starting animations one tick too late. But maybe that's ok. > Even we start animations in nsDisplayList::PaintRoot, we still end up one > tick late, isn't it? Oh, you're right, this is probably the opposite. It looks like we set the start time to the current refresh driver time so we miss the optimization that delays the start until the first paint has completed. However, since it is painted indirectly, I guess that optimization doesn't work anyway.
Comment on attachment 8784896 [details] Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs. https://reviewboard.mozilla.org/r/74212/#review74792 ::: image/VectorImage.cpp:528 (Diff revision 7) > { > if (HadRecentRefresh(aTime)) { > return; > } > > + // Call TriggerPendingAnimationsOnNextTick here so that all pending animtions s/animtions/animations/ (In fact, I'm not sure we even need this comment)
Attachment #8784896 - Flags: review?(bbirtles) → review+
Thanks, all r+ granted(Part 1. already get r+ from dholbert, but miss-carry by accident while uploading patch to reviewboard) Will land code after manually and try test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/803ef95bed16d06f67681902e8a8a7fc659b202e Bug 1190881 - Part 1. Create and pass a navigation timing object to the wrapped SVG document. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/982c67b9fe74fc1c0b7de6bf203acb98edfc5b57 Bug 1190881 - Part 2. Trigger pending animation for SVG-as-image docs. r=birtles https://hg.mozilla.org/integration/mozilla-inbound/rev/aad83800847aecd82266dc44daa7853f247c7256 Bug 1190881 - Part 3. mochitest for svg css animation. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/cd2ae4eeae40b4d4dbc19cbe66b7d92cb68ff497 Bug 1190881 - Part 4. Use infallible PresShell getter in PaintRoot, and remove unnecessary null-check on it result. r=dholbert
Priority: -- → P1
Release Note Request (optional, but appreciated) [Why is this notable]: A known carryover regression that is about malfunction of SVG CSS animation. [Suggested wording]: As bug summary. [Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Keywords: dev-doc-needed
Whiteboard: tlt-done
Per talk with RM Gerry, this fix is more related to developers or authors so that it's supposed to be listed in MDN release note for developers.
relnote-firefox: ? → ---
Flags: qe-verify+
I reproduced this bug using Fx 42.0a1, build ID: 20150804030204, on Windows 10 x64. I can confirm this issue is fixed, I verified using Fx 51.0a2, build ID: 20161108004019 on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS. Cheers!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
No longer blocks: 759252
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: