Fix uses of AnimationUtils::GetCurrentRealmDocument to use the appropriate document

RESOLVED FIXED in Firefox 50

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(8 attachments)

58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
See bug 1275449 comment 15 onwards.

In particular, I think what we need to do here is:

Part 1:

* Update the spec to say that keyframe effects and timing objects are associated with the document. So far as I can tell this isn't observable so it doesn't need to block the code changes here. The only open question there is whether they should be associated with documents or windows and that only matters in the document.open or about:blank case and using the document seems like a reasonable option there.

* Update the parent object of AnimationEffectReadOnly to store an nsDocument* pointer (and update KeyframeEffect(ReadOnly) accordingly).

* Update the parent object of AnimationEffectTiming to store an nsDocument* pointer.

* Use AnimationEffectTiming::mDocument in SetEasing as the document to pass to ParseEasing.

* Make KeyframeEffectReadOnly::SetKeyframes use the composed doc of the target element and if there's no target element it doesn't need to resolve computed values.

* Make KeyframeEffectReadOnly::ConstructKeyframeEffect pass mDocument (inherited from AnimationEffectReadOnly) to ParseEasing.

* Pass mDocument to KeyframeUtils::GetKeyframesFromObject and have it use that for CSS parsing in RequiresAdditiveAnimation.

* Pass mDocument to ConvertKeyframeSequence in KeyframeUtils.cpp and have it use that for CSS parsing

* Pass mDocument to GetKeyframeListFromPropertyIndexedKeyframe in KeyframeUtils.cpp and have it use that for CSS parsing


Part 2:

* Update the spec to say that Element.animate creates an Animation object in the relevant Realm[1] of the Element on which is is called. We might need to spec what happens in non-window cases such as the result of DOMParser().parseFromString / XHR results (suggestion: throw). Also, we need to make sure that all further constructors triggered from there use the same realm.

* Update Element::Animate and CSSPseudoElement::Animate to do that.

[1] https://html.spec.whatwg.org/multipage/webappapis.html#concept-relevant-realm
Priority: -- → P3
Assignee: nobody → bbirtles
Review commit: https://reviewboard.mozilla.org/r/63850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63850/
Attachment #8770383 - Flags: review?(hiikezoe)
Attachment #8770384 - Flags: review?(hiikezoe)
Attachment #8770385 - Flags: review?(hiikezoe)
Attachment #8770386 - Flags: review?(bzbarsky)
Attachment #8770387 - Flags: review?(hiikezoe)
Attachment #8770388 - Flags: review?(hiikezoe)
Attachment #8770389 - Flags: review?(bzbarsky)
Attachment #8770390 - Flags: review?(hiikezoe)
There doesn't seem to be a need to have a separate 'sandbox' folder just for
this test. It's a Gecko-specific test so it can go in the 'mozilla' folder.

Review commit: https://reviewboard.mozilla.org/r/63854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63854/
Note that when we actually compute values, we will use the composed document of
the target element (see the next patch in this series).

Review commit: https://reviewboard.mozilla.org/r/63858/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63858/
Previously, when fetching an nsPresShell, we would look up the current realm
document and get the pres shell for it. This patch makes us call GetPresShell()
which uses GetRenderedDocument() which corresponds to the composed document of
the target effect which seems more consistent since it is the target effect we
will use as context for computing CSS values (as required by [1]).

[1] https://w3c.github.io/web-animations/#calculating-computed-keyframes

Review commit: https://reviewboard.mozilla.org/r/63860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63860/
The spec now defines that we should use the relevant Realm of the target
element for the created Animation and KeyframeEffect object.[1] As for the
AnimationEffectTiming object associated with the KeyframeEffect object,
the constructor for KeyframeEffect (or, actually the constructor for
KeyframeEffectReadOnly), specifies that current realm is used (which, at this
point corresponds to the relevant Realm of the target).[2]

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

Review commit: https://reviewboard.mozilla.org/r/63862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63862/
Comment on attachment 8770390 [details]
Bug 1277456 part 8 - Add test that when changing target element, the computed values are re-computed;

https://reviewboard.mozilla.org/r/63864/#review60894
Attachment #8770390 - Flags: review?(hiikezoe) → review+
Comment on attachment 8770383 [details]
Bug 1277456 part 1 - Store the parent document object of AnimationEffectReadOnly as an nsIDocument rather than nsISupports;

https://reviewboard.mozilla.org/r/63850/#review60890
Attachment #8770383 - Flags: review?(hiikezoe) → review+
Attachment #8770384 - Flags: review?(hiikezoe) → review+
Comment on attachment 8770384 [details]
Bug 1277456 part 2 - Store the parent document object of AnimationEffectTiming(ReadOnly) as nsIDocument instead of nsISupports;

https://reviewboard.mozilla.org/r/63852/#review60906
Comment on attachment 8770385 [details]
Bug 1277456 part 3 - Move set easing test to mozilla folder;

https://reviewboard.mozilla.org/r/63854/#review60908
Attachment #8770385 - Flags: review?(hiikezoe) → review+
Comment on attachment 8770387 [details]
Bug 1277456 part 5 - Pass the document associated with an AnimationEffectTiming/KeyframeEffect object to KeyframeUtils as the context to use when parsing CSS properties;

https://reviewboard.mozilla.org/r/63858/#review60910
Attachment #8770387 - Flags: review?(hiikezoe) → review+
Attachment #8770388 - Flags: review?(hiikezoe) → review+
Comment on attachment 8770388 [details]
Bug 1277456 part 6 - Use the composed document of the target effect (if any) when computing keyframe values;

https://reviewboard.mozilla.org/r/63860/#review60912
Comment on attachment 8770386 [details]
Bug 1277456 part 4 - Use the document associated with an AnimationEffectTiming object as the context for parsing easing rather than the current realm document;

https://reviewboard.mozilla.org/r/63856/#review61070

r=me
Attachment #8770386 - Flags: review?(bzbarsky) → review+
Comment on attachment 8770389 [details]
Bug 1277456 part 7 - Add tests for the prototypes of objects created using Animatable.animate();

https://reviewboard.mozilla.org/r/63862/#review61072

::: testing/web-platform/tests/web-animations/interfaces/Animatable/animate.html:30
(Diff revision 1)
> +  var div = createDiv(t, iframe.document);
> +  var anim = div.animate(null);
> +  assert_equals(Object.getPrototypeOf(anim), iframe.Animation.prototype,
> +                'The prototype of the created Animation is that defined on'
> +                + ' the relevant global for the target element');
> +  assert_not_equals(Object.getPrototypeOf(anim), Animation.prototype,

The current global in this case is the same as the relevant global.  What you probably wanted to do instead is:

    var anim = Element.prototype.animate.call(div, null);

::: testing/web-platform/tests/web-animations/interfaces/Animatable/animate.html:46
(Diff revision 1)
>  }, 'Element.animate() creates an Animation object with a KeyframeEffect');
>  
> +test(function(t) {
> +  var iframe = window.frames[0];
> +  var div = createDiv(t, iframe.document);
> +  var anim = div.animate(null);

Similar here.

::: testing/web-platform/tests/web-animations/interfaces/Animatable/animate.html:61
(Diff revision 1)
> +   + ' that is created in the relevant realm of the target element');
> +
> +test(function(t) {
> +  var iframe = window.frames[0];
> +  var div = createDiv(t, iframe.document);
> +  var anim = div.animate(null);

And here.

r=me with those fixed.
Attachment #8770389 - Flags: review?(bzbarsky) → review+
Comment on attachment 8770389 [details]
Bug 1277456 part 7 - Add tests for the prototypes of objects created using Animatable.animate();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63862/diff/1-2/
Comment on attachment 8770390 [details]
Bug 1277456 part 8 - Add test that when changing target element, the computed values are re-computed;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63864/diff/1-2/
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd6cf16dfb74
part 1 - Store the parent document object of AnimationEffectReadOnly as an nsIDocument rather than nsISupports; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5ccd87793088
part 2 - Store the parent document object of AnimationEffectTiming(ReadOnly) as nsIDocument instead of nsISupports; r=hiro
https://hg.mozilla.org/integration/autoland/rev/e85703fad4a6
part 3 - Move set easing test to mozilla folder; r=hiro
https://hg.mozilla.org/integration/autoland/rev/9b2815fda113
part 4 - Use the document associated with an AnimationEffectTiming object as the context for parsing easing rather than the current realm document; r=bz
https://hg.mozilla.org/integration/autoland/rev/b6dd3c6ec969
part 5 - Pass the document associated with an AnimationEffectTiming/KeyframeEffect object to KeyframeUtils as the context to use when parsing CSS properties; r=hiro
https://hg.mozilla.org/integration/autoland/rev/8cac50304a9a
part 6 - Use the composed document of the target effect (if any) when computing keyframe values; r=hiro
https://hg.mozilla.org/integration/autoland/rev/baf146b5a876
part 7 - Add tests for the prototypes of objects created using Animatable.animate(); r=bz
https://hg.mozilla.org/integration/autoland/rev/0df542fb3e35
part 8 - Add test that when changing target element, the computed values are re-computed; r=hiro
You need to log in before you can comment on or make changes to this bug.