Closed
Bug 1277456
Opened 8 years ago
Closed 8 years ago
Fix uses of AnimationUtils::GetCurrentRealmDocument to use the appropriate document
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(8 files)
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
Assignee | ||
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bbirtles
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63852/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63852/
Assignee | ||
Comment 3•8 years ago
|
||
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/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63856/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63856/
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
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/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63864/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63864/
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8770384 -
Flags: review?(hiikezoe) → review+
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8770388 -
Flags: review?(hiikezoe) → review+
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Assignee | ||
Comment 17•8 years ago
|
||
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/
Assignee | ||
Comment 18•8 years ago
|
||
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/
Comment 19•8 years ago
|
||
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
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd6cf16dfb74 https://hg.mozilla.org/mozilla-central/rev/5ccd87793088 https://hg.mozilla.org/mozilla-central/rev/e85703fad4a6 https://hg.mozilla.org/mozilla-central/rev/9b2815fda113 https://hg.mozilla.org/mozilla-central/rev/b6dd3c6ec969 https://hg.mozilla.org/mozilla-central/rev/8cac50304a9a https://hg.mozilla.org/mozilla-central/rev/baf146b5a876 https://hg.mozilla.org/mozilla-central/rev/0df542fb3e35
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•