Closed Bug 975757 Opened 6 years ago Closed 5 years ago
animated updates to a <use> within a <pattern> do not cause a repaint
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release) Build ID: 20140212131424 Steps to reproduce: This used to work in FF the same way it does in Opera, Chrome and ASV http://srufaculty.sru.edu/david.dailey/svg/SVGOpen2008/barberPole.svg Actual results: It still works in Opera and Chrome (don't know about ASV), but it no longer works in FF Expected results: It should continue to work.
Looks like the animation is running and affecting the animated value of transform="" on the <use> in the <pattern>, but maybe we're not repainting? This stopped working between Firefox 22 and 23.
When you click on the barber pole it gets a repaint btw.
Summary: dragging content through clipPath no longer words → animated updates to a <use> within a <pattern> do not cause a repaint
Regression window Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/d6de91faf60a Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130427 Firefox/23.0 ID:20130427184205 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/c9af815f4a9f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130428 Firefox/23.0 ID:20130428035504 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d6de91faf60a&tochange=c9af815f4a9f Regressed by: dc6bd7ea66f6 Jonathan Watt — Bug 839865 - Stop calling nsSVGUtils::InvalidateBounds for SVG transform changes, and use DLBI instead. r=mattwoodrow.
These examples may have the same problem?? http://cs.sru.edu/~ddailey/fading/slides/BoxSlide.svg http://cs.sru.edu/~ddailey/fading/slides/BoxSlide2.svg Both work as intended in Opera, Chrome, but stall in FF http://cs.sru.edu/~ddailey/fading/slides/BoxSlide4.svg This almost works in FF but stalls short of complete rotation.
There's some diagnosis of a related issue in bug 1067375, and a WIP patch, both of which are likely relevant to this bug. So, this may end up being fixed by that bug's patch -- if so, we'll prbobably want to dupe this forward to that bug.
My patch in that bug doesn't fix this.
This backs out the patch in bug 1067375 as that doesn't seem to be needed with this fix in place.
Assignee: nobody → longsonr
Attachment #8505455 - Flags: review?(jwatt)
nsSVGEffects::InvalidateRenderingObservers involves walking up the tree, which is something we want to avoid doing more than necessary. The advantage of doing that when we handle the nsChangeHint_UpdateTransformLayer hint is that we can coalesce what could otherwise be multiple InvalidateRenderingObservers() calls. The reason that we don't end up in the code that handles the nsChangeHint_UpdateTransformLayer hint in this case is that nsSVGElement::DidAnimateTransformList calls SVGTransformableElement::GetAttributeChangeHint which would normally return nsChangeHint_UpdateTransformLayer, but in this case it returns no hints since the frame is NS_FRAME_IS_NONDISPLAY. The call stack is something like: SVGTransformableElement::GetAttributeChangeHint nsSVGElement::DidAnimateTransformList nsSVGAnimatedTransformList::SetAnimValue The reason SVGTransformableElement::GetAttributeChangeHint returns no hints in this case is because we don't want to waste time updating overflows etc. [which are unused] on NS_FRAME_IS_NONDISPLAY frames. It seems like we should add a new nsChangeHint_InvalidateRenderingObservers hint, and add that to retval at the start of SVGTransformableElement::GetAttributeChangeHint so that we return that in all cases. Now that I look more closely at SVGTransformableElement::GetAttributeChangeHint it also seem that the nsChangeHint_ReconstructFrame hint should not be shortcut for NS_FRAME_IS_NONDISPLAY. It doesn't really matter until we add z-index support, but you could fix that while you're there.
I didn't understand what you mean by the last sentence in the review comment. I'll raise a follow up to convert all other nsSVGEffects::InvalidateRenderingObservers to use the hint.
I'm still trying to understand a few aspects of the code before r+'ing this. I expect to do that today.
Comment on attachment 8509317 [details] [diff] [review] observers.txt Review of attachment 8509317 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: layout/base/nsChangeHint.h @@ +154,5 @@ > + > + /** > + * This will cause rendering observers to be invalidated. > + */ > + nsChangeHint_InvalidateRenderingObservers = 0x200000 This needs to be added to nsChangeHint_Hints_NotHandledForDescendants and NS_HintsNotHandledForDescendantsIn. With that change, r=me. ::: layout/reftests/svg/smil/transform/use-1.svg @@ +1,1 @@ > +<svg width="100%" xmlns="http://www.w3.org/2000/svg" You have a trailing space at the end of this first line.
Attachment #8509317 - Flags: review?(jwatt) → review+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Build identifier: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:38.0) Gecko/20100101 Firefox/38.0 Verified fixed.
In Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 I also verify it's fixed.
Yep! It looks fixed to me... thanks
You need to log in before you can comment on or make changes to this bug.