699143 - beginElement() begins not taking document time into account

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

beginElement() does not seem to work if you have a document with no SMIL elements, then insert one and call beginElement() on it in the same script. In this case it seems like we're not keeping a timeline, so when nsSMILTimedElement::BeginElementAt calls container->GetCurrentTime() it returns 0, since we haven't been bothering to set its mCurrentTime.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: testcase

In this testcase, when you click it inserts a blue rect with an animation element. You should see the rect animate from 0% to 100% width. However, if you wait a second after loading the page before clicking, it will start midway through the animation.

Comment 2

[Jonathan Watt [:jwatt]]

Calling nsSMILTimedElement::BeginElementAt in nsSMILTimedElement::BeginElementAt does not seem to be sufficient to fix this bug.

Comment 3

[Jonathan Watt [:jwatt]]

Err, calling nsSMILTimedElement::UpdateCurrentTime() I mean.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case (just added a <set/> element)

Here's a tweaked version of the testcase, with just a <set/> element added (not targeting any particular attribute).

This test-tweak is sufficient to init the SMIL timeline correctly and produce expected behavior.

Comment 5

[Daniel Holbert [:dholbert]]

UpdateCurrentTime is semi-misleading -- we actually receive the "current time" value in nsSMILAnimationController::WillRefresh().

That callback arrives asynchronously, after we've registered with the refresh driver, and we only register with the refresh driver when the first animation element is added to the document.  So presumably our first WillRefresh() is firing *after* our beginElement() call, so the timestamp that beginElement sees is the (stale) document-start-time timestamp.

I suspect we might need to update mCurrentSampleTime in the mDeferredStartSampling case within nsSMILAnimationController::RegisterAnimationElement()

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> I suspect we might need to update mCurrentSampleTime in the
> mDeferredStartSampling case within
> nsSMILAnimationController::RegisterAnimationElement()

Oh, nevermind -- we already update it inside of nsSMILAnimationController::StartSampling.  Hmm... investigating more.

Comment 7

[Daniel Holbert [:dholbert]]

OK, calling UpdateCurrentTime() inside of nsSMILAnimationController::RegisterAnimationElement (after the StartSampling call) seems to fix this.  I think that makes sense...

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix

Still need to write automated tests, but this should do it for the fix, I think.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(reverted an accidental/unrelated newline-insertion)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v2a (now with mochitest)

I've added a mochitest to the patch now.

For the record, there are exactly two checks in the mochitest that fail in unpatched builds.  Those failing checks are this "isnot":
>  a.beginElement();
>  isnot(svg.getCurrentTime(), 0,
>       "beginElement should've triggered a synchronous sample " +
>       "and updated our current time");

and this "is()":

>  // Now, if we rewind to time=0, our "beginElement"-triggered animation
>  // shouldn't be in effect yet.
>  svg.setCurrentTime(0);
>  is(r.width.animVal.valueAsString, INITIAL_VAL,
>     "after rewinding to 0, our beginElement()-generated interval " +
>     "shouldn't be active yet");

(These checks pass with the fix applied, of course.)

I ran the fix through TryServer yesterday, and it passed.  Now I'm running it through TryServer with the mochitest included.

Comment 11

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

Comment on attachment 571826 [details] [diff] [review]
fix v2a (now with mochitest)

Review of attachment 571826 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in getting to this (public holiday yesterday). Thanks for taking care of this!

::: content/smil/nsSMILAnimationController.cpp
@@ +213,5 @@
> +      // mCurrentTime is probably stale.  We need to update it here (using the
> +      // just-updated mCurrentSampleTime that StartSampling has given us), or
> +      // else an immediate beginElement() call on the just-registered animation
> +      // element would incorrectly use our stale mCurrentTime as its beginning.
> +      UpdateCurrentTime();

I think this is fine but I wonder if we can somehow align with what we do in nsSMILAnimationController::AddChild. There we call Sample() before MaybeStartSampling to force a sample. Could we do the same here? (With the exception that we'd use StartSampling here instead of MaybeStartSampling).

The behaviour would be different of course--we'd actually be doing a sample and the test case behaviour would differ--but I wonder if that's a problem or if in fact it's more intuitive as well as making the code a little more consistent. I need to think it through a bit more.

::: content/smil/test/test_smilDynamicDelayedBeginElement.xhtml
@@ +1,3 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=

Missing id here.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (now calling Sample() instead of UpdateCurrentTime())

(In reply to Brian Birtles (:birtles) from comment #11)
> Sorry for the delay in getting to this (public holiday yesterday). Thanks
> for taking care of this!

No worries! Didn't seem like much of a delay at all, actually. :) Thanks for the review.

> I think this is fine but I wonder if we can somehow align with what we do in
> nsSMILAnimationController::AddChild. There we call Sample() before
> MaybeStartSampling to force a sample. Could we do the same here?

Almost. We can definitely replace the UpdateCurrentTime() call with a Sample() call (since Sample will trigger an UpdateCurrentTime) -- however, we can't move that call *before* StartSampling(), or else it won't benefit from the updated mCurrentSampleTime that we get inside of StartSampling.

So, this patch adds a synchronous Sample() as you suggest, but it adds it _after_ StartSampling() so that we use the updated mCurrentSampleTime.  (otherwise the attached mochitest fails)

(I also updated the mochitest so it'll now expect getCurrentTime to be updated slightly earlier now -- after the element-insertion, rather than after the beginElement call)

Also: For consistency, I tweaked AddChild so that its "[Maybe]StartSampling(); Sample();" order matches the new RegisterAnimationElement() code.  I don't anticipate that change having any significant functional effect.

> > +https://bugzilla.mozilla.org/show_bug.cgi?id=
> 
> Missing id here.

Oops. :)  As I wrote this test, I was actually laughing internally at how many different places one needs to paste the bug ID into an autogenerated mochitest template... I pasted it in 4 places, but missed the 5th. :)  Fixed now.

Currently running this through TryServer as a sanity-check, btw:
  https://tbpl.mozilla.org/?tree=Try&rev=2bc097621248

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 571872 [details] [diff] [review]
fix v3 (now calling Sample() instead of UpdateCurrentTime())

oops, there's a comment I need to update in the test. doing that...

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix v3a (test updated)

OK, updated the prose in the test, now that getCurrentTime will be updated as soon as we insert the animation element.

I also added one more check (to verify that simply inserting the animation doesn't have any effect on its target attr, since it has no intervals yet due to begin='indefinite').

Comment 15

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

Comment on attachment 571874 [details] [diff] [review]
fix v3a (test updated)

Review of attachment 571874 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks Daniel!

Comment 16

[Daniel Holbert [:dholbert]]

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

Comment 17

[Marco Bonardo [:mak]]

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