[web-animations-1] Resolve remaining issues missed in pseudo-element change. by george-steel · Pull Request #4616 · w3c/csswg-drafts

george-steel · 2019-12-19T01:12:39Z

Fixup of #4437 addressing #4301.

Add pseudoElement option to KeyframeEffect constructor body
Fix [css-animations-2] List of pseudo-elements in composite order is non-exhaustive #4502 adding catch-all pseudo-element case to composite order
Fix [web-animations] Error handling in KeyframeEffect.pseudoElement #4586 adding error handling to KeyframeEffect.pseudoElement
Fix [web-animations] Procedure not specified when property values cannot be calculated for effect target #4701 making note of case when property values cannot be calculated

george-steel · 2019-12-19T01:12:54Z

@stephenmcgruer @birtles

birtles · 2020-01-22T23:06:55Z

@george-steel Anything I can do to help get this landed?

george-steel · 2020-01-23T02:28:48Z

Sorry, I was delayed by the holidays and fixing a memory leak. I'll have another go at this in the next few days.

Fixup of w3c#4437 addressing w3c#4301. * Add pseudoElement option to KeyframeEffect constructor body * Fix w3c#4502 adding catch-all pseudo-element case to composite order * Fix w3c#4586 adding error handling to KeyframeEffect.pseudoElement * Fix w3c#4701 making note of case when property values cannot be calculated

george-steel · 2020-01-24T23:39:39Z

@birtles @stephenmcgruer Next draft is ready.

I agree with your advice and decided that the error handling and a couple of other issues needed to be fixed at the same time as the constructor. What I wrote here reflects the tentative decisions we made for blink in the absence of a spec.

birtles

Thanks for doing this. It's getting close.

Sorry for all the feedback. It takes a while to get used to writing spec text. I'm still really bad at it, but these suggestions should hopefully tighten things up a little.

birtles · 2020-01-27T22:38:47Z

 Note that this procedure is only performed on a <a>keyframe effect</a> having
-an <a>effect target</a> for which computed property values can be calculated.
+an <a>effect target</a> for which computed property values can be calculated,
+as otherwise all propereties are <a>not animatable</a>.


Although, I'm not exactly sure what the purpose of this change is. How is it related to pseudo elements? What is the difference between saying the procedure is not applied and the properties are "not animatable"?

It's also a bit weird to say they are "not animatable" since that's something that is normally defined for the property itself. We should instead say something like they follow the same logic as not animatable properties or something. Once I understand the purpose of the change I can help suggest wording.

birtles · 2020-01-27T22:39:52Z

 <var>underlying value</var> is calculated as follows.
+Note that this procedure is only performed on a <a>keyframe effect</a> having
+an <a>effect target</a> for which computed property values can be calculated,
+as otherwise case all propereties are <a>not animatable</a>.


"otherwise case"? s/calculated, as otherwise case all/calculated. Otherwise all/ ?

As above, I'd probably avoid invoking the "not animatable" language since that's really an animation type and I think it is confusing to use that concept here. It's probably better just to say this procedure is not run on that case, or, better still, add a step that early returns if the the effect target is in a context where we can't calculate property values.

birtles · 2020-01-27T22:40:05Z

 property</a> is applied using the following process.
+Note that this procedure is only performed on a <a>keyframe effect</a> having
+an <a>effect target</a> for which computed property values can be calculated,
+as otherwise case all propereties are <a>not animatable</a>.


birtles · 2020-01-27T22:41:11Z


-    1.  Set the [=target pseudo-selector=] to the result corresponding to the first
-        matching condition from below.
+    1.  Set the <a>target pseudo-selector</a> of <var>effect</var> to the result


(Totally unrelated but is there any reason to change [=target pseudo-selector=] to <a>target pseudo-selector</a>? I've been trying to switch to more modern bikeshed syntax bit by bit, including using [= =] instead of <a></a> where possible.)

@stephenmcgruer requested this change

Sorry, I had seen the use of <a> in the line above and asked George to match it. Didn't realize [= =] was a more modern syntax and we were trying to switch to it.

Apologies for wasting your time George, but can you switch it back to [= =] then?

birtles · 2020-01-28T04:25:46Z

+            {{KeyframeEffectOptions/pseudoElement}} property,
+        ::  Set the [=target pseudo-selector=] to the value of that property,
+            following the validation procedure from the
+            {{KeyframeEffect/pseudoElement}} setter.


Nit: For consistency, could we follow the same format we use for composite?

e.g.

Set the [=target pseudo-selector=] to the value of the {{KeyframeEffectOptions/pseudoElement}} property. When assigning this property, the error-handling defined for the corresponding setter on the {{KeyframeEffect}} interface is applied. If the setter requires an exception to be thrown, this procedure must throw the same exception and abort all further steps.

birtles

r=me with nits addressed

birtles · 2020-01-30T03:37:23Z

 to specify that particular [=pseudo-element=].

+Note that not all [=effect targets=] specified in this manner (such as ''::part()''
+pseudo-elements, unsupported/invalid pseudo-elements, and `null`) have


I'm not sure invalid pseudo-elements can exist, right? They're rejected at the API boundary?

What does null refer to here? The lack of a target element? (Represented by null in the programming interface) Or the lack of a target pseudo-selector?

birtles · 2020-01-30T03:39:29Z

 1.  If <a>animation type</a> of the <var>target property</var> is
    <a>not animatable</a> abort this procedure
    since the effect cannot be applied.
+1.  If the [=effect target=] is `null` or cannot have computed property values


Nit: Technically I guess this should say, "The [=keyframe effect=] does not have an [=effect target=]...since we're talking about the model here. They fact that the lack of an associated effect target is represented bynull` in the programming interface is specific to the programming interface.

stephenmcgruer

LGTM modulo Brian's nits

Update animation composite order on pseudo-element. Implement change found in w3c/csswg-drafts#4616 making the sort order on pseudo-elements include all selectors. Add missing const declaration to KeyframeEffect::pseudoElement(). Remove empty-string case from KeyframeEffect::setPseudoElement(). Bug: 993365, 981894 Change-Id: I32ff6917b74fb490695299d0fde0c12393755437 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031986 Commit-Queue: George Steel <gtsteel@chromium.org> Reviewed-by: Stephen McGruer <smcgruer@chromium.org> Cr-Commit-Position: refs/heads/master@{#740490}

stephenmcgruer requested review from birtles and stephenmcgruer December 21, 2019 15:00

stephenmcgruer added the web-animations-1 label Dec 21, 2019

stephenmcgruer changed the title ~~add pseudoElement option to constructor body~~ [web-animations-1] add pseudoElement option to constructor body Dec 21, 2019

stephenmcgruer reviewed Dec 21, 2019

View reviewed changes

Comment thread web-animations-1/Overview.bs Outdated

birtles requested changes Dec 23, 2019

View reviewed changes

Comment thread web-animations-1/Overview.bs Outdated

george-steel force-pushed the anim-pseudo-fix branch from 9d08a2e to 99b2845 Compare January 24, 2020 23:32

george-steel changed the title ~~[web-animations-1] add pseudoElement option to constructor body~~ [web-animations-1] Resolve remaining issues missed in pseudo-element change. Jan 24, 2020

stephenmcgruer requested changes Jan 27, 2020

View reviewed changes

Comment thread css-animations-2/Overview.bs Outdated

Comment thread web-animations-1/Overview.bs

Comment thread web-animations-1/Overview.bs

Comment thread web-animations-1/Overview.bs Outdated

review fixes round 1

42d8470

birtles requested changes Jan 28, 2020

View reviewed changes

george-steel added 3 commits January 29, 2020 16:45

revert bad fixes

5ae51f0

round 2 fixes

ce8566d

Add abort condition to effect value

476453e

birtles approved these changes Jan 30, 2020

View reviewed changes

stephenmcgruer approved these changes Jan 30, 2020

View reviewed changes

fix brian's nits

9b22aa5

stephenmcgruer merged commit c03272e into w3c:master Jan 30, 2020

Uh oh!

Conversation

george-steel commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

george-steel commented Dec 19, 2019

Uh oh!

Uh oh!

Uh oh!

birtles commented Jan 22, 2020

Uh oh!

george-steel commented Jan 23, 2020

Uh oh!

george-steel commented Jan 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

birtles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

birtles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

george-steel commented Dec 19, 2019 •

edited

Loading