Closed Bug 699143 Opened 10 years ago Closed 10 years ago

beginElement() begins not taking document time into account

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: jwatt, Assigned: dholbert)

References

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached image 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.
Calling nsSMILTimedElement::BeginElementAt in nsSMILTimedElement::BeginElementAt does not seem to be sufficient to fix this bug.
Err, calling nsSMILTimedElement::UpdateCurrentTime() I mean.
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.
Attachment #571497 - Attachment is patch: false
Attachment #571497 - Attachment mime type: text/plain → image/svg+xml
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()
(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.
OK, calling UpdateCurrentTime() inside of nsSMILAnimationController::RegisterAnimationElement (after the StartSampling call) seems to fix this.  I think that makes sense...
Attached patch fix (obsolete) — Splinter Review
Still need to write automated tests, but this should do it for the fix, I think.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #571515 - Flags: review?(birtles)
Attached patch fix v2 (obsolete) — Splinter Review
(reverted an accidental/unrelated newline-insertion)
Attachment #571515 - Attachment is obsolete: true
Attachment #571515 - Flags: review?(birtles)
Attachment #571531 - Flags: review?(birtles)
Attached patch fix v2a (now with mochitest) (obsolete) — Splinter Review
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.
Attachment #571531 - Attachment is obsolete: true
Attachment #571531 - Flags: review?(birtles)
Attachment #571826 - Flags: review?(birtles)
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.
(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
Attachment #571826 - Attachment is obsolete: true
Attachment #571826 - Flags: review?(birtles)
Attachment #571872 - Flags: review?(birtles)
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...
Attachment #571872 - Flags: review?(birtles)
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').
Attachment #571872 - Attachment is obsolete: true
Attachment #571874 - Flags: review?(birtles)
Comment on attachment 571874 [details] [diff] [review]
fix v3a (test updated)

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

Looks great! Thanks Daniel!
Attachment #571874 - Flags: review?(birtles) → review+
https://hg.mozilla.org/mozilla-central/rev/d6b5c76be4d0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 700077
Duplicate of this bug: 646134
You need to log in before you can comment on or make changes to this bug.