1042860 - animate startOffset with negative value

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[david.dailey]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

http://srufaculty.sru.edu/david.dailey/svg/newstuff/textpath1.svg
animation of startOffset of text along textPath using negative value


Actual results:

Animation doesn't begin, though it does in Chrome, new and old Opera, Safari (Win). I think it used to work in Firefox, though I am not sure of this.


Expected results:

Text should crawl along the curve, as in Chrome.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (reporter's testcase)

Comment 2

[Daniel Holbert [:dholbert]]

Last good revision: cbb24a4a96af (2013-06-30)
First bad revision: d7553251cf43 (2013-07-01)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbb24a4a96af&tochange=d7553251cf43

Regression from http://hg.mozilla.org/mozilla-central/rev/3a23afb038a5 :
 Bug 839955 - Enable new SVG text frames

(To confirm this, I checked whether toggling "svg.text.css-frames.enabled" to false in a post-regression build would fix the problem. It does.)

This seems like it might be an invalidation bug -- if I resize the window continuously (forcing repaints), then I can see the text moving along.

heycam, do you have cycles to take a look?

Comment 3

[Cameron McCormack (:heycam)]

Thanks for finding the regression window; I'll take a look tomorrow.

Comment 4

[Cameron McCormack (:heycam)]

Although the attributes are animating, we're not responding to the changes in values.  This is the case for any animated attributes on child text content elements (<tspan>, <textPath>, ...).  So while we have a mutation observer set up by the SVGTextFrame to look for modifications of the attribute values (base values), the mutation observer doesn't get called when we just call into AttributeChanged from nsSVGElement::DidAnimateLength, since the frame we call it on is the nsInlineFrame for the child text content element.

I think we should, in nsSVGElement::DidAnimateLength and friends, look up to find the SVGTextFrame and call a method on it to report the animated attribute change.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 6

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=f17c599793e7

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8462352 [details] [diff] [review]
patch

Haven't looked at the code in detail yet, but here are a few nits on the test:

>+++ b/layout/reftests/bugs/1042860-1.svg

Let's put this bug's tests in layout/reftests/svg/text/, or perhaps layout/reftests/svg/smil/, and give the test files a more descriptive name.

The "bugs" directory is kind of a grab bag, and we shouldn't put *everything* there -- I think it's really only good for really weird edge-casey tests, or tests that don't have another clear home. (And in this case, this isn't edge-casey and there are several other homes that would make sense.)

>+  <text x="20" y="20">Test 1
>+    <set attributeName="x" to="200" begin="indefinite"/>

Based on comment 4, I'd expect we'd be able to trigger this bug with scripted attribute-changes, too (using the DOM APIs and/or setAttribute(), rather than using SMIL).

Could you add an additional reftest that tweaks at least one of these <text> attributes using setAttribute (or the DOM attribute-specific APIs), too?

>+  <script>
>+    window.addEventListener("load", function() {
>+      [...document.querySelectorAll("set")].forEach((e) => e.beginElement());
>+      document.documentElement.removeAttribute("class");
>+    }, false);

This should probably use MozReftestInvalidate instead of "load", to be sure we're giving ourselves a chance to render the initial state before we make these dynamic modifications.

Comment 8

[Robert Longson [:longsonr]]

If you moved the overrides from SVGTextContentElement.cpp to SVGTSpanElement.cpp and SVGTextPathElement.cpp then you wouldn't need the checks in layout that things are not coming from an SVGTextElement itself so SVGTextFrame::AnimatedAttributeChangedInSubtree would go. 

I'm torn over whether that's cleaner as you'd then need two copies of the overrides but you could put them in a macro as they are all boilerplate.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8462352 [details] [diff] [review]
patch

So, this patch addresses this by virtualizing nsSVGElement::DidAnimate*, and adding specialized versions that look at the frame tree to find the correct frame to notify. (the SVGTextFrame)

Per IRC discussion just now: if possible, I'd prefer that we keep the various nsSVGElement::DidAnimate* methods non-virtual (for simplicity/consistency), and instead, add some logic to the frame classes to "do the right thing" when we notify the wrong frame (in a SVGTextFrame subtree).

So e.g. nsInlineFrame would need an AttributeChanged() specialization, which would have a "IsSVGText()" block much like the other blocks that we already have there (to find the closest SVGTextFrame), and it'd pass along the notification to that SVGTextFrame. Or something along those lines.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: patch v2

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8484770 [details] [diff] [review]
patch v2

># HG changeset patch
># Parent 635055d16df6cbca72dc7bb793732a400558ec4a
># User Cameron McCormack <[e-mail]>
>
>Bug 1042860 - Handle animated attribute changes on descendants of SVG <text> elements. (v2)

(You'll want to remove "v2" before actually landing, of course)

>diff --git a/layout/generic/nsInlineFrame.cpp b/layout/generic/nsInlineFrame.cpp
>--- a/layout/generic/nsInlineFrame.cpp
>+++ b/layout/generic/nsInlineFrame.cpp
>+nsresult 
>+nsInlineFrame::AttributeChanged(int32_t aNameSpaceID,
>+                                nsIAtom* aAttribute,
>+                                int32_t aModType)
>+{
>+  if (IsSVGText()) {
>+    SVGTextFrame* f = static_cast<SVGTextFrame*>(
>+      nsLayoutUtils::GetClosestFrameOfType(this, nsGkAtoms::svgTextFrame));

(optional): If you like, this could be "auto f", to avoid having to repeat SVGTextFrame* twice.

>+    f->HandleAttributeChangeInDescendant(mContent->AsElement(),
>+                                         aNameSpaceID, aAttribute);
>+  }
>+
>+  return NS_OK;
>+}

We need to call the base class's AttributeChanged implementation somewhere. Either at the end, like so...

  return nsInlineFrameBase::AttributeChanged(aNameSpaceID, aAttribute,
                                             aModType);

...or at the beginning, and capture the "rv" for returning at the end.

(Not sure which is preferred; nsBlockFrame::AttributeChanged captures its base class's rv at the beginning, whereas nsGfxButtonControlFrame, nsRangeFrame, and nsFileControlFrame do it at the end. Probably doesn't matter much in this case anyway.) 

>diff --git a/layout/reftests/svg/dynamic-text-attr-01.svg b/layout/reftests/svg/dynamic-text-attr-01.svg
>+  <text x="20" y="20" data-test="x=200">Test 1
>+    <set attributeName="x" to="200" begin="indefinite"/>
>+  </text>

I don't think you want this <set> element here, in the non-SMIL testcase -- let's remove it.

>diff --git a/layout/svg/SVGTextFrame.cpp b/layout/svg/SVGTextFrame.cpp
>--- a/layout/svg/SVGTextFrame.cpp
>+++ b/layout/svg/SVGTextFrame.cpp
>@@ -14,16 +14,17 @@
>+#include "nsIDOMMutationEvent.h"

Why this #include-addition? I don't see nsIDOMMutationEvent or its constants being used in this file (before or after this patch).  Can this be removed?

r=me with the above addressed. Thanks!

Comment 13

[Cameron McCormack (:heycam)]

Thanks for the speedy review!

https://tbpl.mozilla.org/?tree=Try&rev=10d7148dab2c

Comment 14

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f73f0c65c4cb

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f73f0c65c4cb