703992 - SVG animation with indefinite repeatDur doesn't repeat

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Torbjörn Andersson]

Attachment: The SVG image/animation, extracted from the "HTML5 Dashboard" demo

User Agent: Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0 Iceweasel/8.0
Build ID: 20111109093931

Steps to reproduce:

I loaded the attached test case, which I've extracted from the "HTML Dashboard" demo from <https://developer.mozilla.org/en-US/demos/detail/html5-dashboard>. It is part of the "HTML PARSER" example.


Actual results:

The static parts of the image were drawn properly, but the circle that's supposed to move along the dashed line wasn't moving. If I looked quickly enough, I could see it move correctly from left to right, then incorrectly in a straight line from right to left, and then stop.


Expected results:

The circle is supposed to follow the dashed line back and forth indefinitely. (Or at least, that's how it worked until now.)

Comment 1

[Torbjörn Andersson]

As far as I can tell, the regression is quite recent.

Works as expected: http://hg.mozilla.org/mozilla-central/rev/086dea3f0bad
Does not work as expected: http://hg.mozilla.org/mozilla-central/rev/30161b298513

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=086dea3f0bad&tochange=30161b298513

Comment 2

[Torbjörn Andersson]

A few other things I noticed along the way. I'm not really familiar with SVG animations, so please excuse me if I'm writing nonsense.

The problem seems to be that the animations run only once. The up/down animation is shorter than the left/right one, so that's why it moves back in a straight line.

The problem only seems to appear when repeatDur is "indefinite". If I simply set it to, say, "1h", the circle will move back and forth. Presumably for an hour.

If I use repeatCount instead of repeatDur, it seems to work correctly even when the count is "indefinite".

Comment 3

[Torbjörn Andersson]

Smaller regression range:

Good: http://hg.mozilla.org/mozilla-central/rev/2a1ec9ca46cd
Bad:  http://hg.mozilla.org/mozilla-central/rev/36b19dca7f91

I wasn't able to get a working fromchange/tochange URL from that (maybe because it's part of a merge?), but if I'm right (please double-check that) it means that 36b19dca7f91 was the commit that introduced the regression. This change was a fix for Bug 691337.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Torbjörn Andersson from comment #3)
> I wasn't able to get a working fromchange/tochange URL from that (maybe
> because it's part of a merge?)

Yup -- specifically, it's because those two csets are part of the same push, and fromchange/tochange works at the level of pushes, rather than at the level of csets (which is unfortunate).

> it means that 36b19dca7f91 was the commit that introduced the regression.
> This change was a fix for Bug 691337.

Yeah, I think this makes sense as a regression from that bug. birtles, mind taking a look?

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (should continue moving back & forth)

Here's a reduced testcase that triggers the bug a little sooner. :)

The test should continue to bounce back & forth.  (It doesn't, in current nightly builds.)

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Proposed patch v1a

Proposed patch.

The patch for bug 691337 inadvertently altered the error handling of clock value parsing. This patch fixes that and hopefully makes it a little clearer.

What's most concerning is that none of our automated tests picked this up. I guess that's due to bug 474742.

Comment 7

[Daniel Holbert [:dholbert]]

Oops, sorry - I forgot that this patch was awaiting r?me. I'll review it today, sorry for the delay.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 576670 [details] [diff] [review]
Proposed patch v1a

>+  // Indicates we have started parsing a clock-value (not including the optional
>+  // +/- that precedes the clock-value)
>   bool started = false;

This comment isn't entirely accurate after the rest of this patch -- now you toggle |started| when we start parsing "indefinite" & "media", too.

>-    } else if ((aFlags & kClockValueAllowSign)
>-               && (*start == '+' || *start == '-')) {
>+    } else if (!started && (aFlags & kClockValueAllowSign) &&
>+        (*start == '+' || *start == '-')) {

Indentation is off on the second line, in the new code. (I think you can leave the second line there untouched, in its pre-patch form, if the only change there was indentation.)

Also: http://mxr.mozilla.org/mozilla-central/search?string=kClockValueAllowSign says that kClockValueAllowSign is never actually used, which would mean that this clause is never visited.  Is that bad?  (I thought +/- prefixes worked correctly, so this surprises me...)

>-      if ((aFlags & kClockValueAllowIndefinite)
>+      if (!started && (aFlags & kClockValueAllowIndefinite)
>           && ConsumeSubstring(start, end, "indefinite")) {

While you're touching this chunk, bump the second line's "&&" up to the end of the first line. (more common moz style, & matches your syntax elsewhere in this function e.g. for 'ConsumeSubstring(start, end, "media")' a few lines below.)

(Not granting r+ at this point in case the kClockValueAllowSign-unused issue triggers some code-rework).

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 576670 [details] [diff] [review]
Proposed patch v1a

>-    } else if ((aFlags & kClockValueAllowSign)
>-               && (*start == '+' || *start == '-')) {
>+    } else if (!started && (aFlags & kClockValueAllowSign) &&
>+        (*start == '+' || *start == '-')) {
>       // check sign has not already been set
>       if (sign != 0) {
>         return NS_ERROR_FAILURE;
>       }


My initial reaction to this error-check, given the change, was "the sign can't already have been set, because now you're checking "!started". However, my reaction was wrong -- I think we still need this error-check, but only to catch multiple initial +/- characters in a row, e.g. "++-10:00:00".

It's probably worth clarifying the comment here, to clarify that.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Proposed patch v1b

Address review feedback.

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Also:
> http://mxr.mozilla.org/mozilla-central/search?string=kClockValueAllowSign
> says that kClockValueAllowSign is never actually used, which would mean that
> this clause is never visited.  Is that bad?  (I thought +/- prefixes worked
> correctly, so this surprises me...)

It turns out we were using 'true' instead of kClockValueAllowSign. Fixed in updated patch.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 577815 [details] [diff] [review]
Proposed patch v1b

Ah, phew / makes sense. :)  Thanks for the explanation & fix.

r=me

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab37c20227f8

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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