Last Comment Bug 703992 - SVG animation with indefinite repeatDur doesn't repeat
: SVG animation with indefinite repeatDur doesn't repeat
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Brian Birtles (:birtles)
:
Mentors:
Depends on:
Blocks: 691337
  Show dependency treegraph
 
Reported: 2011-11-20 09:50 PST by Torbjörn Andersson
Modified: 2011-12-01 15:50 PST (History)
3 users (show)
bbirtles: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The SVG image/animation, extracted from the "HTML5 Dashboard" demo (541 bytes, text/html)
2011-11-20 09:50 PST, Torbjörn Andersson
no flags Details
testcase 2 (should continue moving back & forth) (193 bytes, image/svg+xml)
2011-11-20 19:47 PST, Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th)
no flags Details
Proposed patch v1a (5.98 KB, patch)
2011-11-23 18:14 PST, Brian Birtles (:birtles)
no flags Details | Diff | Splinter Review
Proposed patch v1b (7.49 KB, patch)
2011-11-29 17:34 PST, Brian Birtles (:birtles)
dholbert: review+
Details | Diff | Splinter Review

Description Torbjörn Andersson 2011-11-20 09:50:00 PST
Created attachment 575752 [details]
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 2011-11-20 09:52:13 PST
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 2011-11-20 10:04:39 PST
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 2011-11-20 13:18:01 PST
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] (mostly OOTO until Aug 9th) 2011-11-20 19:39:00 PST
(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] (mostly OOTO until Aug 9th) 2011-11-20 19:47:03 PST
Created attachment 575796 [details]
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) 2011-11-23 18:14:44 PST
Created attachment 576670 [details] [diff] [review]
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] (mostly OOTO until Aug 9th) 2011-11-28 10:11:32 PST
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] (mostly OOTO until Aug 9th) 2011-11-28 10:58:16 PST
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] (mostly OOTO until Aug 9th) 2011-11-28 12:56:23 PST
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) 2011-11-29 17:34:49 PST
Created attachment 577815 [details] [diff] [review]
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] (mostly OOTO until Aug 9th) 2011-11-29 17:40:41 PST
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) 2011-11-30 15:47:31 PST
Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab37c20227f8
Comment 13 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-12-01 04:44:22 PST
https://hg.mozilla.org/mozilla-central/rev/ab37c20227f8

Note You need to log in before you can comment on or make changes to this bug.