Background is summarized in the spec issue: https://github.com/w3c/web-animations/issues/180 I'm concerned that this feature might slow down our stylo work so we should drop this soon. The only trouble is Daisuke's current DevTools work (bug 1210796) is using paced timing so we need to create a JS implementation of distance calculation to use there.

Let's drop this now.

In this bug, I will try to do the following things: 1. Remove the wpt tests of paced timing 2. Remove its tests in dom/animation/tests 3. Create a JS implementation of distance calculation (for dev tool) 4. Remove the paced timing implementation, including the part of storing the invalid string.

I think we'll also need to update various tests that expect invalid properties to be stored.

(In reply to Brian Birtles (:birtles) from comment #0) > The only trouble is Daisuke's current DevTools work (bug 1210796) is using > paced timing so we need to create a JS implementation of distance > calculation to use there. I tried to find the part we use for computing distance in DevTools, and looks like we use ComputeAnimationDistance [1] (Bug 1339690 Comment 137) for now, which is already there for both gecko and servo backend. [1] http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/devtools/server/actors/animation.js#567-580

(In reply to Brian Birtles (:birtles) from comment #3) > I think we'll also need to update various tests that expect invalid > properties to be stored. The invalid property in "tests that expect invalid properties to be stored" is the property string store in KeyframeEffectParams, right? If so, I think these tests will be removed together with other paced timing tests. Do I misread this line?

The changes to the spec that dropped paced timing also dropped preserving invalid properties[1]. I assume that means that tests like the one for "Custom iterator with value list in keyframe should give bizarre string representation of list." in testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html will no longer pass? [1] https://github.com/w3c/web-animations/commit/c50c2d5009f9a5896706023ac06d05d894428484#diff-66070a1c4cce1f88da9dfe109fca9fddL7292

(In reply to Brian Birtles (:birtles) from comment #6) > The changes to the spec that dropped paced timing also dropped preserving > invalid properties[1]. I assume that means that tests like the one for > "Custom iterator with value list in keyframe should give bizarre string > representation of list." in > testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/ > processing-a-keyframes-argument.html will no longer pass? > > [1] > https://github.com/w3c/web-animations/commit/ > c50c2d5009f9a5896706023ac06d05d894428484#diff- > 66070a1c4cce1f88da9dfe109fca9fddL7292 Ya, ok. I just noticed that we have another bug for stop storing invalid keyframe values, Bug 1339693. Should I drop the preserving invalid properties in that bug?

(In reply to Boris Chiou [:boris] from comment #7) > (In reply to Brian Birtles (:birtles) from comment #6) > > The changes to the spec that dropped paced timing also dropped preserving > > invalid properties[1]. I assume that means that tests like the one for > > "Custom iterator with value list in keyframe should give bizarre string > > representation of list." in > > testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/ > > processing-a-keyframes-argument.html will no longer pass? > > > > [1] > > https://github.com/w3c/web-animations/commit/ > > c50c2d5009f9a5896706023ac06d05d894428484#diff- > > 66070a1c4cce1f88da9dfe109fca9fddL7292 > > Ya, ok. I just noticed that we have another bug for stop storing invalid > keyframe values, Bug 1339693. Should I drop the preserving invalid > properties in that bug? Ya, I can also do stop storing invalid value pair (i.e. discard the property-value pair) while parsing the property value [1] for MakePropertyValuePair() in this bug and tweak some tests. (And maybe mark duplicated on bug 1339693) [1] http://searchfox.org/mozilla-central/rev/61054508641ee76f9c49bcf7303ef3cfb6b410d2/dom/animation/KeyframeUtils.cpp#1069-1090

(In reply to Brian Birtles (:birtles) from comment #6) > I assume that means that tests like the one for > "Custom iterator with value list in keyframe should give bizarre string > representation of list." will no longer pass? Yes. After I stop storing the invalid property value, the test you mentioned, "Custom iterator with value list in keyframe ...", is failed.

Hi, Brian I have a question about how to implement the warning highlighting of the invalid property value [1]. If we discard the invalid property value, should we just call ReportToConsole for this invalid property value string because we no longer store this invalid property value? Thanks. [1] SPEC: If the property value is invalid according to the syntax for the property, discard the property-value pair. User agents that provide support for diagnosing errors in content SHOULD produce an appropriate warning highlighting the invalid property value.

(In reply to Boris Chiou [:boris] from comment #10) > Hi, Brian > > I have a question about how to implement the warning highlighting of the > invalid property value [1]. If we discard the invalid property value, should > we just call ReportToConsole for this invalid property value string because > we no longer store this invalid property value? Thanks. > > [1] SPEC: If the property value is invalid according to the syntax for the > property, discard the property-value pair. User agents that provide support > for diagnosing errors in content SHOULD produce an appropriate warning > highlighting the invalid property value. Yes, that sounds right to me. Sometimes we need to be careful not to spam the console, but I think this code only runs once when we setup the animation so maybe it is ok?

Comment on attachment 8877432 [details] Bug 1339690 - Part 1: Drop w-p-t tests of paced timing. https://reviewboard.mozilla.org/r/148788/#review153338

Comment on attachment 8877433 [details] Bug 1339690 - Part 2: Drop tests of paced timing in dom/animation/test. https://reviewboard.mozilla.org/r/148790/#review153340

Comment on attachment 8877434 [details] Bug 1339690 - Part 3: Drop spacing mode. https://reviewboard.mozilla.org/r/148792/#review153344 ::: dom/animation/KeyframeEffectReadOnly.cpp:220 (Diff revision 1) > mKeyframes = Move(aKeyframes); > - // Apply distribute spacing irrespective of the spacing mode. We will apply > - // the specified spacing mode when we generate computed animation property > - // values from the keyframes since both operations require a style context > - // and need to be performed whenever the style context changes. > KeyframeUtils::ApplyDistributeSpacing(mKeyframes); In a separate patch, perhaps we rename this to just DistributeKeyframes? ::: dom/animation/KeyframeUtils.h:95 (Diff revision 1) > GetComputedKeyframeValues(const nsTArray<Keyframe>& aKeyframes, > dom::Element* aElement, > const ServoComputedValues* aComputedValues); > > /** > - * Fills in the mComputedOffset member of each keyframe in the given array > + * Apply distribute spacing. Let's just say something like, "Calculate the computed offset of keyframes by evenly distributing keyframes with a missing offset. @see https://w3c.github.io/web-animations/#calculating-computed-keyframes @param aKeyframes..." And as mentioned above, rename this to DistributeKeyframes or ComputeKeyframeOffsets either in this patch or as a separate patch. ::: dom/animation/KeyframeUtils.h:108 (Diff revision 1) > * @param aComputedValues The computed keyframe values (as returned by > * GetComputedKeyframeValues) used to fill in the individual > * AnimationPropertySegment objects. Although these values could be > * calculated from |aKeyframes|, passing them in as a separate parameter > - * allows the result of GetComputedKeyframeValues to be re-used both > + * allows the result of GetComputedKeyframeValues to be re-used here. > - * here and in ApplySpacing. (I actually don't think this comment makes sense anymore. Now that we don't need to re-use the result of GetComputedKeyframeValues for spacing, it seems like this method could just call GetComputedKeyframeValues itself. Perhaps a simplification for a separate patch/bug?) ::: dom/animation/KeyframeUtils.cpp:410 (Diff revision 1) > static void > -DistributeRange(const Range<Keyframe>& aSpacingRange, > - const Range<Keyframe>& aRangeToAdjust); > - > -static void > DistributeRange(const Range<Keyframe>& aSpacingRange); Both of these function definitions still exist. It seems one is implemented in terms of the other but we only call one before it is defined. Looking at the two functions, it's probably better to merge them into one now (i.e. calculate the rangeToAdjust locally). Feel free to do that as a separate patch/bug however. ::: dom/animation/KeyframeUtils.cpp:1509 (Diff revision 1) > * computed offsets in (A, B). The second range, aRangeToAdjust, is passed, so > * we can know which keyframe we want to apply to. aRangeToAdjust should be in > * the range of aSpacingRange. > * > * @param aSpacingRange The sequence of keyframes between whose endpoints we > * should apply distribute spacing. should evenly distribute offsets.

Comment on attachment 8877434 [details] Bug 1339690 - Part 3: Drop spacing mode. https://reviewboard.mozilla.org/r/148792/#review153344 > In a separate patch, perhaps we rename this to just DistributeKeyframes? OK, I will rename this. Thanks. > (I actually don't think this comment makes sense anymore. Now that we don't need to re-use the result of GetComputedKeyframeValues for spacing, it seems like this method could just call GetComputedKeyframeValues itself. Perhaps a simplification for a separate patch/bug?) OK. I will update this comment together with the next patch. > Both of these function definitions still exist. It seems one is implemented in terms of the other but we only call one before it is defined. Looking at the two functions, it's probably better to merge them into one now (i.e. calculate the rangeToAdjust locally). Feel free to do that as a separate patch/bug however. Sure. I will do that. Thanks.

Comment on attachment 8877432 [details] Bug 1339690 - Part 1: Drop w-p-t tests of paced timing. https://reviewboard.mozilla.org/r/148788/#review153338 Forgot to remove the meta in this patch: "testing/web-platform/meta/web-animations/animation-model/animation-types/spacing-keyframes-shapes.html.ini" I will update this.

Comment on attachment 8877434 [details] Bug 1339690 - Part 3: Drop spacing mode. https://reviewboard.mozilla.org/r/148792/#review153450 r+ for .webidl change. Looks like this has been exposed only in Nightlies

Comment on attachment 8877434 [details] Bug 1339690 - Part 3: Drop spacing mode. https://reviewboard.mozilla.org/r/148792/#review153450 > Looks like this has been exposed only in Nightlies Yes. we have exposed spacing only in Nightlies.

(In reply to Boris Chiou [:boris] from comment #22) > > (I actually don't think this comment makes sense anymore. Now that we don't need to re-use the result of GetComputedKeyframeValues for spacing, it seems like this method could just call GetComputedKeyframeValues itself. Perhaps a simplification for a separate patch/bug?) > > OK. I will update this comment together with the next patch. Oh, I didn't get up to part 4 yesterday but I see you've already fixed it there! You can ignore that comment.

Comment on attachment 8877435 [details] Bug 1339690 - Part 6: Move GetComputedKeyframeValues into local static. https://reviewboard.mozilla.org/r/148794/#review153674

Comment on attachment 8877436 [details] Bug 1339690 - Part 7: Stop storing invalid property value. https://reviewboard.mozilla.org/r/148810/#review153684 Looks good but it seems like this could be simplified further so I'd like to check it once again. ::: dom/animation/KeyframeUtils.cpp:320 (Diff revision 1) > - > +#ifdef DEBUG > inline bool > IsInvalidValuePair(const PropertyValuePair& aPair, StyleBackendType aBackend) > { > if (aBackend == StyleBackendType::Servo) { > return !aPair.mServoDeclarationBlock; > } > > // There are three types of values we store as token streams: > // > // * Shorthand values (where we manually extract the token stream's string > // value) and pass that along to various parsing methods > // * Longhand values with variable references > // * Invalid values > // > // We can distinguish between the last two cases because for invalid values > // we leave the token stream's mPropertyID as eCSSProperty_UNKNOWN. > return !nsCSSProps::IsShorthand(aPair.mProperty) && > aPair.mValue.GetUnit() == eCSSUnit_TokenStream && > aPair.mValue.GetTokenStreamValue()->mPropertyID > == eCSSProperty_UNKNOWN; > } > +#endif I don't think this makes sense any more, right? How can we ever have invalid values now that we are not storing them? Can we just drop the default constructor for PropertyValuePair altogether? And in the Servo version of the constructor assert that the pointer is non-null? i.e. make it impossible to create an invalid value. ::: dom/animation/KeyframeUtils.cpp:712 (Diff revision 1) > MOZ_ASSERT(pair.mValues.Length() == 1); > - keyframe->mPropertyValues.AppendElement( > + > + Maybe<PropertyValuePair> valuePair = > MakePropertyValuePair(pair.mProperty, pair.mValues[0], parser, > - aDocument)); > + aDocument); > + if (valuePair.isNothing()) { I think you can just write: if (!valuePair) { ::: dom/animation/KeyframeUtils.cpp:868 (Diff revision 1) > * > * @param aProperty The CSS property. > * @param aStringValue The property value to parse. > * @param aParser The CSS parser object to use. > * @param aDocument The document to use when parsing. > - * @return The constructed PropertyValuePair object. > + * @return The constructed PropertyValuePair object, which could be nothing if Nit: "The constructed PropertyValuePair, or Nothing() if |aStringValue| is an invalid property value." (If you say "could be nothing" it sounds a little bit uncertain. If the property is invalid it *will* be nothing.) ::: dom/animation/KeyframeUtils.cpp:883 (Diff revision 1) > if (aDocument->GetStyleBackendType() == StyleBackendType::Servo) { > RefPtr<RawServoDeclarationBlock> servoDeclarationBlock = > KeyframeUtils::ParseProperty(aProperty, aStringValue, aDocument); > > if (servoDeclarationBlock) { > - result.mServoDeclarationBlock = servoDeclarationBlock.forget(); > + result.emplace(aProperty, Move(servoDeclarationBlock)); Do we also need to do: result->mSimulateComputeValuesFailure = false; here? Better still, we should just define mSimulateComputeValuesFailure with a default initializer that sets it to zero. i.e. bool mSimulateComputeValuesFailure = false; ::: dom/animation/KeyframeUtils.cpp (Diff revision 1) > - // We are about to convert a null value to a token stream value but > - // by leaving the mPropertyID as unknown, we will be able to > - // distinguish between invalid values and valid token stream values > - // (e.g. values with variable references). > - MOZ_ASSERT(tokenStream->mPropertyID == eCSSProperty_UNKNOWN, > - "The property of a token stream should be initialized" > - " to unknown"); > - I think maybe this assertion and comment still make sense? Perhaps we just need to replace 'invalid' with 'shorthand'? ::: dom/animation/KeyframeUtils.cpp:918 (Diff revision 1) > value.SetTokenStreamValue(tokenStream); > } > > - result.mValue = value; > - > + result.emplace(aProperty, Move(value)); > +#ifdef DEBUG > + (*result).mSimulateComputeValuesFailure = false; Nit: This can just be result->mSimulateComputeValuesFailure = false; But, as mentioned above, we should just initialize this to false by default and skip setting it here. ::: dom/animation/KeyframeUtils.cpp:1426 (Diff revision 1) > keyframe->mComputedOffset = offset; > } > - keyframe->mPropertyValues.AppendElement( > - MakePropertyValuePair(pair.mProperty, stringValue, parser, aDocument)); > + > + Maybe<PropertyValuePair> valuePair = > + MakePropertyValuePair(pair.mProperty, stringValue, parser, aDocument); > + if (valuePair.isNothing()) { if (!valuePair) { ::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument.html:263 (Diff revision 1) > {done: true}, > ])); > assert_frame_lists_equal(effect.getKeyframes(), [ > - {offset: null, computedOffset: 1, easing: 'linear', left: '100px,200px'} > + {offset: null, computedOffset: 1, easing: 'linear'} > ]); > }, 'Custom iterator with value list in keyframe should give bizarre string representation of list.'); This test description is no longer correct.

Comment on attachment 8877437 [details] Bug 1339690 - Part 8: Produce an appropriate warning highlighting the invalid property value. https://reviewboard.mozilla.org/r/148846/#review153702 Would you mind flagging someone from l10n for review too since they might be able to word-shop the message. Also, let's include the property name in the message. If you just see: Keyframe property value ‘0’ is invalid according to the syntax for the property. That's not very helpful. You want to know what property that was set on. ::: dom/animation/KeyframeUtils.cpp:862 (Diff revision 1) > return ConvertJSValueToString(aCx, aValue, dom::eStringify, dom::eStringify, > *aValues.AppendElement()); > } > > +static void > +ReportiInvalidPropertyValueToConsole(const nsAString& aInvalidPropertyValue, s/ReportiInvalid.../ReportInvalid.../ ::: dom/animation/KeyframeUtils.cpp:865 (Diff revision 1) > + const char16_t* params[] = { nullptr }; > + aInvalidPropertyValue.GetData(&params[0]); We can't just do: const char16_t* params[] = { aInvalidPropertyValue.get() }; ::: dom/animation/KeyframeUtils.cpp:872 (Diff revision 1) > + > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > + NS_LITERAL_CSTRING("Animation"), > + aDoc, > + nsContentUtils::eDOM_PROPERTIES, > + "InvalidPropertyValue", Can we call this InvalidKeyframePropertyValue instead?

Comment on attachment 8877437 [details] Bug 1339690 - Part 8: Produce an appropriate warning highlighting the invalid property value. https://reviewboard.mozilla.org/r/148846/#review153702 > We can't just do: > > const char16_t* params[] = { aInvalidPropertyValue.get() }; I tried that before, but it seems ```nsAString``` (because we pass nsAString into MakePropertyValuePair) doesn't have get() function, and so I use GetData() here. Or we can make MakePropertyValuePair also accept ```nsString```, so we can use get() function here.

Comment on attachment 8877437 [details] Bug 1339690 - Part 8: Produce an appropriate warning highlighting the invalid property value. https://reviewboard.mozilla.org/r/148846/#review153702 > I tried that before, but it seems ```nsAString``` (because we pass nsAString into MakePropertyValuePair) doesn't have get() function, and so I use GetData() here. > > Or we can make MakePropertyValuePair also accept ```nsString```, so we can use get() function here. nsString is guaranteed to be null terminated, so maybe it's better to use nsString here.

Comment on attachment 8877437 [details] Bug 1339690 - Part 8: Produce an appropriate warning highlighting the invalid property value. https://reviewboard.mozilla.org/r/148846/#review153702 > nsString is guaranteed to be null terminated, so maybe it's better to use nsString here. Or we just pass two non-null ```const char16_t*```s (one for property, and another one for property value).

(In reply to Boris Chiou [:boris] from comment #30) > Comment on attachment 8877437 [details] > Bug 1339690 - Part 6: Produce an appropriate warning highlighting the > invalid property value. > > https://reviewboard.mozilla.org/r/148846/#review153702 > > > We can't just do: > > > > const char16_t* params[] = { aInvalidPropertyValue.get() }; > > I tried that before, but it seems ```nsAString``` (because we pass nsAString > into MakePropertyValuePair) doesn't have get() function, and so I use > GetData() here. > > Or we can make MakePropertyValuePair also accept ```nsString```, so we can > use get() function here. Just use const nsString& invalidPropertyValue = PromiseFlatString(aInvalidPropertyValue); Then you can use invalidPropertyValue.get(). You can see some notes here: http://searchfox.org/mozilla-central/source/xpcom/string/nsTPromiseFlatString.h

Even better, a concrete example: http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/dom/base/nsContentUtils.cpp#7074

(In reply to Brian Birtles (:birtles) from comment #34) > Even better, a concrete example: > http://searchfox.org/mozilla-central/rev/ > c49a70b53f67dd5550eec8a08793805f2aca8d42/dom/base/nsContentUtils.cpp#7074 Thanks very much!

Comment on attachment 8877437 [details] Bug 1339690 - Part 8: Produce an appropriate warning highlighting the invalid property value. https://reviewboard.mozilla.org/r/148846/#review153814 ::: dom/locales/en-US/chrome/dom/dom.properties:353 (Diff revision 2) > # LOCALIZATION NOTE: Do not translate "<script>". > ScriptSourceMalformed=<script> source URI is malformed: “%S”. > # LOCALIZATION NOTE: Do not translate "<script>". > ScriptSourceNotAllowed=<script> source URI is not allowed in this document: “%S”. > +# LOCALIZATION NOTE: %1$S is the invalid property value and %2$S is the property name. > +InvalidKeyframePropertyValue=Keyframe property value ‘%1$S’ is invalid according to the syntax for '%2$S'. I would suggest to use ”” instead of ’’ (see the other strings in the file), but truth is that we're far from good at consistency.

Comment on attachment 8877437 [details] Bug 1339690 - Part 8: Produce an appropriate warning highlighting the invalid property value. https://reviewboard.mozilla.org/r/148846/#review153816 ::: dom/animation/KeyframeUtils.cpp:838 (Diff revision 2) > + const char16_t* params[] = { invalidValue.get(), > + NS_ConvertUTF8toUTF16( > + nsCSSProps::GetStringValue(aProperty)).get() }; That doesn't seem right. Won't the second pointer dangle in this case?

Comment on attachment 8877907 [details] Bug 1339690 - Part 4: Rename ApplyDistributeSpacing to DistributeKeyframes. https://reviewboard.mozilla.org/r/149316/#review153818 ::: dom/animation/KeyframeUtils.h:96 (Diff revision 1) > dom::Element* aElement, > const ServoComputedValues* aComputedValues); > > /** > * Calculate the computed offset of keyframes by evenly distributing keyframes > * with a missing offset Very minor nit: period (.) at end of sentence.

Comment on attachment 8877908 [details] Bug 1339690 - Part 5: Merge two DistributeRange functions. https://reviewboard.mozilla.org/r/149318/#review153820 ::: dom/animation/KeyframeUtils.cpp:1502 (Diff revision 1) > - * Evenly distribute the computed offsets in (A, B). > - * We pass the range keyframes in [A, B] and use A, B to calculate distributing > + * Apply distribute offsets to all keyframes in between the endpoints of the > + * given range. Distribute the offsets of all keyframes in between the endpoints of the given range. ::: dom/animation/KeyframeUtils.cpp:1505 (Diff revision 1) > - * @param aSpacingRange The sequence of keyframes between whose endpoints we > - * should apply distribute offsets. > + * @param aRange The sequence of keyframes between whose endpoints we should > + * apply distribute offsets. The sequence of keyframes between whose endpoints we should distribute offsets.

Comment on attachment 8877436 [details] Bug 1339690 - Part 7: Stop storing invalid property value. https://reviewboard.mozilla.org/r/148810/#review153824 ::: dom/animation/Keyframe.h:36 (Diff revision 2) > + RefPtr<RawServoDeclarationBlock>&& aValue) > + : mProperty(aProperty), mServoDeclarationBlock(Move(aValue)) > + { > + MOZ_ASSERT(mServoDeclarationBlock, "Should be valid property value"); > + } > + PropertyValuePair() = delete; Not sure this is needed? If we have any user-defined ctors then the compiler shouldn't generate a default ctor.

Comment on attachment 8877436 [details] Bug 1339690 - Part 7: Stop storing invalid property value. https://reviewboard.mozilla.org/r/148810/#review153824 > Not sure this is needed? If we have any user-defined ctors then the compiler shouldn't generate a default ctor. Actually, we can remove this line. :)

Comment on attachment 8877436 [details] Bug 1339690 - Part 7: Stop storing invalid property value. https://reviewboard.mozilla.org/r/148810/#review153872 ::: dom/animation/Keyframe.h:38 (Diff revision 3) > + { > + MOZ_ASSERT(mServoDeclarationBlock, "Should be valid property value"); > + } > + > nsCSSPropertyID mProperty; > - // The specified value for the property. For shorthand properties or invalid > + // The specified value for the property. For shorthand properties values, Nit: For shorthand property values

I landed some changes to KeyframeUtils.cpp earlier today so you'll probably need to rebase on top of autoland if you're going to land this tonight.

(In reply to Brian Birtles (:birtles) from comment #57) > I landed some changes to KeyframeUtils.cpp earlier today so you'll probably > need to rebase on top of autoland if you're going to land this tonight. (As I was writing that comment, they got merged to m-c so you only need to rebase on m-c.)

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7543c6d0a392 Part 1: Drop w-p-t tests of paced timing. r=birtles https://hg.mozilla.org/integration/autoland/rev/dfc4c1833d4d Part 2: Drop tests of paced timing in dom/animation/test. r=birtles https://hg.mozilla.org/integration/autoland/rev/1d93ac41c2e3 Part 3: Drop spacing mode. r=birtles,smaug https://hg.mozilla.org/integration/autoland/rev/884cc66df85e Part 4: Rename ApplyDistributeSpacing to DistributeKeyframes. r=birtles https://hg.mozilla.org/integration/autoland/rev/ec66458477b1 Part 5: Merge two DistributeRange functions. r=birtles https://hg.mozilla.org/integration/autoland/rev/ef7afa61eb6f Part 6: Move GetComputedKeyframeValues into local static. r=birtles https://hg.mozilla.org/integration/autoland/rev/8a0c89fee7aa Part 7: Stop storing invalid property value. r=birtles https://hg.mozilla.org/integration/autoland/rev/1d54ad88eaba Part 8: Produce an appropriate warning highlighting the invalid property value. r=birtles,flod

https://hg.mozilla.org/mozilla-central/rev/7543c6d0a392 https://hg.mozilla.org/mozilla-central/rev/dfc4c1833d4d https://hg.mozilla.org/mozilla-central/rev/1d93ac41c2e3 https://hg.mozilla.org/mozilla-central/rev/884cc66df85e https://hg.mozilla.org/mozilla-central/rev/ec66458477b1 https://hg.mozilla.org/mozilla-central/rev/ef7afa61eb6f https://hg.mozilla.org/mozilla-central/rev/8a0c89fee7aa https://hg.mozilla.org/mozilla-central/rev/1d54ad88eaba

According to https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/spacing this feature is not shipped yet, so no site-compat note needed.

We've released FF54. Mark 54 won't fix.

I've updated the following pages to remove any mention of the spacing property: https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffect https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffect/KeyframeEffect https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/KeyframeEffectReadOnly I've also deleted this page: https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/spacing And added a note to the Fx56 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/56#APIs_2 Does this work for you? I didn't think we needed to keep the specific page, as WAA is such a nascent implementation. Let me know if you think anything else needs to be updated.

Looks good. Although, again, we've never shipped spacing to the release channel so I'm not sure if we need it in the release notes. Thanks again!

(In reply to Brian Birtles (:birtles) from comment #72) > Looks good. Although, again, we've never shipped spacing to the release > channel so I'm not sure if we need it in the release notes. OK, point taken and mention removed from rel notes.