Closed
Bug 1067375
Opened 10 years ago
Closed 10 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)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: longsonr, Assigned: longsonr)
References
Details
(Keywords: regression)
Attachments
(5 files, 1 obsolete file)
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8489366 -
Attachment is patch: false
Assignee | ||
Updated•10 years ago
|
Attachment #8489366 -
Attachment mime type: text/plain → image/svg+xml
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8489377 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8489417 -
Attachment mime type: text/plain → text/html
Updated•10 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 5•10 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
Comment 6•10 years ago
|
||
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
Comment 7•10 years ago
|
||
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
Comment 8•10 years ago
|
||
(I'll bet the layers code, referenced at line 134 there, doesn't trigger a call to nsSVGEffects::InvalidateRenderingObservers...)
Comment 9•10 years ago
|
||
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
Comment 10•10 years ago
|
||
...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•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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•10 years ago
|
Attachment #8489632 -
Flags: review?(jwatt)
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
Comment on attachment 8489632 [details] [diff] [review] observers.txt r+, thanks!
Attachment #8489632 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 16•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7686ad338dcd
Assignee | ||
Comment 17•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cb95257f2684
Assignee | ||
Comment 18•10 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.
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9532ec1b7e80
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•