Bug 1244590 implements the read-only spacing of KeyframeEffectReadOnly. However, we still need the writable one for KeyframeEffect, so we should add the new API, SetSpacing(), in KeyframeEffect.

I'm trying to figure out the meaning of the spec while setting an invalid spacing: 1) > If an attempt is made to set the spacing mode to a value that does not conform to the above grammar, > a TypeError must be thrown and the value of the spacing mode left unaffected. -- if the new spacing is not conformed to the grammar, we throw and early returns, so the original spacing is unaffected. 2) > If the paced property is not a property supported by the implementation, or is not animatable by the implementation, > the spacing mode must be set to distribute. A property is considered to be animatable if its animation type is anything > other than not animatable, or, for shorthand properties, if at least one of its component longhand properties has an > animation type other than not animatable. -- if the paced property is a invalid property or not a animatable property, should we set the original spacing to 'distribute', or just keep it unaffected? For SetSpacing() API, I prefer to keep the original spacing unaffected and dump the invalid property.

(In reply to Boris Chiou [:boris] from comment #1) > I'm trying to figure out the meaning of the spec while setting an invalid > spacing: > > 1) > > If an attempt is made to set the spacing mode to a value that does not conform to the above grammar, > > a TypeError must be thrown and the value of the spacing mode left unaffected. > -- if the new spacing is not conformed to the grammar, we throw and early > returns, so the original spacing is unaffected. > > 2) > > If the paced property is not a property supported by the implementation, or is not animatable by the implementation, > > the spacing mode must be set to distribute. A property is considered to be animatable if its animation type is anything > > other than not animatable, or, for shorthand properties, if at least one of its component longhand properties has an > > animation type other than not animatable. > -- if the paced property is a invalid property or not a animatable property, > should we set the original spacing to 'distribute', or just keep it > unaffected? For SetSpacing() API, I prefer to keep the original spacing > unaffected and dump the invalid property. I don't think we can leave it unaffected. If we do: effect.spacing = 'paced(transform)'; then: effect.spacing = 'paced(unsupported-property)'; I think we have the following options: a. Throw an exception and leave |spacing| as 'paced(transform)'. b. Leave |spacing| as 'paced(transform)' and ignore the change. c. Set |spacing| to 'paced(unsupported-property)' but internally apply distribute spacing. d. Set |spacing| to 'paced(unsupported-property)' but internally apply 'paced(transform)' spacing. e. Set |spacing| to 'distribute' and apply distribute spacing. I think you're suggesting we do (b) but the spec says we should do (e). I don't like (a) because it is likely to differ between browsers so we shouldn't throw exceptions for that. I think (b) is surprising -- if you can't change the value of something, there should be an exception/error. Otherwise it just seems to be broken. I would be ok with (c) -- it's similar to what we do with unsupported animation properties: we record them but we simply don't apply them. (A console warning would be helpful, I guess.) (d) just feels like a bug. Otherwise (e) is ok too -- at least, unlike (c), it makes the error detectable.

(In reply to Brian Birtles (:birtles) from comment #2) > I don't think we can leave it unaffected. > > If we do: > > effect.spacing = 'paced(transform)'; > > then: > > effect.spacing = 'paced(unsupported-property)'; > > I think we have the following options: > > a. Throw an exception and leave |spacing| as 'paced(transform)'. > b. Leave |spacing| as 'paced(transform)' and ignore the change. > c. Set |spacing| to 'paced(unsupported-property)' but internally apply > distribute spacing. > d. Set |spacing| to 'paced(unsupported-property)' but internally apply > 'paced(transform)' spacing. > e. Set |spacing| to 'distribute' and apply distribute spacing. > > I think you're suggesting we do (b) but the spec says we should do (e). > > I don't like (a) because it is likely to differ between browsers so we > shouldn't throw exceptions for that. > I think (b) is surprising -- if you can't change the value of something, > there should be an exception/error. Otherwise it just seems to be broken. > I would be ok with (c) -- it's similar to what we do with unsupported > animation properties: we record them but we simply don't apply them. (A > console warning would be helpful, I guess.) > (d) just feels like a bug. > Otherwise (e) is ok too -- at least, unlike (c), it makes the error > detectable. Thanks, Brian. I just rechecked what we do with unsupported animation properties from Animation constructor, and looks like we don't really record the unsupported animation property. We just report a warning (with the unsupported property) and apply 'distribute' spacing directly, so GetSpacing() will return 'distribute'. Therefore, between (c) and (e), I prefer (e) because it has the same behavior as what we do for applying spacing from Animation constructor (and we can also report a warning to console in SetSpacing()).

Comment on attachment 8787525 [details] Bug 1274944 - Part 2: Implement SetSpacing(). https://reviewboard.mozilla.org/r/76250/#review74338 Thanks for doing this. This is looking good but I don't understand the final comment (maybe because it's Friday afternoon :)) ::: dom/animation/KeyframeEffect.cpp:1415 (Diff revision 1) > + > + if (!invalidPacedProperty.IsEmpty()) { > + const char16_t* params[] = { invalidPacedProperty.get() }; > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > + NS_LITERAL_CSTRING("Animation"), > + GetRenderedDocument(), I think maybe this is not the right document. GetRenderedDocument corresponds to the document of the target element but according to [1] we should pass the document that triggered the error. I *think* AnimationUtils::GetCurrentRealmDocument might be what we want here. You'll need a global and I suspect you might be able to get that from a JSContext passed to the method[2] but you should check what other callers of this do. [1] https://dxr.mozilla.org/mozilla-central/rev/3ba5426a03b495b6417fffb872d42874edb80855/dom/base/nsContentUtils.h#866 [2] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#How_to_get_a_JSContext_passed_to_a_given_method ::: dom/animation/KeyframeEffect.cpp:1438 (Diff revision 1) > + // We might reuse the mFromValue/mToValue from mProperties to > + // re-construct nsTArray<ComputedKeyframeValue>, but building > + // animation segments may eliminate some zero length segments > + // which make the re-construction harder, so calling UpdateProperties() > + // directly here, and we don't need to maintain two contrary procedures: > + // a) nsTArray<ComputedKeyframeValue> -> AnimationProperty > + // b) AnimationProperty -> nsTArray<ComputedKeyframeValue> > + UpdateProperties(styleContext); I don't understand this comment. Can you explain? ::: dom/animation/KeyframeEffect.cpp:1446 (Diff revision 1) > + // which make the re-construction harder, so calling UpdateProperties() > + // directly here, and we don't need to maintain two contrary procedures: > + // a) nsTArray<ComputedKeyframeValue> -> AnimationProperty > + // b) AnimationProperty -> nsTArray<ComputedKeyframeValue> > + UpdateProperties(styleContext); > + MaybeUpdateFrameForCompositor(); Do we need the call to MaybeUpdateFrameForCompositor? I think that only needs to be called if the set of animated properties changes?

Comment on attachment 8787524 [details] Bug 1274944 - Part 1: Add writable spacing attribute. https://reviewboard.mozilla.org/r/76248/#review74346

Comment on attachment 8787525 [details] Bug 1274944 - Part 2: Implement SetSpacing(). https://reviewboard.mozilla.org/r/76250/#review74338 > I don't understand this comment. Can you explain? OK. I spent some time thinking about the question: Could we reuse the *StyleAnimationValue* from mProperties because UpdateProperties() will compute the StyleAnimationValue for each animation property per keyframe again [1]. However, if we want to reuse the StyleAnimationValue from mProperties, we have to try to convert an AnimationProperty object into a nsTArray<ComputedKeyframeValue> object, so we can use this reconstructed nsTArray<ComputedKeyframeValue> object to compute the paced spacing. But I notice that the implementaion of KeyframeUtils::BuildSegmentsFromValueEntries() will optimize this AnimationProperty object (i.e. eliminate some animation segments), which makes it harder to write an inversing function of BuildSegmentsFromValueEntries(). In addition, maintaining both BuildSegmentsFromValueEntries() and its inversing function at the same time may not be a good idea, so I prefer to call UpdateProperies() here to compute these StyleAnimationValues [1] and build a new Animation property object to replace the original one. Maybe I don't need to add this comment to let others confused. [1] http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/dom/animation/KeyframeEffect.cpp#255-258 > Do we need the call to MaybeUpdateFrameForCompositor? I think that only needs to be called if the set of animated properties changes? Actually, I'm not sure about this one. I just saw that we also add this after UpdateProperties() everywhere in KeyframeEffect(ReadOnly).cpp. As your comment, looks like we can remove it because we don't update animated properties. Thanks.

Comment on attachment 8787525 [details] Bug 1274944 - Part 2: Implement SetSpacing(). https://reviewboard.mozilla.org/r/76250/#review74338 > I think maybe this is not the right document. GetRenderedDocument corresponds to the document of the target element but according to [1] we should pass the document that triggered the error. > > I *think* AnimationUtils::GetCurrentRealmDocument might be what we want here. You'll need a global and I suspect you might be able to get that from a JSContext passed to the method[2] but you should check what other callers of this do. > > [1] https://dxr.mozilla.org/mozilla-central/rev/3ba5426a03b495b6417fffb872d42874edb80855/dom/base/nsContentUtils.h#866 > [2] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#How_to_get_a_JSContext_passed_to_a_given_method Thanks. I just tried it and can add an implicity JSContext successfully. I will update this by AnimationUtils::GetCurrentRealmDocument().

Comment on attachment 8787524 [details] Bug 1274944 - Part 1: Add writable spacing attribute. Sorry about bothering you again. I need you to check the change in dom/binding/Bindings.conf because I need an implicitly JSContext to the setter of KeyframeEffect.spacing. Thanks.

Comment on attachment 8787524 [details] Bug 1274944 - Part 1: Add writable spacing attribute. https://reviewboard.mozilla.org/r/76248/#review74400 fine, though I don't then quite understand the KeyframeEffect::SetSpacing implementation. Why just want and not throw an exception? Is that from the spec?

Comment on attachment 8787525 [details] Bug 1274944 - Part 2: Implement SetSpacing(). https://reviewboard.mozilla.org/r/76250/#review74650 I think this needs to do observer notification, right? Otherwise, it's looking good, I think.

(In reply to Brian Birtles (:birtles) from comment #16) > Comment on attachment 8787525 [details] > Bug 1274944 - Part 2: Implement SetSpacing(). > > https://reviewboard.mozilla.org/r/76250/#review74650 > > I think this needs to do observer notification, right? > > Otherwise, it's looking good, I think. Yes, I forgot it. Let me do observer notification now. :)

(In reply to Olli Pettay [:smaug] from comment #15) > Comment on attachment 8787524 [details] > Bug 1274944 - Part 1: Add writable spacing attribute. > > https://reviewboard.mozilla.org/r/76248/#review74400 > > fine, though I don't then quite understand the KeyframeEffect::SetSpacing > implementation. Why just want and not throw an exception? Is that from the > spec? My reason to use [SetterThrows] is: The input of KeyframeEffect::SetSpacing is a DOMString, and we will parse it according to the grammar mentioned in the spec. The spec says: "If an attempt is made to set the spacing mode to a value that does not conform to the above grammar, a TypeError must be thrown and the value of the spacing mode left unaffected." Therefore, I think we should pass an ErrorResult object into SetSpacing() and call something like "aRv.ThrowTypeError<dom::MSG_INVALID_SPACING_MODE_ERROR>(...)" if needed. This TypeError may be called in KeyframeEffectParams::ParseSpacing() [1]. Part 2 will call ParseSpacing() first and then doing other things, and Part 3 added some tests to test this TypeError. Hope this can answer your question. [1] http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/dom/animation/KeyframeEffectParams.cpp#134,145,159 BTW, the reason of adding "SetterOnly: [spacing]" in Bindings.conf is that we need the implicitly JSContext to get the current global document in SetSpacing(). Thank you.

(In reply to Boris Chiou [:boris] from comment #17) > (In reply to Brian Birtles (:birtles) from comment #16) > > Comment on attachment 8787525 [details] > > Bug 1274944 - Part 2: Implement SetSpacing(). > > > > https://reviewboard.mozilla.org/r/76250/#review74650 > > > > I think this needs to do observer notification, right? > > > > Otherwise, it's looking good, I think. > > Yes, I forgot it. Let me do observer notification now. :) I added an extra patch (part 4) for mutation observer because I think it could be an independent task. Thanks.

Comment on attachment 8787525 [details] Bug 1274944 - Part 2: Implement SetSpacing(). https://reviewboard.mozilla.org/r/76250/#review74734

Comment on attachment 8787526 [details] Bug 1274944 - Part 3: Test for setting spacing. https://reviewboard.mozilla.org/r/76252/#review74740 r=me with the following comments addressed ::: dom/animation/test/style/file_animation-setting-spacing.html:18 (Diff revision 3) > + const last = cumDist[cumDist.length - 1]; > + var offsets = cumDist.map( (curr) => { return curr / last; } ); > + > + var idx = 0; > + for (let i = 0; i < offsets.length - 1; ++i) { Nit: It's a bit odd to mix var with let/const here. Since this is a Gecko-only test it's ok to use let/const here. Remember that 'const' just means that you won't reassign the name--so I think you can use const for |offsets| and let for |idx| and |i|. ::: dom/animation/test/style/file_animation-setting-spacing.html:46 (Diff revision 3) > + var marginLeftValues = [0, -20, 100, 50]; > + var cumDist = [0, 20, 20 + 120, 20 + 120 + 50]; Could we pass the distances to calculateInterpolation and calculate the cumulative distances there? ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/spacing.html:34 (Diff revision 3) > +test(function(t) { > + var anim = createDiv(t).animate(null); > + anim.effect.spacing = 'paced(--bg-color)'; > + assert_equals(anim.effect.spacing, 'distribute', 'spacing mode'); > +}, 'Test falling back to distribute spacing if using CSS variables'); Is this right? I think custom properties will soon be animatable (based on jyc's work) so maybe this test is not right?

Comment on attachment 8788083 [details] Bug 1274944 - Part 4: Implement mutation observer for setting spacing. https://reviewboard.mozilla.org/r/76668/#review74746 r=me with the following changes addressed ::: dom/animation/KeyframeEffect.cpp:176 (Diff revision 1) > + > + nsAutoAnimationMutationBatch mb(mTarget->mElement->OwnerDoc()); > + if (mAnimation && mAnimation->IsRelevant()) { > + nsNodeUtils::AnimationChanged(mAnimation); > + } Nit: Can we move this before the call to UpdateProperties, just to match GetKeyframes? Also, do we need the nsAutoAnimationMutationBatch? I don't think there's anything else here that could trigger a mutation? So, could we just write: if (mAnimation && mAnimation->IsRelevant()) { nsNodeUtils::AnimationChanged(mAnimation); } if (mTarget) { ... ::: dom/animation/test/chrome/test_animation_observers.html:1913 (Diff revision 1) > + "records after animation is added"); > + > + anim.effect.spacing = "paced(margin-left)"; > + yield await_frame(); > + assert_records([{ added: [], changed: [anim], removed: [] }], > + "records after animation is changed"); Please add a test that: * Setting something like paced(animation-duration) triggers a change record (since we'll effectively update spacing to distribute) * Setting to the same value does *not* produce a change record It might be nice to clean up at the end too (although I'm not sure if it's really necessary): anim.cancel(); yield await_frame(); assert_records([{ added: [], changed: [], removed: [anim] }], "records after animation end");

One thing I am curious in this bug is about dom.animations-api.core.enabled. According to part 2 patch users can use this setter without any exceptions even if the pref is false but the actual value of the spacing remains unchanged value (i.e. distribute). Is it intentional?

Comment on attachment 8787526 [details] Bug 1274944 - Part 3: Test for setting spacing. https://reviewboard.mozilla.org/r/76252/#review74740 > Could we pass the distances to calculateInterpolation and calculate the cumulative distances there? Sure! > Is this right? I think custom properties will soon be animatable (based on jyc's work) so maybe this test is not right? I just checked bug 1273706, part 29: Implement animations for custom properties, and looks like it hasn't be merged into m-c, so I still pass this test locally. If this patch is landed before jyc's patch, I will keep this result, and hence I can pass the try. Otherwise, I will update the test. Thanks for the reminder.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > One thing I am curious in this bug is about dom.animations-api.core.enabled. > According to part 2 patch users can use this setter without any exceptions > even if the pref is false but the actual value of the spacing remains > unchanged value (i.e. distribute). Is it intentional? Yes. ParseSpacing always returns "distribute" if dom.animations-api.core.enabled is false. I intentionally let both "setting from keyframeEffect.spacing" and "setting from constructor" have the same behavior if dom.animations-api.core.enabled is true or false. Or should we only expose writable spacing when dom.animations-api.core.enabled is true? i.e. revise part 1, webidl file: [Pref="dom.animations-api.core.enabled"] inherit attribute DOMString spacing;

(In reply to Boris Chiou [:boris] from comment #29) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > > One thing I am curious in this bug is about dom.animations-api.core.enabled. > > According to part 2 patch users can use this setter without any exceptions > > even if the pref is false but the actual value of the spacing remains > > unchanged value (i.e. distribute). Is it intentional? > > Yes. ParseSpacing always returns "distribute" if > dom.animations-api.core.enabled is false. I intentionally let both "setting > from keyframeEffect.spacing" and "setting from constructor" have the same > behavior if dom.animations-api.core.enabled is true or false. Or should we > only expose writable spacing when dom.animations-api.core.enabled is true? I think it's fine as-is. If you can set spacing but it always ends up as distribute, that's fine. (We really just wanted to make sure that we don't accidentally ship 'spacing' as part of Element.animate.)

(In reply to Brian Birtles (:birtles) from comment #30) > (In reply to Boris Chiou [:boris] from comment #29) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #27) > > > One thing I am curious in this bug is about dom.animations-api.core.enabled. > > > According to part 2 patch users can use this setter without any exceptions > > > even if the pref is false but the actual value of the spacing remains > > > unchanged value (i.e. distribute). Is it intentional? > > > > Yes. ParseSpacing always returns "distribute" if > > dom.animations-api.core.enabled is false. I intentionally let both "setting > > from keyframeEffect.spacing" and "setting from constructor" have the same > > behavior if dom.animations-api.core.enabled is true or false. Or should we > > only expose writable spacing when dom.animations-api.core.enabled is true? > > I think it's fine as-is. If you can set spacing but it always ends up as > distribute, that's fine. (We really just wanted to make sure that we don't > accidentally ship 'spacing' as part of Element.animate.) Thank you both. We will follow the same rule in bug 1294651.

Comment on attachment 8788083 [details] Bug 1274944 - Part 4: Implement mutation observer for setting spacing. https://reviewboard.mozilla.org/r/76668/#review74746 > Please add a test that: > > * Setting something like paced(animation-duration) triggers a change record (since we'll effectively update spacing to distribute) > * Setting to the same value does *not* produce a change record > > It might be nice to clean up at the end too (although I'm not sure if it's really necessary): > > anim.cancel(); > yield await_frame(); > assert_records([{ added: [], changed: [], removed: [anim] }], > "records after animation end"); OK. I will add one test to set the spacing from "paced(margin-left)" to "paced(animation-duration)" and another one to set the same spacing.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e346ea721c4b Part 1: Add writable spacing attribute. r=smaug https://hg.mozilla.org/integration/autoland/rev/ff71bfb9e5a4 Part 2: Implement SetSpacing(). r=birtles https://hg.mozilla.org/integration/autoland/rev/dad5e450c9bf Part 3: Test for setting spacing. r=birtles https://hg.mozilla.org/integration/autoland/rev/65ca6f3dfc43 Part 4: Implement mutation observer for setting spacing. r=birtles

https://hg.mozilla.org/mozilla-central/rev/e346ea721c4b https://hg.mozilla.org/mozilla-central/rev/ff71bfb9e5a4 https://hg.mozilla.org/mozilla-central/rev/dad5e450c9bf https://hg.mozilla.org/mozilla-central/rev/65ca6f3dfc43