beginElement() begins not taking document time into account

RESOLVED FIXED in mozilla10

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwatt, Assigned: dholbert)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

6 years ago
Created attachment 571373 [details]
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.
(Reporter)

Comment 2

6 years ago
Calling nsSMILTimedElement::BeginElementAt in nsSMILTimedElement::BeginElementAt does not seem to be sufficient to fix this bug.
(Reporter)

Comment 3

6 years ago
Err, calling nsSMILTimedElement::UpdateCurrentTime() I mean.
(Assignee)

Comment 4

6 years ago
Created attachment 571497 [details]
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.
(Assignee)

Updated

6 years ago
Attachment #571497 - Attachment is patch: false
Attachment #571497 - Attachment mime type: text/plain → image/svg+xml
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

6 years ago
Created attachment 571515 [details] [diff] [review]
fix

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)
(Assignee)

Comment 9

6 years ago
Created attachment 571531 [details] [diff] [review]
fix v2

(reverted an accidental/unrelated newline-insertion)
Attachment #571515 - Attachment is obsolete: true
Attachment #571515 - Flags: review?(birtles)
(Assignee)

Updated

6 years ago
Attachment #571531 - Flags: review?(birtles)
(Assignee)

Comment 10

6 years ago
Created attachment 571826 [details] [diff] [review]
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.
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.
(Assignee)

Comment 12

6 years ago
Created attachment 571872 [details] [diff] [review]
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
Attachment #571826 - Attachment is obsolete: true
Attachment #571826 - Flags: review?(birtles)
Attachment #571872 - Flags: review?(birtles)
(Assignee)

Comment 13

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

Comment 14

6 years ago
Created attachment 571874 [details] [diff] [review]
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').
Attachment #571872 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 16

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b5c76be4d0
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/d6b5c76be4d0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Depends on: 700077
(Assignee)

Updated

5 years ago
Duplicate of this bug: 646134
You need to log in before you can comment on or make changes to this bug.