Closed Bug 1208951 Opened 4 years ago Closed 4 years ago

Implement KeyframeEffectReadOnly constructor

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

()

Details

Attachments

(11 files, 4 obsolete files)

4.68 KB, patch
birtles
: review+
Details | Diff | Splinter Review
3.72 KB, patch
birtles
: review+
Details | Diff | Splinter Review
1.63 KB, patch
birtles
: review+
Details | Diff | Splinter Review
5.95 KB, patch
birtles
: review+
Details | Diff | Splinter Review
5.79 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.54 KB, patch
birtles
: review+
Details | Diff | Splinter Review
10.33 KB, patch
birtles
: review+
Details | Diff | Splinter Review
1.32 KB, patch
birtles
: review+
Details | Diff | Splinter Review
49.96 KB, patch
birtles
: review+
Details | Diff | Splinter Review
35.86 KB, patch
birtles
: review+
Details | Diff | Splinter Review
18.46 KB, patch
birtles
: review+
Details | Diff | Splinter Review
This is piece 2 in bug 1198705 comment 0.

I think we may as well handle PropertyIndexedKeyframe and sequence<Keyframe> at the same time, so this will encompass piece 6 too.
Making this bug about implementing the constructor for KeyframeEffectReadOnly.  I've filed bug 1211783 for KeyframeEffect.
Summary: Implement KeyframeEffect constructor → Implement KeyframeEffectReadOnly constructor
In a subsequent patch, we will have another struct like
KeyframeValueEntry, but storing an StyleAnimationValue and an
ComputingTimingFunction object (not a pointer).  So we split
KeyframeValueEntry into two, retaining the KeyframeValueEntry name for
the base class and naming the current one KeyframeStringValueEntry.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8671216 - Flags: review?(bbirtles)
Attachment #8671216 - Attachment description: Bug 1208951 - Part 1: Split half of KeyframeValueEntry into a base class. → Part 1: Split half of KeyframeValueEntry into a base class.
Attachment #8671218 - Attachment description: Bug 1208951 - Part 2: Use a comparator object instead of operator< on KeyframeValueEntry. → Part 2: Use a comparator object instead of operator< on KeyframeValueEntry.
It is a slight hassle for code to have to create a Declaration object to
pass in to nsCSSParser::ParseProperty when all it wants is the resulting
single nsCSSValue, if parsing a longhand property.  Declaration is also
inaccessible outside layout/style/.
Attachment #8671220 - Flags: review?(bbirtles)
Since we will call StyleAnimationValue::ComputeValues with values that
come from script, we don't want to warn if we have an invalid value.
Attachment #8671227 - Flags: review?(bbirtles)
Sorry this one is so big; I don't think it makes sense to split up any further, though.  Start reading at KeyframeEffectReadOnly::Constructor.  I've taken the approach I mentioned in 1198708, where I convert a JS object to a Keyframe (or PropertyIndexedKeyframes) dictionary, and then iterate over its properties to find the open ended set.

r?bz for all the IDL/bindings/JSAPI stuff, and to sanity check my error handling.  r?birtles for the logic.
Attachment #8671229 - Flags: review?(bzbarsky)
Attachment #8671229 - Flags: review?(bbirtles)
Attached patch Part 10: Tests. (obsolete) — Splinter Review
Attachment #8671230 - Flags: review?(bbirtles)
Comment on attachment 8671223 [details] [diff] [review]
Part 5: Add nsCSSProps method to look a property by its IDL name.

r=me
Attachment #8671223 - Flags: review?(bzbarsky) → review+
Comment on attachment 8671229 [details] [diff] [review]
Part 9: Implement KeyframeEffectReadOnly constructor.

>+++ b/dom/animation/KeyframeEffect.cpp
>+KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
>+  aAnimationTiming.mFillMode = NS_STYLE_ANIMATION_FILL_MODE_NONE;

I assume we have a followup bug for doing the auto thing here?  Perhaps cite in comment?

>+struct OffsetIndexedKeyframe
>+  binding_detail::FastKeyframe mKeyframeDict;

I guess this avoids doing an unncessary Init() with null on the dictionary...  It's a bit unfortunate that we're leaking binding_detail stuff into here.  :(

>+struct AdditionalProperty
>+  jsid mID;

What keeps this alive?  jsids can be gced.  So you need to root these things somehow.

Probably simplest is to just store the nsCSSProperty and the _index_ of the jsid, and use the latter to get the jsid out of ids when you need it.  Otherwise, you get to write some custom rooting/tracing code.  That means you'll need an indexed loop, not a spiffy iterator one, over ids, but I think that's ok.

>+AppendValueAsString(JSContext* aCx,
>+                    nsTArray<nsString>& aValues,
>+                    JS::Handle<JS::Value> aValue)

Why does this need to do anything other than:

  return ConvertJSValueToString(aCx, aValue, eStringify, eStringify,
                                *aValues.AppendElement());

?

We could add an overload of ConvertJSValueToString that doesn't require you to pass those two StringificationBehavior arguments to make this shorter and eliminate the need for this function altogether...  We already have one other caller in nsGenericHTMLElement.cpp that could benefit from such a simpler overload.

Anyway, I'd much rather we use ConvertJSValueToString than add new codepaths that do the equivalent thing.

>+GetPropertyValuePairs(JSContext* aCx,
>+  for (jsid id : ids) {

So id is not rooted...

>+    if (!propName.init(aCx, id)) {

But this call can gc...

>+      p->mID = id;

So here id might be dead.  Or at least moved, in theory.  I assume the rooting analysis would have caught this; if it does not, please file a bug?

>+ConvertKeyframeSequence(JSContext* aCx,
>+      ThrowErrorMessage(aCx, MSG_NOT_OBJECT,
>+                        "Element of sequence<Keyframes> argument");

Yeah, it's too bad we can't include the name of the caller function and whatnot like the generated bindings can.  :(  This is why custom types are a bad idea, I tell you!

>+    if (!keyframe->mKeyframeDict.Init(aCx, value,
>+                                      "Element of sequence<Keyframes> argument",
>+                                      false)) {

Drop the last arg, please?  false is the default value.

>+HasValidOffsets(nsTArray<OffsetIndexedKeyframe>& aKeyframes)

It might have been nice to be able to throw different exceptions for "out of range" and "not ordered", but this is probably OK for now.

>+++ b/dom/webidl/KeyframeEffect.webidl
>+// 2. We use object? instead of
>+//
>+//    (PropertyIndexedKeyframes or sequence<Keyframe> or SharedKeyframeList)
>+//
>+//    for the second argument so that we can get the property-value pairs from
>+//    the PropertyIndexedKeyframes or Keyframe objects.  We also don't support
>+//    SharedKeyframeList yet.

OK, but we really do need to get the spec updated here.  The spec as it stands
is not exactly valid IDL, and it needs to get there somehow.

>+// 3. We use unrestricted double instead of (double or KeyframeEffectOptions)

"instead of (unrestricted double or KeyframeEffectOptions)", right?

>+ Constructor(Element? target,

Why does having a null target make sense?  I guess it might for a non-readonly KeyframeEffect?

>+             optional unrestricted double options)]

So it's not clear to me why the spec wants a union here anyway.  Is passing just the duration a common enough use case that passing { duration: whatever } is too much pain?  Because it seems to me like for non-legacy specs we'd typically just go with a dictionary here.

While looking at the spec, the "Otherwise (options is undefined)" case in the spec can't happen, since the argument has a default value of empty dictionary, which will cause it to be a "KeyframeEffectOptions".  Please raise a spec issue.

>+++ b/dom/webidl/PropertyIndexedKeyframes.webidl
>+  CompositeOperation? composite = null;

Why does this do the null thing?  If we need a value that means "use the value from somewhere else", then can't we just use the missing value (undefined) here instead?

r=me with the above fixed.
Attachment #8671229 - Flags: review?(bzbarsky) → review+
Attachment #8671216 - Flags: review?(bbirtles) → review+
Attachment #8671218 - Flags: review?(bbirtles) → review+
Attachment #8671219 - Flags: review?(bbirtles) → review+
Attachment #8671220 - Flags: review?(bbirtles) → review+
Attachment #8671224 - Flags: review?(bbirtles) → review+
Comment on attachment 8671226 [details] [diff] [review]
Part 7: Add a StyleAnimationValue::ComputeValues method to compute components of a shorthand.

>diff --git a/layout/style/StyleAnimationValue.cpp b/layout/style/StyleAnimationValue.cpp
>+/* static */ bool
>+StyleAnimationValue::ComputeValues(nsCSSProperty aProperty,
>+                                   nsCSSProps::EnabledState aEnabledState,
>+                                   dom::Element* aTargetElement,
>+                                   const nsAString& aSpecifiedValue,
>+                                   bool aUseSVGMode,
>+                                   nsTArray<PropertyStyleAnimationValuePair>& aResult)
>+{
>+  MOZ_ASSERT(aTargetElement, "null target element");
>+  MOZ_ASSERT(aTargetElement->GetCurrentDoc(),
>+             "we should only be able to actively animate nodes that "
>+             "are in a document");
>+
>+  // Parse specified value into a temporary css::StyleRule
>+  nsRefPtr<css::StyleRule> styleRule =
>+    BuildStyleRule(aProperty, aTargetElement, aSpecifiedValue, aUseSVGMode);
>+  if (!styleRule) {
>+    return false;
>+  }
>+
>+  aResult.Clear();
>+  return ComputeValues(aProperty, aEnabledState, aTargetElement, styleRule,
>+                       aResult, /* aIsContextSensitive */ nullptr);

Nit: In the case where we return false, should we still clear aResult?

>-  // Extract computed value of our property from the temporary style rule
>-  return ExtractComputedValue(aProperty, tmpStyleContext, aComputedValue);
>+  // Extract computed value of our property (or all longhand components, if
>+  // aProperty is a shorthand) from the temporary style rule
>+  if (nsCSSProps::IsShorthand(aProperty)) {
>+    CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aProperty, aEnabledState) {
>+      PropertyStyleAnimationValuePair* pair = aValues.AppendElement();
>+      pair->mProperty = *p;
>+      if (!ExtractComputedValue(*p, tmpStyleContext,
>+                                pair->mValue)) {
>+        return false;
>+      }
>+    }
>+    return true;
>+  } else {
>+    PropertyStyleAnimationValuePair* pair = aValues.AppendElement();
>+    pair->mProperty = aProperty;
>+    return ExtractComputedValue(aProperty, tmpStyleContext, pair->mValue);
>+  }

Actually, we have a similar situation here, I guess, where if there is an error, we will return a partially-filled aValues. So we should either pass in a temp array and swap it on success, or just document that the array shouldn't be used if the function returns false.

>diff --git a/layout/style/StyleAnimationValue.h b/layout/style/StyleAnimationValue.h
>@@ -149,16 +155,31 @@ public:
>   static bool ComputeValue(nsCSSProperty aProperty,
>                              mozilla::dom::Element* aTargetElement,
>                              const nsAString& aSpecifiedValue,
>                              bool aUseSVGMode,
>                              StyleAnimationValue& aComputedValue,
>                              bool* aIsContextSensitive = nullptr);
> 
>   /**
>+   * Like ComputeValue, but returns an array of StyleAnimationValues.
>+   *
>+   * On success, when aProperty is a longhand, aResult will have a single
>+   * value in it.  When aProperty is a shorthand, aResult will be filled with
>+   * values for all of aProperty's longhand components that match the
>+   * specified aEnabledState.
>+   */
>+  static bool ComputeValues(nsCSSProperty aProperty,
>+                            nsCSSProps::EnabledState aEnabledState,
>+                            mozilla::dom::Element* aTargetElement,
>+                            const nsAString& aSpecifiedValue,
>+                            bool aUseSVGMode,
>+                            nsTArray<PropertyStyleAnimationValuePair>& aResult);

I think we need to describe aEnabledState since it's not present in ComputeValue so the comment there doesn't cover it.
Attachment #8671226 - Flags: review?(bbirtles) → review+
Attachment #8671227 - Flags: review?(bbirtles) → review+
I've nearly finished part 9 but I'll have to come back to it tomorrow. I'll follow up Boris' comments then as well and hopefully make some of the changes to the spec at the same time. So far, looks good though.
Comment on attachment 8671229 [details] [diff] [review]
Part 9: Implement KeyframeEffectReadOnly constructor.

>diff --git a/dom/animation/KeyframeEffect.cpp b/dom/animation/KeyframeEffect.cpp
>+/* static */ void
>+KeyframeEffectReadOnly::ConvertKeyframeEffectOptions(
>+    const Optional<double>& aOptions,
>+    AnimationTiming& aAnimationTiming)
>+{
>+  // We don't support auto durations yet, so we set it to 0 if a duration
>+  // wasn't specified.
>+  if (aOptions.WasPassed()) {
>+    aAnimationTiming.mIterationDuration =
>+      TimeDuration::FromMilliseconds(aOptions.Value());
>+  } else {
>+    aAnimationTiming.mIterationDuration = TimeDuration();

Nit: You can construct a TimeDuration passing zero as the argument to improve
readibility.

>+/**
>+ * A property-value pair obtained from the open-ended properties
>+ * discovered on a Keyframe or PropertyIndexedKeyframes object.
>+ *
>+ * Single values (as required by Keyframe, and as also supported
>+ * on PropertyIndexedKeyframes) are stored as the only element in
>+ * mValues.
>+ */
>+struct PropertyValuePair
>+{
>+  nsCSSProperty mProperty;
>+  nsTArray<nsString> mValues;
>+};

Does PropertyValuesPair make more sense? PropertyAndValues?

>+/**
>+ * Parses a CSS <single-transition-timing-function> value from
>+ * aEasing into aResult.  If parsing fails, aResult will be
>+ * to 'linear'.
>+ */

Would "into a ComputedTimingFunction" be more clear than "into aResult"?

Also, there's something funny with the grammar in the second sentence. Missing
'set'?

>+static void
>+ParseEasing(Element* aTarget,
>+            const nsAString& aEasing,
>+            ComputedTimingFunction& aResult)
>+{
>+  nsIDocument* doc = aTarget->GetOwnerDocument();

I think this should be aTarget->OwnerDoc()?

>+/**
>+ * Converts aValue to DOMString, if aAllowLists is eDisallow, or
>+ * to (DOMString or sequence<DOMString>) if aAllowLists is aAllow.
>+ * The resulting strings are appended to aValues.
>+ */
>+static bool
>+AppendStringOrStringSequence(JSContext* aCx,
>+                             nsTArray<nsString>& aValues,
>+                             ListAllowance aAllowLists,
>+                             JS::Handle<JS::Value> aValue)
>+{

So aValue is an input parameter and aValues is an output parameter?

I would have expected the input parameter to come first, and I wonder
if there is any naming that would make this more clear?

>+/**
>+ * Fills in any null offsets for the given keyframes by applying the
>+ * "distribute" spacing algorithm.
>+ *
>+ * http://w3c.github.io/web-animations/#distribute-keyframe-spacing-mode
>+ */
>+static void
>+ApplyDistributeSpacing(nsTArray<OffsetIndexedKeyframe>& aKeyframes)
>+{
>+  // If the first or last keyframes have an unspecified offset,
>+  // fill them in with 0% and 100%.  If there is only a single keyframe,
>+  // then it gets 100%.
>+  if (aKeyframes.LastElement().mKeyframeDict.mOffset.IsNull()) {
>+    aKeyframes.LastElement().mKeyframeDict.mOffset.SetValue(1.0);
>+  }
>+  if (aKeyframes[0].mKeyframeDict.mOffset.IsNull()) {
>+    aKeyframes[0].mKeyframeDict.mOffset.SetValue(0.0);
>+  }
>+
>+  // Fill in remaining missing offsets.
>+  size_t i = 0;
>+  while (i < aKeyframes.Length() - 1) {
>+    MOZ_ASSERT(!aKeyframes[i].mKeyframeDict.mOffset.IsNull());
>+    double from = aKeyframes[i].mKeyframeDict.mOffset.Value();
>+    size_t j = i + 1;
>+    while (aKeyframes[j].mKeyframeDict.mOffset.IsNull()) {
>+      ++j;
>+    }
>+    double to = aKeyframes[j].mKeyframeDict.mOffset.Value();
>+    double portion = 1.0 / (j - i);
>+    for (size_t k = 1; k < j - i; ++k) {
>+      double offset = from + k * portion * (to - from);
>+      aKeyframes[i + k].mKeyframeDict.mOffset.SetValue(offset);
>+    }
>+    i = j;
>+  }

It might be worthwhile to use the same names as the spec here?

  https://w3c.github.io/web-animations/#evenly-distributing-a-keyframe

i.e. s/from/start/, s/to/end/, instead of portion, n = (j-i) and we divide by
it, rather than multiplying by it (and then we can use it in the for-loop
condition too).

>+static void
>+GenerateValueEntries(Element* aTarget,
>+                     nsTArray<OffsetIndexedKeyframe>& aKeyframes,
>+                     nsTArray<KeyframeStyleAnimationValueEntry>& aResult,
>+                     ErrorResult& aRv)

Does aKeyframes need to be mutable here? This applies to a few functions in
this file. If it's possible to make the inputs const it would be easier to
distinguish them. It may be, however, that we're using some non-const methods
on them, in which case it's fine as-is.

>+/**
>+ * Builds an array of AnimationProperty objects to represent the keyframe
>+ * animation segments in aEntries.
>+ */
>+static void
>+BuildSegmentsFromValueEntries(
>+    nsTArray<KeyframeStyleAnimationValueEntry>& aEntries,
>+    nsTArray<AnimationProperty>& aResult)
>+{
>+  // Sort the KeyframeStyleAnimationValueEntry objects so that all entries
>+  // for a given property are together, and the entries are sorted by
>+  // offset otherwise.
>+  aEntries.Sort(KeyframeValueEntry::PropertyOffsetComparator());
>+
>+  nsCSSProperty lastProperty = eCSSProperty_UNKNOWN;
>+  AnimationProperty* animationProperty = nullptr;
>+  for (size_t i = 0; i < aEntries.Length(); ++i) {
>+    KeyframeStyleAnimationValueEntry& thisEntry = aEntries[i];
>+    if (thisEntry.mOffset != 1.0f) {
>+      while (thisEntry.mOffset == aEntries[i + 1].mOffset &&
>+             thisEntry.mProperty == aEntries[i + 1].mProperty) {
>+        ++i;
>+      }

What stops this from reading past the end of aEntries? Is it the fact that we
know that the last offset is guaranteed to be 1.0 so therefore the equality
test will fail (since thisEntry.mOffset != 1.0f)? If so, we should probably
add a comment/assertion to that effect.

>+      KeyframeStyleAnimationValueEntry& nextEntry = aEntries[i + 1];
>+      if (thisEntry.mOffset != nextEntry.mOffset ||
>+          thisEntry.mProperty != nextEntry.mProperty) {

I don't quite understand this part. When will the condition be false?
It seems like we keep incrementing |i| until it refers to the last entry with
the same offset property. Then we assign the *next* entry after that. So
why would thisEntry and nextEntry not differ here?

Also, this doesn't seem to line up with the spec as far as I understand it.

According to the spec, if you have:

  { left: '100px', offset: 0 },
  { left: '200px', offset: 0 },
  { left: '300px', offset: 1 },
  { left: '400px', offset: 1 }

You should end using 100px when you backwards fill, and 400px when you
forwards fill, but interpolate between 200px and 300px.

That comes from: https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect

So, I think we should end up creating three segments in this case (offset
0->0, 0->1, 1->1). I don't see how the current code achieves this, however.

Similarly, for the following case:

  { left: '100px', offset: 0 },
  { left: '200px', offset: 0.5 },
  { left: '300px', offset: 0.5 },
  { left: '400px', offset: 0.5 },
  { left: '500px', offset: 1.0 }

We should create a segments like:

  left:
     { mFromKey: 0, mToKey: 0.5, mFrom: '100px', mTo: '200px' },
     { mFromKey: 0.5, mToKey: 1.0, mFrom: '400px', mTo: '500px' }

>diff --git a/dom/animation/KeyframeEffect.h b/dom/animation/KeyframeEffect.h
>index 2e09a2a..2d4c1ef 100644
>--- a/dom/animation/KeyframeEffect.h
>+++ b/dom/animation/KeyframeEffect.h
> protected:
>   virtual ~KeyframeEffectReadOnly();
>   void ResetIsRunningOnCompositor();
> 
>+  static void ConvertKeyframeEffectOptions(const Optional<double>& aOptions,
>+                                           AnimationTiming& aAnimationTiming);

Any reason not to just make this return a AnimationTiming object as its return
value?

>diff --git a/dom/webidl/KeyframeEffect.webidl b/dom/webidl/KeyframeEffect.webidl
>+// For the constructor:
>+//
>+// 1. We use Element? for the first argument since we don't support Animatable
>+//    for pseudo-elements yet.
>+//
>+// 2. We use object? instead of
>+//
>+//    (PropertyIndexedKeyframes or sequence<Keyframe> or SharedKeyframeList)
>+//
>+//    for the second argument so that we can get the property-value pairs from
>+//    the PropertyIndexedKeyframes or Keyframe objects.  We also don't support
>+//    SharedKeyframeList yet.
>+//
>+// 3. We use unrestricted double instead of (double or KeyframeEffectOptions)
>+//    since we don't support KeyframeEffectOptions yet.

Why an unrestricted double? Infinity is not a valid iteration duration so we
shouldn't allow it.

>diff --git a/dom/webidl/PropertyIndexedKeyframes.webidl b/dom/webidl/PropertyIndexedKeyframes.webidl
>+dictionary PropertyIndexedKeyframes {
>+  DOMString           easing = "linear";
>+  CompositeOperation? composite = null;
>+};

Since we don't actually use this in KeyframeEffect.webidl, we should probably
add a comment about why we have this at all. Maybe that will be an easier
comment to write once I update the spec, however.


It looks fine (great, actually) with those comments addressed, particularly
the questions about BuildSegmentsFromValueEntries and the unrestricted double.
Attachment #8671229 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #18)
> Similarly, for the following case:
> 
>   { left: '100px', offset: 0 },
>   { left: '200px', offset: 0.5 },
>   { left: '300px', offset: 0.5 },
>   { left: '400px', offset: 0.5 },
>   { left: '500px', offset: 1.0 }
> 
> We should create a segments like:
> 
>   left:
>      { mFromKey: 0, mToKey: 0.5, mFrom: '100px', mTo: '200px' },
>      { mFromKey: 0.5, mToKey: 1.0, mFrom: '400px', mTo: '500px' }

I think I was confusing the visual result from what keyframes get created here.

For input:

  { left: '100px', offset: 0 },
  { left: '200px', offset: 0.5 },
  { left: '300px', offset: 0.5 },
  { left: '400px', offset: 0.5 },
  { left: '500px', offset: 1.0 }

We should return the same keyframes from getFrames(). That suggests internally we will need to store:

  { mFromKey: 0, mToKey: 0.5, mFrom: '100px', mTo: '200px' },
  { mFromKey: 0.5, mToKey: 0.5, mFrom: '200px', mTo: '300px' },
  { mFromKey: 0.5, mToKey: 0.5, mFrom: '300px', mTo: '400px' },
  { mFromKey: 0.5, mToKey: 1.0, mFrom: '400px', mTo: '500px' }
Comment on attachment 8671230 [details] [diff] [review]
Part 10: Tests.

>diff --git a/testing/web-platform/meta/web-animations/keyframe-effect/constructor.html.ini b/testing/web-platform/meta/web-animations/keyframe-effect/constructor.html.ini
>+[constructor.html]
>+  type: testharness
>+  [easing values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeTimingOptions]
>+    expected: FAIL
>+  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in PropertyIndexedKeyframes]
>+    expected: FAIL
>+  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in Keyframe]
>+    expected: FAIL
>+  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeTimingOptions]
>+    expected: FAIL
>+  [a KeyframeEffectReadOnly can be constructed with a Keyframe sequence with different composite values, but the same composite value for a given offset]
>+    expected: FAIL

Should we annotate these with bug numbers?

I think we need the following bugs:

* Implement animation-level easing
* Implement animation composition
* Implement additive animation (not yet tested as mentioned in the TODO)

>diff --git a/testing/web-platform/tests/web-animations/keyframe-effect/constructor.html b/testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
>+var gEmptyKeyframeListTests = [
>+  [],
>+  [{}],
>+  [{ easing: "ease-in" }],
>+  [{ unknown: "unknown" }, { unknown: "unknown" }],
>+  [{ color: "invalid" }, { color: "invalid" }],
>+  { easing: "ease-in" },
>+  { unknown: "unknown" },
>+  { unknown: [] },
>+  { unknown: ["unknown"] },
>+  { unknown: ["unknown", "unknown"] },
>+  { animationName: ["none", "abc"] },
>+  { color: [] },
>+  null,
>+  undefined,
>+];

I wonder if it is ok to use the Mozilla naming convention for global variables
in web-platform-tests?

>+var gPropertyIndexedKeyframesTests = [
>+  { desc:   "a one property two value PropertyIndexedKeyframes specification",
>+    input:  { left: ["10px", "20px"] },
>+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", left: "10px" },

Is it possible to wrap this section to 80 characters?

>+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", left: "20px" }] },
>+  { desc:   "a one shorthand property two value PropertyIndexedKeyframes specification",
>+    input:  { margin: ["10px", "10px 20px 30px 40px"] },
>+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", marginTop: "10px", marginRight: "10px", marginBottom: "10px", marginLeft: "10px" },
>+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", marginTop: "10px", marginRight: "20px", marginBottom: "30px", marginLeft: "40px" }] },

Just to check my recollection here, we dicussed making sure longhands always
override shorthands. Most of the time that happens to work since we apply
properties in lexographical order by property name. However, for some
properties like border-*-width that isn't the case. We decided to address that
in follow-up bug. Is that right?

>+  { desc:   "a PropertyIndexedKeyframes specification using a camel-cased property's IDL name",
>+    input:  { marginLeft: ["10px", "20px"],
>+              "margin-top": ["30px", "40px"] },
>+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", marginLeft: "10px" },
>+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", marginLeft: "20px" }] },
>+  { desc:   "a PropertyIndexedKeyframes specification using a camel-cased property's IDL name",

I believe the Chrome guys mentioned wanting to support "margin-top" like this
at one point. Can you check if Chrome supports this?

>+  { desc:   "a Keyframe sequence with duplicate values for a given offset",
>+    input:  [{ offset: 0, left: "10px" },
>+             { offset: 0, left: "20px" },
>+             { offset: 1, left: "30px" },
>+             { offset: 1, left: "40px" }],
>+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", left: "10px" },
>+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", left: "30px" }] },

For this case, all four keyframes should be preserved, I believe. This is
actually useful for providing different fill values.


A few other tests I think we should add here include:

* The test case from comment 19

* It might be worth doing a test that we can take the output of getFrames
  and feed it back into the constructor and get the same result again?

* I think we should test that the following throws:

  new KeyframeEffectReadOnly(target,
    [ { left: [ "10px", "30px" ] }, { left: "50px" } ])

* And I think we need tests for stringification, e.g.

    new KeyframeEffectReadOnly(target, { opacity: [ 0, 1 ] });
    new KeyframeEffectReadOnly(target, [ { opacity: 0 }, { opacity: 1 } ]);

  I think that one is important.

>+// TODO: add tests for the following, once the spec has been clarified:
>+//
>+//   * a single value (either as a string or an array of one string)
>+//     in a PropertyIndexedKeyframes, which should be an animation with the
>+//     first segment having an additive zero value
>+//   * the first or last value being invalid in a PropertyIndexedKeyframes,
>+//     which should be an animation with the first value of the first segment
>+//     or the second value of the last segment having an additive zero value
>+//   * a sequence<Keyframe> with missing properties on the 0% or 100%
>+//     keyframe, which should be an animation with those properties having
>+//     an additive zero value

Is it worth testing these three and adding a fails annotations? Seeing as we
expect to ship before implementing addition, it seems like a good idea to
exercise this code to ensure it doesn't trigger anything nasty.

>+//   * the easing value exposed on the final Keyframe object returned by
>+//     getFrames()
>+//   * a sequence<Keyframe> that uses different easing/composite values for
>+//     different offsets, which tests that we generate a minimal set of
>+//     keyframes and in a specific order
>+//   * tests for parsing easing values using perhaps-unexpected CSS syntax,
>+//     such as "Ease\\2d in-out" and "linear /**/"
>+//   * setting border-color (a shorthand) and border-bottom-color on the
>+//     one keyframe to test whether shorthands are always overridden by
>+//     their longhands, or whether it is done purely based on CSS property
>+//     IDL name order

Do you plan on adding any of these before landing?

In particular, it would be good to at least have follow-up bugs for the last
two here.


What's there looks great except for that one about overlapping keyframes which
relates back to part 9.
Attachment #8671230 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #18)
> >+// 3. We use unrestricted double instead of (double or KeyframeEffectOptions)
> >+//    since we don't support KeyframeEffectOptions yet.
> 
> Why an unrestricted double? Infinity is not a valid iteration duration so we
> shouldn't allow it.

Hmm, I was wrong. I we do allow infinity here and have for a long time. I'll fix the spec to use unrestricted double in the union here.
(In reply to Brian Birtles (:birtles) from comment #21)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > >+// 3. We use unrestricted double instead of (double or KeyframeEffectOptions)
> > >+//    since we don't support KeyframeEffectOptions yet.
> > 
> > Why an unrestricted double? Infinity is not a valid iteration duration so we
> > shouldn't allow it.
> 
> Hmm, I was wrong. I we do allow infinity here and have for a long time. I'll
> fix the spec to use unrestricted double in the union here.

It looks like the spec already uses unrestricted double, but was copied incorrectly into the WebIDL comment. However, I noticed the Animatable interface mentions double instead of unrestricted double so I've fixed that in the spec.
(In reply to Boris Zbarsky [:bz] from comment #15)
> >+++ b/dom/webidl/KeyframeEffect.webidl
> >+// 2. We use object? instead of
> >+//
> >+//    (PropertyIndexedKeyframes or sequence<Keyframe> or SharedKeyframeList)
> >+//
> >+//    for the second argument so that we can get the property-value pairs from
> >+//    the PropertyIndexedKeyframes or Keyframe objects.  We also don't support
> >+//    SharedKeyframeList yet.
> 
> OK, but we really do need to get the spec updated here.  The spec as it
> stands
> is not exactly valid IDL, and it needs to get there somehow.

I'd like to fix the spec while you are both available and have this in your
minds so I started to outline the changes, as I understand them, here:

  https://github.com/w3c/web-animations/issues/107#issuecomment-147963257

Please comment when you have a moment.

> >+// 3. We use unrestricted double instead of (double or KeyframeEffectOptions)
> 
> "instead of (unrestricted double or KeyframeEffectOptions)", right?
> 
> >+ Constructor(Element? target,
> 
> Why does having a null target make sense?  I guess it might for a
> non-readonly KeyframeEffect?

Right. One case we were anticipating was that in a subsequent level we'd add
an onsample callback to effects. An author could clear the target, then use
onsample to get the current timing parameters of the animation (especially the
current progress value) to update a <canvas>, for example. It basically lets
you use the timing of an effect without needing to point it at something to
animate.

> >+             optional unrestricted double options)]
> 
> So it's not clear to me why the spec wants a union here anyway.  Is passing
> just the duration a common enough use case that passing { duration: whatever
> } is too much pain?  Because it seems to me like for non-legacy specs we'd
> typically just go with a dictionary here.

Yes, it's quite common.

jQuery: animate( properties [, duration ] [, easing ] [, complete ] )
        animate( properties, options )[1]
        i.e. you can do either animate(..., 400) or
                               animate(..., { duration: 400} )
velocity: Same API as jQuery[2]
snap.svg: animate(attrs, duration, [easing], [callback])[3]
Greensock: to(target, duration, props)[4]

[1] http://api.jquery.com/animate/
[2] http://julian.com/research/velocity/#arguments
[3] http://snapsvg.io/docs/#Element.animate
[4] https://greensock.com/docs/#/HTML5/GSAP/TimelineLite/to/


> While looking at the spec, the "Otherwise (options is undefined)" case in
> the spec can't happen, since the argument has a default value of empty
> dictionary, which will cause it to be a "KeyframeEffectOptions".  Please
> raise a spec issue.
> 
> >+++ b/dom/webidl/PropertyIndexedKeyframes.webidl
> >+  CompositeOperation? composite = null;
> 
> Why does this do the null thing?  If we need a value that means "use the
> value from somewhere else", then can't we just use the missing value
> (undefined) here instead?

Hopefully I can fix that along with some of the other problems described
above.
> Please comment when you have a moment.

Will do.
(In reply to Boris Zbarsky [:bz] from comment #15)
> I assume we have a followup bug for doing the auto thing here?  Perhaps cite
> in comment?

The spec says auto means 0 until a later version of the spec where I think we'll be integrating media with Web Animations, so there's no concrete plan to do anything else with auto just yet.  But I've filed bug 1215406 for handling KeyframeEffectOptions there more generally, and will reference that from ConvertKeyframeEffectOptions.

> >+struct OffsetIndexedKeyframe
> >+  binding_detail::FastKeyframe mKeyframeDict;
> 
> I guess this avoids doing an unncessary Init() with null on the
> dictionary...  It's a bit unfortunate that we're leaking binding_detail
> stuff into here.  :(

Yeah.  Should these just be taken out of binding_detail?

> >+AppendValueAsString(JSContext* aCx,
> >+                    nsTArray<nsString>& aValues,
> >+                    JS::Handle<JS::Value> aValue)
> 
> Why does this need to do anything other than:
> 
>   return ConvertJSValueToString(aCx, aValue, eStringify, eStringify,
>                                 *aValues.AppendElement());
> 
> ?

I just wasn't aware of ConvertJSValueToString.

> So here id might be dead.  Or at least moved, in theory.  I assume the
> rooting analysis would have caught this; if it does not, please file a bug?

Just tried and it did catch it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50d2fdf6cc0a
(In reply to Brian Birtles (:birtles) from comment #18)
> So aValue is an input parameter and aValues is an output parameter?
> 
> I would have expected the input parameter to come first, and I wonder
> if there is any naming that would make this more clear?

I swapped the arguments around.

> >+static void
> >+GenerateValueEntries(Element* aTarget,
> >+                     nsTArray<OffsetIndexedKeyframe>& aKeyframes,
> >+                     nsTArray<KeyframeStyleAnimationValueEntry>& aResult,
> >+                     ErrorResult& aRv)
> 
> Does aKeyframes need to be mutable here? This applies to a few functions in
> this file. If it's possible to make the inputs const it would be easier to
> distinguish them. It may be, however, that we're using some non-const methods
> on them, in which case it's fine as-is.

I've added a number of consts.

> >+/**
> >+ * Builds an array of AnimationProperty objects to represent the keyframe
> >+ * animation segments in aEntries.
> >+ */
> >+static void
> >+BuildSegmentsFromValueEntries(
> >+    nsTArray<KeyframeStyleAnimationValueEntry>& aEntries,
> >+    nsTArray<AnimationProperty>& aResult)
> >+{
> >+  // Sort the KeyframeStyleAnimationValueEntry objects so that all entries
> >+  // for a given property are together, and the entries are sorted by
> >+  // offset otherwise.
> >+  aEntries.Sort(KeyframeValueEntry::PropertyOffsetComparator());
> >+
> >+  nsCSSProperty lastProperty = eCSSProperty_UNKNOWN;
> >+  AnimationProperty* animationProperty = nullptr;
> >+  for (size_t i = 0; i < aEntries.Length(); ++i) {
> >+    KeyframeStyleAnimationValueEntry& thisEntry = aEntries[i];
> >+    if (thisEntry.mOffset != 1.0f) {
> >+      while (thisEntry.mOffset == aEntries[i + 1].mOffset &&
> >+             thisEntry.mProperty == aEntries[i + 1].mProperty) {
> >+        ++i;
> >+      }
> 
> What stops this from reading past the end of aEntries? Is it the fact that we
> know that the last offset is guaranteed to be 1.0 so therefore the equality
> test will fail (since thisEntry.mOffset != 1.0f)? If so, we should probably
> add a comment/assertion to that effect.

Yes, that's it; we've already previously ensured that the last entry's offset is 1.0f.  I'll assert so.

> >+      KeyframeStyleAnimationValueEntry& nextEntry = aEntries[i + 1];
> >+      if (thisEntry.mOffset != nextEntry.mOffset ||
> >+          thisEntry.mProperty != nextEntry.mProperty) {
> 
> I don't quite understand this part. When will the condition be false?
> It seems like we keep incrementing |i| until it refers to the last entry with
> the same offset property. Then we assign the *next* entry after that. So
> why would thisEntry and nextEntry not differ here?

Hmm, yes, that if statement is superfluous.

> Also, this doesn't seem to line up with the spec as far as I understand it.
> 
> According to the spec, if you have:
> 
>   { left: '100px', offset: 0 },
>   { left: '200px', offset: 0 },
>   { left: '300px', offset: 1 },
>   { left: '400px', offset: 1 }
> 
> You should end using 100px when you backwards fill, and 400px when you
> forwards fill, but interpolate between 200px and 300px.
> 
> That comes from:
> https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-
> animation-effect
> 
> So, I think we should end up creating three segments in this case (offset
> 0->0, 0->1, 1->1). I don't see how the current code achieves this, however.
> 
> Similarly, for the following case:
> 
>   { left: '100px', offset: 0 },
>   { left: '200px', offset: 0.5 },
>   { left: '300px', offset: 0.5 },
>   { left: '400px', offset: 0.5 },
>   { left: '500px', offset: 1.0 }
> 
> We should create a segments like:
> 
>   left:
>      { mFromKey: 0, mToKey: 0.5, mFrom: '100px', mTo: '200px' },
>      { mFromKey: 0.5, mToKey: 1.0, mFrom: '400px', mTo: '500px' }

OK I'll work on that.
> Yeah.  Should these just be taken out of binding_detail?

They're there for a reason: using them is a giant footgun because they leave their members uninitialized until you initialize them.  I would prefer to leave them in binding_detail so people don't start using them willy nilly; at least this way they know there's something to watch out for.
Blocks: 1108055
(In reply to Brian Birtles (:birtles) from comment #20)
> >+[constructor.html]
> >+  type: testharness
> >+  [easing values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeTimingOptions]
> >+    expected: FAIL
> >+  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in PropertyIndexedKeyframes]
> >+    expected: FAIL
> >+  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in Keyframe]
> >+    expected: FAIL
> >+  [composite values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeTimingOptions]
> >+    expected: FAIL
> >+  [a KeyframeEffectReadOnly can be constructed with a Keyframe sequence with different composite values, but the same composite value for a given offset]
> >+    expected: FAIL
> 
> Should we annotate these with bug numbers?

OK.

> I think we need the following bugs:
> 
> * Implement animation-level easing

Bug 1216842.

> * Implement animation composition

Bug 1216843.

> * Implement additive animation (not yet tested as mentioned in the TODO)

Bug 1216844.

> >diff --git a/testing/web-platform/tests/web-animations/keyframe-effect/constructor.html b/testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
> >+var gEmptyKeyframeListTests = [
> >+  [],
> >+  [{}],
> >+  [{ easing: "ease-in" }],
> >+  [{ unknown: "unknown" }, { unknown: "unknown" }],
> >+  [{ color: "invalid" }, { color: "invalid" }],
> >+  { easing: "ease-in" },
> >+  { unknown: "unknown" },
> >+  { unknown: [] },
> >+  { unknown: ["unknown"] },
> >+  { unknown: ["unknown", "unknown"] },
> >+  { animationName: ["none", "abc"] },
> >+  { color: [] },
> >+  null,
> >+  undefined,
> >+];
> 
> I wonder if it is ok to use the Mozilla naming convention for global
> variables in web-platform-tests?

Not sure if anyone would complain...

> >+var gPropertyIndexedKeyframesTests = [
> >+  { desc:   "a one property two value PropertyIndexedKeyframes specification",
> >+    input:  { left: ["10px", "20px"] },
> >+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", left: "10px" },
> 
> Is it possible to wrap this section to 80 characters?

So I deliberately violated 80 character limits in these test tables here because it makes them a lot easier to read and compare.  I hope that's OK.

> >+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", left: "20px" }] },
> >+  { desc:   "a one shorthand property two value PropertyIndexedKeyframes specification",
> >+    input:  { margin: ["10px", "10px 20px 30px 40px"] },
> >+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", marginTop: "10px", marginRight: "10px", marginBottom: "10px", marginLeft: "10px" },
> >+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", marginTop: "10px", marginRight: "20px", marginBottom: "30px", marginLeft: "40px" }] },
> 
> Just to check my recollection here, we dicussed making sure longhands always
> override shorthands. Most of the time that happens to work since we apply
> properties in lexographical order by property name. However, for some
> properties like border-*-width that isn't the case. We decided to address
> that
> in follow-up bug. Is that right?

Yes, although in the end I decided to handle it in this bug, and with a slightly more complicated ordering method -- longhands override shorthands, shorthands with fewer longhand components override those with more longhand components, and for shorthands with equal numbers of longhand components, IDL name order is used.

> >+  { desc:   "a PropertyIndexedKeyframes specification using a camel-cased property's IDL name",
> >+    input:  { marginLeft: ["10px", "20px"],
> >+              "margin-top": ["30px", "40px"] },
> >+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", marginLeft: "10px" },
> >+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", marginLeft: "20px" }] },
> >+  { desc:   "a PropertyIndexedKeyframes specification using a camel-cased property's IDL name",
> 
> I believe the Chrome guys mentioned wanting to support "margin-top" like this
> at one point. Can you check if Chrome supports this?

They do support it.  I can remove that test if you think we'll want to support it too in the future.

> >+  { desc:   "a Keyframe sequence with duplicate values for a given offset",
> >+    input:  [{ offset: 0, left: "10px" },
> >+             { offset: 0, left: "20px" },
> >+             { offset: 1, left: "30px" },
> >+             { offset: 1, left: "40px" }],
> >+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", left: "10px" },
> >+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", left: "30px" }] },
> 
> For this case, all four keyframes should be preserved, I believe. This is
> actually useful for providing different fill values.

Upcoming new patches will preserve that.

> A few other tests I think we should add here include:
> 
> * The test case from comment 19

Added.

> * It might be worth doing a test that we can take the output of getFrames
>   and feed it back into the constructor and get the same result again?

For each subtest, or to test something in particular?

> * I think we should test that the following throws:
> 
>   new KeyframeEffectReadOnly(target,
>     [ { left: [ "10px", "30px" ] }, { left: "50px" } ])

OK, I think this should be a mochitest then, since it's not something we want to add to web-platform-tests.

> * And I think we need tests for stringification, e.g.
> 
>     new KeyframeEffectReadOnly(target, { opacity: [ 0, 1 ] });
>     new KeyframeEffectReadOnly(target, [ { opacity: 0 }, { opacity: 1 } ]);
> 
>   I think that one is important.

Added.

> >+// TODO: add tests for the following, once the spec has been clarified:
> >+//
> >+//   * a single value (either as a string or an array of one string)
> >+//     in a PropertyIndexedKeyframes, which should be an animation with the
> >+//     first segment having an additive zero value
> >+//   * the first or last value being invalid in a PropertyIndexedKeyframes,
> >+//     which should be an animation with the first value of the first segment
> >+//     or the second value of the last segment having an additive zero value
> >+//   * a sequence<Keyframe> with missing properties on the 0% or 100%
> >+//     keyframe, which should be an animation with those properties having
> >+//     an additive zero value
> 
> Is it worth testing these three and adding a fails annotations? Seeing as we
> expect to ship before implementing addition, it seems like a good idea to
> exercise this code to ensure it doesn't trigger anything nasty.

Can you tell me what the expected output for this would be when you call getFrames():

  [{ offset: 0, left: "10px" },
   { offset: 1, left: "20px", top: "30px" }]

I'm assuming it's something like:

  [{ offset: 0, left: "10px", easing: "linear", composite: "replace" },
   { offset: 0, top: "0", easing: "linear", composite: "add" },
   { offset: 1, left: "20px", top: "30px", composite: "replace" }]

i.e. we'd do something similar to what our getFrames() implementation currently does with combining same-easing-value Keyframes together, but is this defined in the spec?

> >+//   * the easing value exposed on the final Keyframe object returned by
> >+//     getFrames()
> >+//   * a sequence<Keyframe> that uses different easing/composite values for
> >+//     different offsets, which tests that we generate a minimal set of
> >+//     keyframes and in a specific order
> >+//   * tests for parsing easing values using perhaps-unexpected CSS syntax,
> >+//     such as "Ease\\2d in-out" and "linear /**/"
> >+//   * setting border-color (a shorthand) and border-bottom-color on the
> >+//     one keyframe to test whether shorthands are always overridden by
> >+//     their longhands, or whether it is done purely based on CSS property
> >+//     IDL name order
> 
> Do you plan on adding any of these before landing?

I wasn't going to -- unless the spec now defines what to do in these cases?
This changed a little while getting zero-length 0%/100% and discontinuous animations to work.
Attachment #8671229 - Attachment is obsolete: true
Attachment #8676605 - Flags: review?(bbirtles)
Attached patch Part 11: Tests. (v2) (obsolete) — Splinter Review
Attachment #8671230 - Attachment is obsolete: true
Attachment #8676607 - Flags: review?(bbirtles)
(In reply to Cameron McCormack (:heycam) from comment #28)
> > >+var gPropertyIndexedKeyframesTests = [
> > >+  { desc:   "a one property two value PropertyIndexedKeyframes specification",
> > >+    input:  { left: ["10px", "20px"] },
> > >+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", left: "10px" },
> > 
> > Is it possible to wrap this section to 80 characters?
> 
> So I deliberately violated 80 character limits in these test tables here
> because it makes them a lot easier to read and compare.  I hope that's OK.

It's fine. I'll simply begrudge you for life.

> > Just to check my recollection here, we dicussed making sure longhands always
> > override shorthands. Most of the time that happens to work since we apply
> > properties in lexographical order by property name. However, for some
> > properties like border-*-width that isn't the case. We decided to address
> > that
> > in follow-up bug. Is that right?
> 
> Yes, although in the end I decided to handle it in this bug, and with a
> slightly more complicated ordering method -- longhands override shorthands,
> shorthands with fewer longhand components override those with more longhand
> components, and for shorthands with equal numbers of longhand components,
> IDL name order is used.

Ok, I need to spec that.

Filed as: https://github.com/w3c/web-animations/issues/136

> > I believe the Chrome guys mentioned wanting to support "margin-top" like this
> > at one point. Can you check if Chrome supports this?
> 
> They do support it.  I can remove that test if you think we'll want to
> support it too in the future.

Yes please. I think we'll probably end up allowing that unless you have an
opinion about this?

> > * It might be worth doing a test that we can take the output of getFrames
> >   and feed it back into the constructor and get the same result again?
> 
> For each subtest, or to test something in particular?

Probably for each subtest if it's easy to add as a simple sanity check.
If it's difficult, however, I don't think it's important.

> > * I think we should test that the following throws:
> > 
> >   new KeyframeEffectReadOnly(target,
> >     [ { left: [ "10px", "30px" ] }, { left: "50px" } ])
> 
> OK, I think this should be a mochitest then, since it's not something we
> want to add to web-platform-tests.

Oh, in that case, it's not so important since this is only a temporary measure
until we get to support additive animation.

> > >+// TODO: add tests for the following, once the spec has been clarified:
> > >+//
> > >+//   * a single value (either as a string or an array of one string)
> > >+//     in a PropertyIndexedKeyframes, which should be an animation with the
> > >+//     first segment having an additive zero value
> > >+//   * the first or last value being invalid in a PropertyIndexedKeyframes,
> > >+//     which should be an animation with the first value of the first segment
> > >+//     or the second value of the last segment having an additive zero value
> > >+//   * a sequence<Keyframe> with missing properties on the 0% or 100%
> > >+//     keyframe, which should be an animation with those properties having
> > >+//     an additive zero value
> > 
> > Is it worth testing these three and adding a fails annotations? Seeing as we
> > expect to ship before implementing addition, it seems like a good idea to
> > exercise this code to ensure it doesn't trigger anything nasty.
> 
> Can you tell me what the expected output for this would be when you call
> getFrames():
> 
>   [{ offset: 0, left: "10px" },
>    { offset: 1, left: "20px", top: "30px" }]
> 
> I'm assuming it's something like:
> 
>   [{ offset: 0, left: "10px", easing: "linear", composite: "replace" },
>    { offset: 0, top: "0", easing: "linear", composite: "add" },
>    { offset: 1, left: "20px", top: "30px", composite: "replace" }]

Actually, the middle frame is added at style computation time. It shouldn't be
returned by getFrames().

> i.e. we'd do something similar to what our getFrames() implementation
> currently does with combining same-easing-value Keyframes together, but is
> this defined in the spec?

As the spec stands, this should give:

   [{ offset: 0, left: "10px", easing: "linear" },
    { offset: 1, left: "20px", top: "30px", easing: "linear" }]

> > >+//   * the easing value exposed on the final Keyframe object returned by
> > >+//     getFrames()
> > >+//   * a sequence<Keyframe> that uses different easing/composite values for
> > >+//     different offsets, which tests that we generate a minimal set of
> > >+//     keyframes and in a specific order
> > >+//   * tests for parsing easing values using perhaps-unexpected CSS syntax,
> > >+//     such as "Ease\\2d in-out" and "linear /**/"
> > >+//   * setting border-color (a shorthand) and border-bottom-color on the
> > >+//     one keyframe to test whether shorthands are always overridden by
> > >+//     their longhands, or whether it is done purely based on CSS property
> > >+//     IDL name order
> > 
> > Do you plan on adding any of these before landing?
> 
> I wasn't going to -- unless the spec now defines what to do in these cases?

> > >+//   * the easing value exposed on the final Keyframe object returned by
> > >+//     getFrames()

I guess this will depend on what we spec for CSS. For all other cases however it
will be "linear".

> > >+//   * a sequence<Keyframe> that uses different easing/composite values for
> > >+//     different offsets, which tests that we generate a minimal set of
> > >+//     keyframes and in a specific order

Merging is only applied to property-indexed keyframes and since you can't
specify different easing/composite values on property-indexed keyframes we
don't need to test that a minimal set of keyframes is generated. (On the
contrary we might need to test that the original keyframes are preserved--but we
can do that once we implement that.)

> > >+//   * tests for parsing easing values using perhaps-unexpected CSS syntax,
> > >+//     such as "Ease\\2d in-out" and "linear /**/"

The spec now says "use the CSS parser" but that's probably not the correct
language. In any case, that's the intention. I still need to spec what we do
with invalid timing values however. I should get to that today.

> > >+//   * setting border-color (a shorthand) and border-bottom-color on the
> > >+//     one keyframe to test whether shorthands are always overridden by
> > >+//     their longhands, or whether it is done purely based on CSS property
> > >+//     IDL name order

As above, I filed an issue to spec this.
(In reply to Brian Birtles (:birtles) from comment #32)
> > So I deliberately violated 80 character limits in these test tables here
> > because it makes them a lot easier to read and compare.  I hope that's OK.
> 
> It's fine. I'll simply begrudge you for life.

:)

> > > I believe the Chrome guys mentioned wanting to support "margin-top" like this
> > > at one point. Can you check if Chrome supports this?
> > 
> > They do support it.  I can remove that test if you think we'll want to
> > support it too in the future.
> 
> Yes please. I think we'll probably end up allowing that unless you have an
> opinion about this?

It's OK, but introducing even more complexity in finding which properties override which other ones, when you specify both say "margin-left" and "marginLeft".

> > > * It might be worth doing a test that we can take the output of getFrames
> > >   and feed it back into the constructor and get the same result again?
> > 
> > For each subtest, or to test something in particular?
> 
> Probably for each subtest if it's easy to add as a simple sanity check.
> If it's difficult, however, I don't think it's important.

Turned out to be easy.

> > > * I think we should test that the following throws:
> > > 
> > >   new KeyframeEffectReadOnly(target,
> > >     [ { left: [ "10px", "30px" ] }, { left: "50px" } ])
> > 
> > OK, I think this should be a mochitest then, since it's not something we
> > want to add to web-platform-tests.
> 
> Oh, in that case, it's not so important since this is only a temporary
> measure
> until we get to support additive animation.

OK.  Well the patch I just uploaded before has it; it'll easy to remove once we do support it.

> > Can you tell me what the expected output for this would be when you call
> > getFrames():
> > 
> >   [{ offset: 0, left: "10px" },
> >    { offset: 1, left: "20px", top: "30px" }]
> > 
> > I'm assuming it's something like:
> > 
> >   [{ offset: 0, left: "10px", easing: "linear", composite: "replace" },
> >    { offset: 0, top: "0", easing: "linear", composite: "add" },
> >    { offset: 1, left: "20px", top: "30px", composite: "replace" }]
> 
> Actually, the middle frame is added at style computation time. It shouldn't
> be
> returned by getFrames().
> 
> > i.e. we'd do something similar to what our getFrames() implementation
> > currently does with combining same-easing-value Keyframes together, but is
> > this defined in the spec?
> 
> As the spec stands, this should give:
> 
>    [{ offset: 0, left: "10px", easing: "linear" },
>     { offset: 1, left: "20px", top: "30px", easing: "linear" }]

Thanks.  And that would have composite:"replace" on both Keyframes, right?

> > > >+//   * the easing value exposed on the final Keyframe object returned by
> > > >+//     getFrames()
> 
> I guess this will depend on what we spec for CSS. For all other cases
> however it
> will be "linear".

I'll leave that for a followup.

> > > >+//   * a sequence<Keyframe> that uses different easing/composite values for
> > > >+//     different offsets, which tests that we generate a minimal set of
> > > >+//     keyframes and in a specific order
> 
> Merging is only applied to property-indexed keyframes and since you can't
> specify different easing/composite values on property-indexed keyframes we
> don't need to test that a minimal set of keyframes is generated. (On the
> contrary we might need to test that the original keyframes are
> preserved--but we
> can do that once we implement that.)

Right, OK.  In that case I'll just remove that dot point in the comment.

> > > >+//   * tests for parsing easing values using perhaps-unexpected CSS syntax,
> > > >+//     such as "Ease\\2d in-out" and "linear /**/"
> 
> The spec now says "use the CSS parser" but that's probably not the correct
> language. In any case, that's the intention. I still need to spec what we do
> with invalid timing values however. I should get to that today.

OK I'll add some subtests for that.

> > > >+//   * setting border-color (a shorthand) and border-bottom-color on the
> > > >+//     one keyframe to test whether shorthands are always overridden by
> > > >+//     their longhands, or whether it is done purely based on CSS property
> > > >+//     IDL name order
> 
> As above, I filed an issue to spec this.

I'll add tests for this too then.
Attachment #8676607 - Attachment is obsolete: true
Attachment #8676607 - Flags: review?(bbirtles)
Attachment #8676625 - Flags: review?(bbirtles)
Comment on attachment 8676625 [details] [diff] [review]
Part 11: Tests. (v3)

>+  { desc:   "a PropertyIndexedKeyframes specification using a camel-cased property's IDL name",
>+    input:  { marginLeft: ["10px", "20px"],
>+              "margin-top": ["30px", "40px"] },
>+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", marginLeft: "10px" },
>+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", marginLeft: "20px" }] },
>+  { desc:   "a PropertyIndexedKeyframes specification using a camel-cased property's IDL name",
>+    input:  { marginLeft: ["10px", "20px"],
>+              "margin-top": ["30px", "40px"] },
>+    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", marginLeft: "10px" },
>+             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", marginLeft: "20px" }] },

Forgot to remove this; will do that locally.
Comment on attachment 8676605 [details] [diff] [review]
Part 9: Implement KeyframeEffectReadOnly constructor. r=bzbarsky (v2)

Review of attachment 8676605 [details] [diff] [review]:
-----------------------------------------------------------------

Looks splendid.

::: dom/animation/KeyframeEffect.cpp
@@ +756,5 @@
> + */
> +struct OffsetIndexedKeyframe
> +{
> +  binding_detail::FastKeyframe mKeyframeDict;
> +  nsTArray<PropertyValuesPair> mPropertyValuePairs;

Nit: I noticed you renamed a lot of s/PropertyValue/PropertyValues/. You did that for the type here but did you mean to rename the member to mPropertyValuesPairs as well?

@@ +1126,5 @@
> +    // and:
> +    //
> +    //   border-top-color, border-color, border-top, border
> +    //
> +    // This allows us to prioritise values specified by longhands (or smaller

I think the Americans say prioritize.
Attachment #8676605 - Flags: review?(bbirtles) → review+
Comment on attachment 8676606 [details] [diff] [review]
Part 10: Make GetFrames aware of initial/final zero-length segments and discontinuities between segments.

Review of attachment 8676606 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I want to understand how to the frame sorting works.

::: dom/animation/KeyframeEffect.cpp
@@ +1137,5 @@
>   * animation segments in aEntries.
>   */
>  static void
> +BuildSegmentsFromValueEntries(nsTArray<KeyframeValueEntry>& aEntries,
> +                                       nsTArray<AnimationProperty>& aResult)

This indentation seems off.

@@ +1549,5 @@
>    return effect.forget();
>  }
>  
> +/**
> + * An offset and ComputeTimingFunction pointer pair.

ComputedTimingFunction

@@ +1555,5 @@
> +struct FrameKey
> +{
> +  FrameKey() {}
> +  FrameKey(const FrameKey& aOther)
> +    : mOffset(aOther.mOffset), mTimingFunction(aOther.mTimingFunction) {}

Why are the compiler-generated default ctor and copy ctor not sufficient here?

@@ +1588,5 @@
> +
> +/**
> + * FrameKey hashtable entry class.
> + */
> +class Frame : public PLDHashEntryHdr {

FrameEntry?

@@ +1600,5 @@
> +  Frame(const Frame& aOther)
> +    : mKey(aOther.mKey)
> +    , mValues(aOther.mValues)
> +    , mGeneratesTwoFrames(aOther.mGeneratesTwoFrames) {}
> +  ~Frame() {}

Do we need the dtor here?

@@ +1717,5 @@
> +  nsTArray<FrameKey> keys;
> +  for (auto iter = frames.Iter(); !iter.Done(); iter.Next()) {
> +    keys.AppendElement(iter.Get()->GetKey());
> +  }
> +  keys.Sort(FrameKey::OffsetEasingComparator());

Let me check I've understood how this works.

Suppose we have the following segments for the "left" property:

  mFromKey -> mToKey, mFromValue -> mToValue, mTimingFunction
  0 -> 0, "100px" -> "200px", "ease"
  0 -> 0.5, "200px" -> "300px", "linear"
  0.5 -> 1, "300px" -> "400px", "ease-out"
  1 -> 1, "400px" -> "500px", "step-end" (Does this happen?)

We'll have a 'frames' hashtable that includes the following entries:

  (mOffset, mTimingFunction) => mValues, mGeneratesTwoFrames
  (0, "ease") => [ left/100px ], false
  (0, "linear") => [ left/200px ], false
  (0.5, "ease-out") => [ left/300px ], false
  (1, "step-end") => [ left/400px, left/500px ], true

Is that right? If so the sorting here would not necessarily produce the right result since the order of the first two frames is significant.

I'm probably missing some invariant about how the segments are set up, however.

@@ +1750,5 @@
> +             (i == 0 ||
> +              frameValue.mProperty != frame->mValues[i - 1].mProperty)) ||
> +            (frameNum == 1 &&
> +             (i == n - 1 ||
> +              frameValue.mProperty != frame->mValues[i + 1].mProperty))) {

To check I understand this, this works because we add values to mValues on the Frame such that properties are clumped together due to the way we iterate through segments (which is, we go through all the segments for a property, then move onto the next property).
Cameron, as per my question in comment 39, I think I'm missing something about how segments are set up.

(Apparently splinter doesn't let you set needinfo.)
Flags: needinfo?(cam)
You are not missing something; your example shows that we'll get the wrong output due to the different easing values.

So we need to preserve the relative order of the two values for a given property, and only once we maintain that ordering should we consider merging Keyframes together for identical easing values.  I think we'll need to:

* move easing from the key and into the KeyframeValue
* additionally annotate each KeyframeValue with a boolean that indicates whether it was the first or second value
* make the comparator sort just by offset
* when iterating over each offset, sort the KeyframeValue array by fields in this order:
    first-or-second boolean, easing, property ID

[re the condition in the JS Keyframe object generation loop]
> To check I understand this, this works because we add values to mValues on the Frame
> such that properties are clumped together due to the way we iterate through segments
> (which is, we go through all the segments for a property, then move onto the next
> property).

Yes that's right.  Although with the above changes I think we can just inspect the boolean that we stored (particularly since the two values for a property won't necessarily be adjacent now.)

Does that all sound right?
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #41)
> Does that all sound right?

Yes, that sounds good. Thanks!

(At some point I wonder if it would be easier to just store the original keyframes, and make up the per-property segment arrays on-demand. The we could skip merging except for when dealing with property-indexed keyframes. But for now this seems quite doable and we can do the rest in bug 1217252.)
Comment on attachment 8676625 [details] [diff] [review]
Part 11: Tests. (v3)

Review of attachment 8676625 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks!

::: dom/animation/test/mozilla/file_partial_keyframes.html
@@ +35,5 @@
> +      new KeyframeEffectReadOnly(div, subtest.keyframes);
> +    } catch (ex) {
> +      threw = true;
> +    }
> +    assert_true(threw, "exception thrown");

I think we could use assert_throws here?

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html
@@ +76,5 @@
> +  ["inherit", "linear"],
> +  ["var(--x)", "linear"],
> +  ["ease-in-out, ease-out", "linear"],
> +  ["Ease\\2d in-out", "ease-in-out"],
> +  ["linear /**/", "linear"],

Since the default is linear, would it be better to test some other value for this? (To check the comment is successfully ignored rather than the whole thing simply being treated as invalid.)

@@ +238,5 @@
> +    input:  { left: ["10px"] },
> +    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "add",     left: "0" },
> +             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", left: "10px" }] },
> +  { desc:   "a one property one non-array value PropertyIndexedKeyframes specification",
> +    input:  { left: ["10px"] },

This seems to be an array value?

@@ +253,5 @@
> +  { desc:   "a two property PropertyIndexedKeyframes specification where one property is missing from the first Keyframe",
> +    input:  [{ offset: 0, left: "10px" },
> +             { offset: 1, left: "20px", top: "30px" }],
> +    output: [{ offset: 0, computedOffset: 0, easing: "linear",       composite: "replace", left: "10px" },
> +             { offset: 1, computedOffset: 1, /* easing: "linear", */ composite: "replace", left: "20px", top: "30px" }] },

This is a bit inconsistent with the previous tests. In the previous tests we are checking for the synthesized 0/1 keyframes but in this case we don't check for the synthesized 0% value for 'top' (which would have to be a separate keyframe in this case in order to have a different "replace" value).

As the spec currently stands, we shouldn't return synthesized values at all so this is correct while the previous tests need to be updated.
Attachment #8676625 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #37)
> > +  binding_detail::FastKeyframe mKeyframeDict;
> > +  nsTArray<PropertyValuesPair> mPropertyValuePairs;
> 
> Nit: I noticed you renamed a lot of s/PropertyValue/PropertyValues/. You did
> that for the type here but did you mean to rename the member to
> mPropertyValuesPairs as well?

That one was deliberate, since we only store one value in here.
Comment on attachment 8677251 [details] [diff] [review]
Part 10: Make GetFrames aware of initial/final zero-length segments and discontinuities between segments. (v2)

Review of attachment 8677251 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Cameron!

::: dom/animation/KeyframeEffect.cpp
@@ +1657,5 @@
> +          ValuePosition::First :
> +          ValuePosition::Right;
> +
> +      if (i == n - 1 ||
> +          segment.mToValue != property.mSegments[i + 1].mFromValue) {

Is it ok to unconditionally add the last segment's to-value because we'll never add a redundant last segment (e.g. one that goes from offset 1->1 and has identical values)?

@@ +1710,5 @@
>        }
> +      if (++i == n) {
> +        break;
> +      }
> +      previousEntry = entry;

I guess we could even declare previousEntry here?
Attachment #8677251 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #46)
> Is it ok to unconditionally add the last segment's to-value because we'll
> never add a redundant last segment (e.g. one that goes from offset 1->1 and
> has identical values)?

Right.  That would already have been filtered out by the KeyframeEffectReadOnly constructor (and shouldn't occur with CSS Animations).

> @@ +1710,5 @@
> >        }
> > +      if (++i == n) {
> > +        break;
> > +      }
> > +      previousEntry = entry;
> 
> I guess we could even declare previousEntry here?

No I don't think the scope extends to the condition there.
Hi Cameron, I'm just going through this code so I can spec something compatible regarding shorthand handling.[1]

One thing I don't quite understand, however, is how we handle shorthand prioritization for the property-indexed keyframe case.

For a sequence of keyframes, we call BuildAnimationPropertyListFromKeyframeSequence which follows this sequence:

 1. ConvertKeyframeSequence (which calls GetPropertyValuesPairs)
 2. HasValidOffsets
 3. ApplyDistributeSpacing
 4. GenerateValueEntries
 5. BuildSegmentsFromValueEntries

During (4), we get the list of property-value pairs and sort them using the PropertyValuesPair::PropertyPriorityComparator() which implements the proposed shorthand handling (i.e. longhands override shorthands, shorthands with fewer longhand components override those with more longhand components, and for shorthands with equal numbers of longhand components, IDL name order is used.)

However, for the property-indexed keyframe case, we call GetPropertyValuesPairs on the object, then we iterate over those properties and make up the segments directly. We still expand shorthands and take care not to set longhands that have already been set, but the properties returned by GetPropertyValuesPairs are only sorted using AdditionalProperty::PropertyComparator() which just sorts by IDL name. Are we missing a call here to sort the property value pairs by PropertyValuesPair::PropertyPriorityComparator()? Or am I overlooking something?

[1] https://github.com/w3c/web-animations/issues/136
Flags: needinfo?(cam)
(In reply to Brian Birtles (:birtles) from comment #50)
> However, for the property-indexed keyframe case, we call
> GetPropertyValuesPairs on the object, then we iterate over those properties
> and make up the segments directly. We still expand shorthands and take care
> not to set longhands that have already been set, but the properties returned
> by GetPropertyValuesPairs are only sorted using
> AdditionalProperty::PropertyComparator() which just sorts by IDL name. Are
> we missing a call here to sort the property value pairs by
> PropertyValuesPair::PropertyPriorityComparator()?

I think we are.  I can't think of a good reason why we can't (or shouldn't) use the same shorthand/longhand priority order for property-indexed keyframes.
Flags: needinfo?(cam)
Depends on: 1234476
(In reply to Cameron McCormack (:heycam) from comment #51)
> (In reply to Brian Birtles (:birtles) from comment #50)
> > However, for the property-indexed keyframe case, we call
> > GetPropertyValuesPairs on the object, then we iterate over those properties
> > and make up the segments directly. We still expand shorthands and take care
> > not to set longhands that have already been set, but the properties returned
> > by GetPropertyValuesPairs are only sorted using
> > AdditionalProperty::PropertyComparator() which just sorts by IDL name. Are
> > we missing a call here to sort the property value pairs by
> > PropertyValuesPair::PropertyPriorityComparator()?
> 
> I think we are.  I can't think of a good reason why we can't (or shouldn't)
> use the same shorthand/longhand priority order for property-indexed
> keyframes.

Great, thanks for checking. Filed bug 1234476 for that.
You need to log in before you can comment on or make changes to this bug.