SVG CSS animation not working through img tag

VERIFIED FIXED in Firefox 51

Status

()

Core
SVG
P1
normal
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: acterhd, Assigned: u459114)

Tracking

(Blocks: 1 bug, {dev-doc-complete, regression})

unspecified
mozilla51
dev-doc-complete, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: tlt-done, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 4 obsolete attachments)

293.07 KB, image/svg+xml
Details
129 bytes, text/html
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
58 bytes, text/x-review-board-request
birtles
: review+
Details | Review
58 bytes, text/x-review-board-request
dholbert
: review+
Details | Review
58 bytes, text/x-review-board-request
Details | Review
(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Component: Untriaged → SVG
Product: Firefox → Core
(Reporter)

Comment 1

3 years ago
Fixed test case. I working on.
Created attachment 8643533 [details]
reporter's testcase without all the javascript creation
(Reporter)

Comment 4

3 years ago
Paste into "img" tag.

Comment 5

3 years ago
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.
Keywords: regression
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.
(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
The flickering in the test case might be bug 1223658.
Depends on: 763784
Duplicate of this bug: 1276108

Updated

2 years ago
Blocks: 1262352
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)

Updated

2 years ago
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)
(Assignee)

Comment 21

2 years ago
bug 1262352 comment 6
(Assignee)

Comment 22

2 years ago
No luck, simply rollback patches in bug 1002632 and bug 763784 does not work. I will look deeper into this bug.
(Assignee)

Updated

2 years ago
Flags: needinfo?(acterhd)
See Also: → bug 908634
(Assignee)

Updated

2 years ago
Flags: needinfo?(acterhd)
(Assignee)

Comment 23

2 years ago
Created attachment 8783957 [details] [diff] [review]
WIP

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/
(Assignee)

Comment 24

2 years ago
and
(Bug 1121478) https://jsfiddle.net/evg46ns2/

The root cause of these three bug is the same
(Assignee)

Updated

2 years ago
See Also: → bug 1121478
(Assignee)

Updated

2 years ago
See Also: → bug 1184530
(Assignee)

Comment 27

2 years ago
This bug is different:
Bug 1118710 - http://jsfiddle.net/wojtekjodel/kq3jpksj/14/
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

2 years ago
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.
(Assignee)

Updated

2 years ago
See Also: bug 1184530
(Assignee)

Updated

2 years ago
Attachment #8783957 - Attachment is obsolete: true

Comment 34

2 years ago
mozreview-review
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 35

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8784319 - Attachment is obsolete: true
Attachment #8784319 - Flags: review?(dholbert)
(Assignee)

Updated

2 years ago
Attachment #8784895 - Flags: review?(dholbert)
Attachment #8784896 - Flags: review?(dholbert)
Attachment #8784897 - Flags: review?(dholbert)
(Assignee)

Comment 41

2 years ago
mozreview-review
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
(Assignee)

Comment 42

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 46

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 52

2 years ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 56

2 years ago
mozreview-review
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 57

2 years ago
mozreview-review
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 58

2 years ago
mozreview-review
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 61

2 years ago
mozreview-review
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 62

2 years ago
mozreview-review
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)
(Assignee)

Comment 65

2 years ago
We can find another place to trigger animation:
https://reviewboard.mozilla.org/r/74212/diff/2/

Let me ask help from web animation engineer.
(Assignee)

Comment 66

2 years ago
mozreview-review-reply
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?
(Assignee)

Updated

2 years ago
Attachment #8784895 - Flags: review?(bbirtles)
(Assignee)

Comment 67

2 years ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8784318 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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.
(Assignee)

Comment 78

2 years ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8784895 - Attachment is obsolete: true
Attachment #8784895 - Flags: review?(bbirtles)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 84

2 years ago
(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 86

2 years ago
mozreview-review
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+
(Assignee)

Comment 87

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 92

2 years ago
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

Updated

2 years ago
Priority: -- → P1

Comment 93

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/803ef95bed16
https://hg.mozilla.org/mozilla-central/rev/982c67b9fe74
https://hg.mozilla.org/mozilla-central/rev/aad83800847a
https://hg.mozilla.org/mozilla-central/rev/cd2ae4eeae40
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

2 years ago
Duplicate of this bug: 908634
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1121478
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

Updated

2 years ago
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+
Added a note in:
https://developer.mozilla.org/en-US/Firefox/Releases/51#SVG
Keywords: dev-doc-needed → dev-doc-complete
Duplicate of this bug: 1311317
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.