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

RESOLVED FIXED in mozilla35

Status

()

Core
SVG
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

({regression})

Trunk
mozilla35
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8489366 [details]
image.svg (part of broken testcase)
(Assignee)

Comment 2

3 years ago
Created attachment 8489367 [details]
image2.svg (part of working testcase - same as broken but without the final static circle)
(Assignee)

Updated

3 years ago
Attachment #8489366 - Attachment is patch: false
(Assignee)

Updated

3 years ago
Attachment #8489366 - Attachment mime type: text/plain → image/svg+xml
(Assignee)

Comment 3

3 years ago
Created attachment 8489377 [details]
broken image container
(Assignee)

Updated

3 years ago
Attachment #8489377 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 4

3 years ago
Created attachment 8489417 [details]
working container
(Assignee)

Updated

3 years ago
Attachment #8489417 - Attachment mime type: text/plain → text/html
Version: unspecified → Trunk
(Assignee)

Comment 5

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

Comment 11

3 years ago
Created attachment 8489594 [details] [diff] [review]
observers.txt

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: → bug 975757
(Assignee)

Comment 13

3 years ago
Created attachment 8489632 [details] [diff] [review]
observers.txt

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

Updated

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

Comment 16

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7686ad338dcd
(Assignee)

Comment 17

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cb95257f2684
(Assignee)

Comment 18

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.