Closed Bug 975757 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: SVG, defect)

23 Branch
defect
Not set

Tracking

()

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

People

(Reporter: david.dailey, Assigned: longsonr)

References

Details

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

Attachments

(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
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.
Status: UNCONFIRMED → NEW
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
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.
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:

  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.
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]
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+
https://hg.mozilla.org/mozilla-central/rev/9eb2a0bd0afc
Status: NEW → RESOLVED
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)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.