1246320 - Avoid passing the target element when parsing a timing function.

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Boris Chiou [:boris]]

According to Bug 1174575 Comment 74, this is really odd that we need a target element in order to parse a timing function, so it's better to fix this.

Comment 1

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

Attachment: MozReview Request: Bug 1246320 part 0 - Whitespace fixes; r=whitespace-only

Review commit: https://reviewboard.mozilla.org/r/39425/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39425/

Comment 2

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

Attachment: MozReview Request: Bug 1246320 part 1 - Add AnimationUtils::GetDocumentFromJSContext; r=bz

Adds a utility function for getting the document on the global associated with
a JSContext. We will need this in various situations where we want to use
the CSS parser (which requires various bits of state we pull off a document)
to parse a timing function but might not have a target element.

Strictly speaking we currently always have a target element but in future we
expect to support creating KeyframeEffects without an associated target
element. Also, we will need this for some situations in bug 1245748 where we
need to parse CSS properties on keyframe objects when we may not have a
target element.

Review commit: https://reviewboard.mozilla.org/r/39427/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39427/

Comment 3

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

Attachment: MozReview Request: Bug 1246320 part 2 - Pass document to ParseEasing

Review commit: https://reviewboard.mozilla.org/r/39429/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39429/

Comment 4

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

Attachment: MozReview Request: Bug 1246320 part 3 - Rework KeyframeEffect(ReadOnly) constructor helpers

Once we update TimingParams to take a document, we will need to get an
appropriate document within the various constructor methods. This complicates
these methods and suggests they should be pushed into the .cpp file where
we can hide the complexity more easily and templatize the type of the options
argument so that we can share the document-fetching code.

By moving all uses of the declared template methods to the .cpp file we
can drop the explicit instantiations.

(We still need to declare the templated methods in the header file since
these methods need to be protected methods of KeyframeEffectReadOnly in
order to construct a KeyframeEffectReadOnly since its constructor is
protected.)

Review commit: https://reviewboard.mozilla.org/r/39431/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39431/

Comment 5

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

Attachment: MozReview Request: Bug 1246320 part 4 - Pass a document to TimingParams

Review commit: https://reviewboard.mozilla.org/r/39433/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39433/

Comment 6

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

Comment on attachment 8729394 [details]
MozReview Request: Bug 1246320 part 3 - Rework KeyframeEffect(ReadOnly) constructor helpers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39431/diff/1-2/

Comment 7

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

Comment on attachment 8729395 [details]
MozReview Request: Bug 1246320 part 4 - Pass a document to TimingParams

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39433/diff/1-2/

Comment 8

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

Boris, I'm not sure if this part 1 makes any sense at all. And if it does, I wonder if it belongs somewhere else.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8729392 [details]
MozReview Request: Bug 1246320 part 1 - Add AnimationUtils::GetDocumentFromJSContext; r=bz

https://reviewboard.mozilla.org/r/39427/#review36197

::: dom/animation/AnimationUtils.cpp:108
(Diff revision 1)
>  }
>  
> +/* static */ nsIDocument*
> +AnimationUtils::GetDocumentFromJSContext(JSContext* aCx)
> +{
> +  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

You want xpc::CurrentWindowOrNull here, I think.  Less work on your end.

Also, I'd call this GetCurrentRealmDocument() to make it clear which of the 4 (I think) concepts of "document associated with jscontext" you're actually using here.

r=me with that.

Comment 10

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

Comment on attachment 8729391 [details]
MozReview Request: Bug 1246320 part 0 - Whitespace fixes; r=whitespace-only

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39425/diff/1-2/

Comment 11

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

Comment on attachment 8729392 [details]
MozReview Request: Bug 1246320 part 1 - Add AnimationUtils::GetDocumentFromJSContext; r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39427/diff/1-2/

Comment 12

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

Comment on attachment 8729393 [details]
MozReview Request: Bug 1246320 part 2 - Pass document to ParseEasing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39429/diff/1-2/

Comment 13

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

Comment on attachment 8729394 [details]
MozReview Request: Bug 1246320 part 3 - Rework KeyframeEffect(ReadOnly) constructor helpers

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39431/diff/2-3/

Comment 14

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

Comment on attachment 8729395 [details]
MozReview Request: Bug 1246320 part 4 - Pass a document to TimingParams

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39433/diff/2-3/

Comment 15

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

Attachment: MozReview Request: Bug 1246320 part 5 - Simplify KeyframeEffect(ReadOnly) Constructor overloads further

As well as generally simplifying the different KeyframeEffect(ReadOnly)
constructor methods, this patch also means we will use the realm document for
parsing timing functions in all cases. Although this currently doesn't have
any impact (the current set of timing functions are expected to be parsed
identically regardless of the document used) it may become significant if, in
future, it becomes possible to register hooks with certain documents for
parsing CSS properties as part of the houdini efforts.

Review commit: https://reviewboard.mozilla.org/r/39595/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39595/

Comment 16

[Boris Zbarsky [:bzbarsky]]

I would be a bit careful about the realm document thing.  Once it actually matters, specs will need to define which document is used and there is no guarantee that they will pick the realm document...

Comment 17

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

(In reply to Boris Zbarsky [:bz] from comment #16)
> I would be a bit careful about the realm document thing.  Once it actually
> matters, specs will need to define which document is used and there is no
> guarantee that they will pick the realm document...

Yes, for sure. But until then I think we should ideally be consistent in case the document used in parsing does become significant. Element.animate() is supposed to be equivalent to constructing the necessary objects, putting them together and playing them. If the document used becomes significant, it would be better to consistently do either the right or the wrong thing so that we don't accidentally ship in a state where one approach works but another doesn't.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/262b8fce9561
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a82b7529ca5

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8729393 [details]
MozReview Request: Bug 1246320 part 2 - Pass document to ParseEasing

https://reviewboard.mozilla.org/r/39429/#review36293

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8729394 [details]
MozReview Request: Bug 1246320 part 3 - Rework KeyframeEffect(ReadOnly) constructor helpers

https://reviewboard.mozilla.org/r/39431/#review36299

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Element.animate() is supposed to be equivalent to constructing the necessary objects

Constructing them in the global of the animate function, the global of the element, or the global of the caller of the function?

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8729832 [details]
MozReview Request: Bug 1246320 part 5 - Simplify KeyframeEffect(ReadOnly) Constructor overloads further

https://reviewboard.mozilla.org/r/39595/#review36291

::: dom/animation/KeyframeEffect.h:431
(Diff revision 1)
>    // Not exposed to content.
>    static already_AddRefed<KeyframeEffect>
>    Constructor(const GlobalObject& aGlobal,
>                const Nullable<ElementOrCSSPseudoElement>& aTarget,
>                JS::Handle<JSObject*> aFrames,
> -              const TimingParams& aTiming,
> +              const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
>                ErrorResult& aRv);

Nit: UnrestrictedDoubleOrKeyframeAnimationOptions shoule be forward declared.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8729395 [details]
MozReview Request: Bug 1246320 part 4 - Pass a document to TimingParams

https://reviewboard.mozilla.org/r/39433/#review36301

Although I can't judge which document should be used for parsing CSS properties other than timing functions, this patch looks good to me.

It's not related to this patch, TimingParamsFromOptionsUnion should be rewritten a little at some point.  It could return early in some cases.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/262b8fce9561
https://hg.mozilla.org/mozilla-central/rev/7a82b7529ca5

Comment 25

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

(In reply to Boris Zbarsky [:bz] from comment #21)
> > Element.animate() is supposed to be equivalent to constructing the necessary objects
> 
> Constructing them in the global of the animate function, the global of the
> element, or the global of the caller of the function?

The caller of the function, I believe.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> The caller of the function, I believe.

OK, then that would be the incumbent global, not the current Realm global.  But note that Microsoft has been pushing back pretty hard on using "the caller of the function" because it requires expensive stack inspection.... Can you point me to the place where the spec says this?

> Although I can't judge which document should be used for parsing CSS properties other than timing functions

Actually, this is important too, because it affects URI resolution (in particular what base URI is used).  Again, I would expect the relevant spec to define it.  Does it?

Comment 27

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

(In reply to Boris Zbarsky [:bz] from comment #26)
> > The caller of the function, I believe.
> 
> OK, then that would be the incumbent global, not the current Realm global. 
> But note that Microsoft has been pushing back pretty hard on using "the
> caller of the function" because it requires expensive stack inspection....
> Can you point me to the place where the spec says this?

It doesn't use any of those terms, but after the procedural description it says[1]:

  var animation = elem.animate({ opacity: 0 }, 2000);

  is equivalent to:

  var effect = new KeyframeEffect(elem, { opacity: 0 }, 2000);
  var animation = new Animation(effect, elem.ownerDocument.timeline);
  animation.play();

[1] http://w3c.github.io/web-animations/#dom-animatable-animate

I've probably misunderstood the terms though.

> > Although I can't judge which document should be used for parsing CSS properties other than timing functions
> 
> Actually, this is important too, because it affects URI resolution (in
> particular what base URI is used).  Again, I would expect the relevant spec
> to define it.  Does it?

No, it doesn't but indeed, it should.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> It doesn't use any of those terms, but after the procedural description it says[1]:

Ok, but was that a principled decision or just an attempt to write some informative text without actually considering the full complexity of the situation?  Or is that bit after the steps meant to be normative?

Anyway, what the spec needs to do is clearly specify which globals are used to create the objects it's creating.  If it doesn't do that, then for now it's not following the IDL spec and in the long term it will end up with the default behavior Anne wants to add to IDL, which will use the global of the "this" value of the method (in this case the element).

Comment 29

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

(In reply to Boris Zbarsky [:bz] from comment #28)
> > It doesn't use any of those terms, but after the procedural description it says[1]:
> 
> Ok, but was that a principled decision or just an attempt to write some
> informative text without actually considering the full complexity of the
> situation?  Or is that bit after the steps meant to be normative?

It wasn't a principled decision, just an naive attempt to explain the intended effect. It is technically normative but shouldn't be.

> Anyway, what the spec needs to do is clearly specify which globals are used
> to create the objects it's creating.  If it doesn't do that, then for now
> it's not following the IDL spec and in the long term it will end up with the
> default behavior Anne wants to add to IDL, which will use the global of the
> "this" value of the method (in this case the element).

I'll gladly make the spec use whatever you recommend. For now, I suppose it's ok to land this using the realm document?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Probably yes, as long as we have a plan to actually figure out the spec here (in conjunction with other implementors) and then change to whatever is decided.

I'm not sure I have any particular recommendations at this stage, because I don't know what this API is aiming to do, exactly, and hence what observables really depend on the document and which behavior they should have...

Comment 31

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

(In reply to Boris Zbarsky [:bz] from comment #30)
> Probably yes, as long as we have a plan to actually figure out the spec here
> (in conjunction with other implementors) and then change to whatever is
> decided.

Filed a spec issue for this: https://github.com/w3c/web-animations/issues/145

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef42bdeeabb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d44886eff3
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56bc90e163d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2891e4650c

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ef42bdeeabb
https://hg.mozilla.org/mozilla-central/rev/f7d44886eff3
https://hg.mozilla.org/mozilla-central/rev/e56bc90e163d
https://hg.mozilla.org/mozilla-central/rev/5b2891e4650c