SVG animation with indefinite repeatDur doesn't repeat

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Torbjörn Andersson, Assigned: birtles)

Tracking

({regression})

Trunk
mozilla11
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.)
(Reporter)

Comment 1

6 years ago
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
(Reporter)

Comment 2

6 years ago
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".
(Reporter)

Comment 3

6 years ago
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.
Attachment #575752 - Attachment mime type: text/plain → text/html
(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?
Blocks: 691337
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Linux → All
Hardware: x86 → All
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.)
(Assignee)

Updated

6 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
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.
Attachment #576670 - Flags: review?(dholbert)
Oops, sorry - I forgot that this patch was awaiting r?me. I'll review it today, sorry for the delay.
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 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.
(Assignee)

Comment 10

6 years ago
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.
Attachment #576670 - Attachment is obsolete: true
Attachment #576670 - Flags: review?(dholbert)
Attachment #577815 - Flags: review?(dholbert)
Comment on attachment 577815 [details] [diff] [review]
Proposed patch v1b

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

r=me
Attachment #577815 - Flags: review?(dholbert) → review+
(Assignee)

Comment 12

6 years ago
Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab37c20227f8
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/ab37c20227f8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.