New issue
Advanced search Search tips

Issue 777971 link

Starred by 1 user
Status: Fixed
Owner:
smcgruer@chromium.org
Closed: Feb 2018
Cc:
flackr@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocked on:
issue 792241
Blocking:
issue 772407



Sign in to add a comment

Web Animations: Implement KeyframeEffectReadOnly::getKeyframes()

Project Member Reported by smcgruer@chromium.org, Oct 24 2017 
This is a tracking bug for implementing KeyframeEffectReadOnly::getKeyframes(). Most of our WPT failures are (at least initially) due to our lack of support for this method.

Doing so is quite a task, as it requires us to re-do how we treat keyframes. My current plan is to:

  * Make StringKeyframe track 'uncomputed' keyframes - short-hand, possibly invalid (property, value) pairs.

  * Make StringKeyframe::{CSS,SVG}PropertySpecificKeyframe track 'computed' keyframes - long-hand, only valid, and apply the short-hand deduping algorithm from the spec (https://www.w3.org/TR/web-animations-1/#computed-keyframes).

This may or may not be feasible.

Comment 1 by smcgruer@chromium.org, Oct 24 2017

Blocking: 772407

Comment 2 by shend@chromium.org, Oct 24 2017

Labels: Update-Monthly

Comment 3 by nainar@chromium.org, Oct 30 2017

Labels: -Type-Bug -Update-Monthly Update-Quarterly Type-Feature
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b16f620ba37c5274ad171321b9e3beab44cd5807

commit b16f620ba37c5274ad171321b9e3beab44cd5807
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Wed Nov 15 00:55:26 2017

Properly differentiate between computed/non-computed keyframe offsets

The web-animation spec makes a clear distinction between keyframe
offsets (which are user-provided) and computed keyframe offsets (which
are computed by the user agent when required). Importantly, we are
required to store the user-provided keyframe offsets for later
retrieval.

Although we did not actually overwrite the user-provided offset when
calculating the computed offsets, it was not clear in the code what a
blink::Keyframe offset represented. This CL gets rid of
NormalizeKeyframes, replacing it with GetComputedOffsets and converting
the callsites to use that instead.

This will also make implementing getKeyframes() much easier, since that
API requires access to both user-specified and computed offsets.

Bug:  777971 
Change-Id: Ib4346b2efede4bb227f47dbc01f9f071dfdcab74
Reviewed-on: https://chromium-review.googlesource.com/755056
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516531}
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/Keyframe.h
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/KeyframeEffectModel.h
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/KeyframeEffectModelTest.cpp
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/StringKeyframe.cpp
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/StringKeyframe.h
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/TransitionKeyframe.cpp
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/animation/TransitionKeyframe.h
[modify] https://crrev.com/b16f620ba37c5274ad171321b9e3beab44cd5807/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 18 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10686d89f1f1ce9eac09b8bc29bfe7e814d55214

commit 10686d89f1f1ce9eac09b8bc29bfe7e814d55214
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Sat Nov 18 05:00:23 2017

Implement KeyframeEffectReadOnly::getKeyframes()

This CL implements most of the getKeyframes() algorithm from the
web-animations spec[0]. There are a few deviations from the spec:

  * We always include the 'composite' property. This is because we
    don't yet track the general composite operation for the KeyframeEffect,
    so cannot tell when a Keyframe has the same operation.
  * Shorthand CSS properties in the input keyframe argument will have been
    expanded to longhand properties in getKeyframes(). This is because we
    improperly expand shorthand properties when creating a StringKeyframe.

Despite the deviations this implementation allows us to pass >40 additional
WPT tests. It also makes the failure reasons for many more tests explicit,
as they no longer just fail on "getKeyframes doesn't exist".

[0]: http://w3c.github.io/web-animations/#dom-keyframeeffectreadonly-getkeyframes

Bug:  777971 
Change-Id: I80cf62bc0121b946b1bf896890e96afd27437057
Reviewed-on: https://chromium-review.googlesource.com/758921
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517701}
[add] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/animations/web-animations/KeyframeEffectReadOnly-getKeyframes-correct-context.html
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animatable/animate-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/composite-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/idlharness-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-002-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/AnimationInputHelpers.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/AnimationInputHelpers.h
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/AnimationInputHelpersTest.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/EffectInput.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/Keyframe.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/Keyframe.h
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/KeyframeEffectModel.h
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.h
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.idl
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/StringKeyframe.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/StringKeyframe.h
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/TransitionKeyframe.cpp
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/animation/TransitionKeyframe.h
[modify] https://crrev.com/10686d89f1f1ce9eac09b8bc29bfe7e814d55214/third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp

Comment 6 by smcgruer@chromium.org, Dec 8 2017

Blockedon: 792241
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5494d16b35fe0108017c95d39ce5c040bb4e3a82

commit 5494d16b35fe0108017c95d39ce5c040bb4e3a82
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Jan 12 18:25:49 2018

Make parsing of object-form keyframes argument more spec-compliant

Our implementation of parsing an object-form keyframes argument was
previously quite far from the current spec. This CL brings it much
closer inline with the spec, including:

  * Correctly merge keyframes with identical computed offsets.
  * Reorder some operations so that thrown exceptions are ordered
    correctly per the spec.
  * Properly handle 'offset' if specified, particularly in accepting
    a list of offset values rather than just a single offset.
  * Properly handle 'composite' if specified, particularly in accepting
    a (repeating) list of composite values rather than just a single offset.
  * Properly handle 'easing; if specified, particularly in both accepting
    a (repeating) list of easing values rather than just a single easing,
    and in delaying parsing the easing until the spec says to.
  * Parse and throw if necessary any unused easing values.

Bug:  777971 
Change-Id: I1d37fb01b1b41b8b5e37e8415d2325b33bdf866e
Reviewed-on: https://chromium-review.googlesource.com/760542
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528996}
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/combining-effects/effect-composition-expected.txt
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animatable/animate-expected.txt
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001-expected.txt
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-002-expected.txt
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/bindings/core/v8/BUILD.gn
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/bindings/core/v8/IDLTypesTest.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/bindings/modules/v8/generated.gni
[add] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/BasePropertyIndexedKeyframe.idl
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/EffectInput.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/EffectInput.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/EffectInputTest.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/ElementAnimation.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/ElementAnimation.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/ElementAnimation.idl
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffect.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffect.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffect.idl
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.idl
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/animation/KeyframeEffectTest.cpp
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/core/core_idl_files.gni
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/modules/indexeddb/IDBDatabase.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/modules/indexeddb/IDBKeyPath.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/modules/locks/LockManager.h
[modify] https://crrev.com/5494d16b35fe0108017c95d39ce5c040bb4e3a82/third_party/WebKit/Source/modules/websockets/DOMWebSocket.cpp
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 15 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a87f09f58dea41514e17b6b05c5dde42895fc96a

commit a87f09f58dea41514e17b6b05c5dde42895fc96a
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Mon Jan 15 16:43:38 2018

Make parsing of array-form keyframes argument more spec-complaint

i) Keyframes are converted during iteration rather than all upfront.
ii) Use the proper IDL BaseKeyframe.
iii) Only access each (property, value) from the keyframes object once, as per the spec.
iv) Delay parsing of the timing function until step 8.2 as per the spec.

Bug:  777971 
Change-Id: If817161026bffbab36a44feca3b94522a463c4f5
Reviewed-on: https://chromium-review.googlesource.com/848080
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529297}
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/LayoutTests/animations/web-animations/keyframe-exceptions.html
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/combining-effects/effect-composition-expected.txt
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animatable/animate-expected.txt
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001-expected.txt
[delete] https://crrev.com/f41c0db9421941c9de55cd367e11900d32bb08fb/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-002-expected.txt
[add] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/Source/core/animation/BaseKeyframe.idl
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/Source/core/animation/EffectInput.cpp
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/Source/core/animation/EffectInput.h
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/Source/core/animation/KeyframeEffect.cpp
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/Source/core/animation/KeyframeEffectReadOnly.cpp
[modify] https://crrev.com/a87f09f58dea41514e17b6b05c5dde42895fc96a/third_party/WebKit/Source/core/core_idl_files.gni

Comment 9 by smcgruer@chromium.org, Feb 27 2018

Status: Fixed (was: Started)
This bug is being incorrectly used (by me!) to conflate far too many CLs. The core work of implementing KeyframeEffectReadOnly::getKeyframes() is done, and so I'm marking this as fixed.

The fact that we don't properly process and store keyframes, making the output of getKeyframes() incorrect , is a separate bug and will be tracked separately in issue 816956
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 2 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/621a7170d03ec4a21c982e43115274304fd4e7d2

commit 621a7170d03ec4a21c982e43115274304fd4e7d2
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Mar 02 00:40:34 2018

Fix bug with parsing of keyframes argument object

By spec, we are actually not allowed to access the value of a
non-animatable property. Before this CL, we would access the value and
then ignore it later.

Bug:  777971 
Change-Id: Iadfc5165fdbe1796f3831e666e1c8f2a8fb55b1f
Reviewed-on: https://chromium-review.googlesource.com/934634
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540377}
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/animation-types/accumulation-per-property-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/animation-types/addition-per-property-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/external/wpt/web-animations/animation-model/animation-types/interpolation-per-property-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/platform/mac/external/wpt/web-animations/animation-model/animation-types/accumulation-per-property-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/platform/mac/external/wpt/web-animations/animation-model/animation-types/addition-per-property-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/LayoutTests/platform/mac/external/wpt/web-animations/animation-model/animation-types/interpolation-per-property-expected.txt
[modify] https://crrev.com/621a7170d03ec4a21c982e43115274304fd4e7d2/third_party/WebKit/Source/core/animation/EffectInput.cpp
►

Sign in to add a comment