Closed Bug 1067375 Opened 5 years ago Closed 5 years ago

animateTransform in svg-as-an-image doesn't get painted, if animated element is covered by something see-through

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

No description provided.
Attachment #8489366 - Attachment is patch: false
Attachment #8489366 - Attachment mime type: text/plain → image/svg+xml
Attached file broken image container
Attachment #8489377 - Attachment mime type: text/plain → text/html
Attached file working container
Attachment #8489417 - Attachment mime type: text/plain → text/html
Version: unspecified → Trunk
Observations from Daniel.

VectorImage::InvalidateObserversOnNextRefreshDriverTick()
which gets triggered by SVGRootRenderingObserver's DoUpdate() method
SVGRootRenderingObserver is a nsSVGRenderingObserver, which is supposed to catch all rendering changes in the image-doc
Right now, in the working version, we're only sending rendering changes to the VectorImage via this chunk at the end of nsIFrame::FinishAndStoreOverflow():
> 7399   if (anyOverflowChanged) {
> 7400     nsSVGEffects::InvalidateDirectRenderingObservers(this);
> 7401   }

I think that's checking whether the top-level SVG document's overflow area changed. And in the broken testcase, the overflow area is already larger than the spinning rect, so it doesn't change. So we don't invalidate.

So, I think we might just not be invalidating rendering observers for transform changes (unless they impact the overflow area of the thing being observed).
Summary: Image animation fails if animated elements are covered by something see-through → animateTransform in svg-as-an-image doesn't get painted, if animated element is covered by something see-through
nsSVGPathGeometryFrame::AttributeChanged() (called by nsSVGElement::DidAnimateTransformList) has a comment that seems relevant:

> 134   // We don't invalidate for transform changes (the layers code does that).
> 135   // Also note that SVGTransformableElement::GetAttributeChangeHint will
> 136   // return nsChangeHint_UpdateOverflow for "transform" attribute changes
> 137   // and cause DoApplyRenderingChangeToTree to make the SchedulePaint call.
http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGPathGeometryFrame.cpp?rev=5db436c928aa#129
(I'll bet the layers code, referenced at line 134 there, doesn't trigger a call to nsSVGEffects::InvalidateRenderingObservers...)
I'm willing to bet this was a regression from bug 839865, "Stop calling nsSVGUtils::InvalidateBounds for SVG transform changes, and use DLBI instead."

That patch removed a special-case (transform-attribute-only) call to InvalidateBounds() from nsSVGPathGeometryFrame::AttributeChanged().

And InvalidateBounds involved a call to InvalidateRenderingObservers, which I believe would save us here.

I'm not sure if rendering observers were considered there (as opposed to just detecting when painting is needed) -- "observer" isn't mentioned in any of the comments on that bug report, so I suspect not.
Blocks: 839865
Keywords: regression
...and this is likely a dupe of bug 975757, which is about animateTransform in a <use>'d element not correctly repainting. (probably due to the same lack-of-a-InvalidateRenderingObservers-call)
Attached patch observers.txt (obsolete) — Splinter Review
Let's see whether jwatt thinks we're on the right lines...

I can knock up a reftest easily enough I think.
Assignee: nobody → longsonr
Attachment #8489594 - Flags: review?(jwatt)
One thought on the patch: InvalidateRenderingObservers() starts with an assertion that the passed-in frame is the first thing in its continuation chain.

I think your patch will violate that, since the code it touches gets run for every frame in a continuation chain:
> 183   for ( ; aFrame; aFrame = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(aFrame)) {
http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp#183

We probably want to wrap the added call in "if (!aFrame->GetPreviousContinuation())" or something, to avoid tripping that.
See Also: → 975757
Attached patch observers.txtSplinter Review
Not totally sure this is right or whether just to assert because continuation frames should not get here.
Attachment #8489594 - Attachment is obsolete: true
Attachment #8489594 - Flags: review?(jwatt)
Attachment #8489632 - Flags: review?(jwatt)
Why do you think they shouldn't get there?

The logic guarding this code is:
> 183   for ( ; aFrame; aFrame = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(aFrame)) {
[...]
> 242     if ((aChange & nsChangeHint_UpdateTransformLayer) &&
> 243         aFrame->IsTransformed()) {
           [new line here] 

For any frame that has continuations & experiences a transform-change (which presumably includes CSS transforms), I'd expect we'll hit this line for each of its continuations.
Comment on attachment 8489632 [details] [diff] [review]
observers.txt

r+, thanks!
Attachment #8489632 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9532ec1b7e80

Couldn't create a reliable reftest. svg-as-image is not interactive and timeouts make things flaky.
https://hg.mozilla.org/mozilla-central/rev/9532ec1b7e80
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.