Closed Bug 975757 Opened 6 years ago Closed 5 years ago

animated updates to a <use> within a <pattern> do not cause a repaint


(Core :: SVG, defect)

23 Branch
Not set



Tracking Status
firefox31 --- affected
firefox32 --- affected
firefox33 --- affected
firefox34 --- affected
firefox38 --- verified
firefox-esr24 --- affected
firefox-esr31 --- affected


(Reporter: david.dailey, Assigned: longsonr)



(Keywords: regression, Whiteboard: [testday-20150123])


(2 files, 1 obsolete file)

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

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.
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 27 Branch → 23 Branch
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
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130427 Firefox/23.0 ID:20130427184205
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130428 Firefox/23.0 ID:20130428035504

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??
Both work as intended in Opera, Chrome, but stall in FF 
This almost works in FF but stalls short of complete rotation.
Duplicate of this bug: 1041773
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.
Attached patch observers.txt (obsolete) — Splinter Review
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)
Attached patch reftestSplinter Review
Attachment #8506302 - 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:


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.
Attachment #8505455 - Flags: review?(jwatt) → review-
Attachment #8506302 - Flags: review?(jwatt) → review+
Attached patch observers.txtSplinter Review
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.
Attachment #8505455 - Attachment is obsolete: true
Attachment #8509317 - Flags: review?(jwatt)
Duplicate of this bug: 1041661
Blocks: 908634
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]

Review of attachment 8509317 [details] [diff] [review]:


::: 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="" 

You have a trailing space at the end of this first line.
Attachment #8509317 - Flags: review?(jwatt) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1090936
Blocks: 1042865
QA Whiteboard: [good first verify]
Build identifier: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:38.0) Gecko/20100101 Firefox/38.0

Verified fixed.
Whiteboard: [testday-20150123]
In Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 I also verify it's fixed.
Flags: needinfo?(david.dailey)
Yep! It looks fixed to me... thanks
Flags: needinfo?(david.dailey)
You need to log in before you can comment on or make changes to this bug.