The default bug view has changed. See this FAQ.

SVG SMIL: Instance times should not be dependent on themselves

VERIFIED FIXED in mozilla7

Status

()

Core
SVG
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 538402 [details]
Proposed patch v1a

In some arrangements of syncbase timing we can arrive at a situation where an instance time is directly dependent on itself. This shouldn't be.

Whilst self-referential timing such as:

  <animate id="a" begin="a.begin+2s" .../>

is perfectly reasonable (and required by the spec) setting an interval endpoint to an instance time that is already dependent on that same interval endpoint doesn't make sense.

This is best illustrated in the test cases in the attached patch.

Just how we should behave isn't specified by SVG/SMIL since they don't really deal with dynamic document changes. However, I think the test cases in the attached patch demonstrate why this new behaviour is correct.

Note that the patch applies on top of the patch for bug 650732 and may not apply entirely cleanly otherwise.
(Assignee)

Updated

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

Comment 1

6 years ago
Created attachment 538404 [details] [diff] [review]
Proposed patch v1b
Attachment #538402 - Attachment is obsolete: true
Attachment #538404 - Flags: review?(dholbert)
(Assignee)

Comment 2

6 years ago
Created attachment 538405 [details]
Test case 1

Test case 1 -- identical to cycle-self-ref-4.svg from the proposed patch but attached separately here for easier testing.
(Assignee)

Comment 3

6 years ago
Created attachment 538406 [details]
Test case 2

Test case 2 -- identical to cycle-self-ref-5.svg from the proposed patch but attached separately here for easier testing.
Comment on attachment 538404 [details] [diff] [review]
Proposed patch v1b

A few notes on "Testcase 2" a.k.a cycle-self-ref-5.svg:

>+       If b's interval disappears the end time "a.end+2s" (calculated as
>+       t=(b.end=3s)+2s=5s, but then updated to become 7s) should not be used as
>+       the end time since b never really ended.

I don't understand the "but then updated to become 7s" part there -- that sounds like something is changing in the document to replace the 5s ending with a 7s ending, but I don't see why that'd happen.

Perhaps this is just talking about the third "end" event that we queue up for element 'a', at (3s + 2s) + 2s = 7s?  If so, it might be good to (a) use a different word from "updated" & (b) explain why we're specifically watching out for the 7s end event, instead of the 5s one.

Also, could you add a comma after "disappears", & after "used as the end time".
Comment on attachment 538404 [details] [diff] [review]
Proposed patch v1b

>    * @param aPrevInterval   The previous interval used. If supplied, the first
>    *                        interval that begins after aPrevInterval will be
>    *                        returned. May be nsnull.
>+   * @param aCurrentInterval The current interval that is being updated. This is
>+   *                        used to ensure we don't return interval endpoints
>+   *                        that are dependent on themselves. May be nsnull.
[...]
>    * @return  PR_TRUE if a suitable interval was found, PR_FALSE otherwise.
>    */
>   PRBool            GetNextInterval(const nsSMILInterval* aPrevInterval,
>+                                    const nsSMILInterval* aCurrentInterval,
>                                     const nsSMILInstanceTime* aFixedBeginTime,
>                                     nsSMILInterval& aResult) const;

I don't love the name "aCurrentInterval" here -- it's a bit confusing. My thought process when initially re-paging in this code: "So, given a previous interval, this method looks up the next interval.  And now we're also considering a 'current' interval. So 'current' sounds like it's in between 'previous' and 'next'... but I thought 'next' was the interval immediately after 'previous'!"  (I've sorted this out now, but it's confusing on an initial reading. :))
(side-note: the above reminds me of http://www.youtube.com/watch?v=VeZ9HhHU86o&t=54s )

Perhaps we could name this argument aReplacedInterval, or something like that?  I'd prefer that we remove the word "current" from the method header-comment, too ("The current interval that is being updated"), to completely avoid potentially-confusing temporal implications. :)

r=dholbert with that
Attachment #538404 - Flags: review?(dholbert) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 539959 [details] [diff] [review]
Proposed patch v1c, r=dholbert

Thanks Daniel! Those are good comments. I've addressed the review feedback in this patch and updated the comments for the test case too.
Attachment #538404 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
Comment on attachment 539959 [details] [diff] [review]
Proposed patch v1c, r=dholbert

Landed on m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/76781a68c1ba
Attachment #539959 - Flags: checkin+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/76781a68c1ba
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7

Comment 9

6 years ago
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 1

Hi. I wanted to verify this bug by running the two testcases. In both of the tabs, there is a green square.

I have tried the same thing in Google Chrome, but in the second test case, a red square appears that turns green.

Wich is the normal behavior?
(Assignee)

Comment 10

6 years ago
(In reply to Vlad from comment #9)
> Hi. I wanted to verify this bug by running the two testcases. In both of the
> tabs, there is a green square.

This is the correct behaviour, both should be green.
 
> I have tried the same thing in Google Chrome, but in the second test case, a
> red square appears that turns green.

Chrome is wrong. Unfortunately WebKit's support of SMIL is poor so many of our test cases fail in Chrome.

Comment 11

6 years ago
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.