[web-animation-2] Making currentTime and startTime CSSNumerish #6479

kevers-google · 2021-07-28T15:00:31Z

[web-animation-2] Making currentTime and startTime CSSNumerish

The animation attributes currentTime and startTime were nullable doubles and represented times in units of milliseconds. These units do not align well with progress based animations where it is more natural to work in percentages.

With this change, progress based animations expect and report times as percentages. When setting current or start time, the value may be expressed as a double with implicit units of milliseconds, or as a CSSNumericValue with duration units ('s' or 'ms') when the animation is associated with a non-monotonic timeline.

issue #6458

web-animations-2/Overview.bs

flackr · 2021-07-28T15:54:21Z

web-animations-2/Overview.bs

+    [=validate a CSSNumberish time=] procedure with <var>seek time</var>
+    as the input.
+
+1.  If valid seek time is false, abort this procedure.


nit, <var>valid seek time</var>

flackr · 2021-07-28T15:54:42Z

web-animations-2/Overview.bs

+    [=validate a CSSNumberish time=] procedure with
+    <var>new start time</var> as the input.
+
+1.  If valid start time is false, abort this procedure.


nit: <var>valid start time</var>

web-animations-2/Overview.bs

flackr · 2021-08-04T13:53:35Z

web-animations-2/Overview.bs

+    [=progress-based timeline=], [=start time=] must be returned as a
+    <a href="https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue">
+    CSSNumericValue</a> with percent units. Otherwise [=start time=] must be
+    returned as a double value, representing the time in units of milliseconds.


It feels a little out of place to declare in the interface the types that must be returned. I would have expected these types to be the implicit result of starting an animation on or switching an animation to a progress based timeline.

CSSNumerish is naturally ambiguous since it could be a double or a CSSNumericValue, and when expressed as a CSSNumericValue it could have any compatible units (e.g. 's' vs 'ms'). Internally, it doesn't matter how we should express time, but should have a canonical form for API return values. Perhaps this belong in a normalization procedure that is called at the end of "getting the current/start time".

Perhaps this should be clarified in an addendum to https://www.w3.org/TR/web-animations-1/#time-value-section.

Yeah, this is why Typed OM never uses CSSNumberish for method return types, and when it uses it for an attribute, it's very explicit about what is returned by the getter.

Ack, sounds like it is appropriate to be explicit here about the type that should be expected.

flackr · 2021-08-05T16:25:54Z

web-animations-2/Overview.bs

+
+    :    If <em>all</em> of the following conditions are true:
+         * The animation is not associated with a [=progress-based timeline=],
+           amd


flackr · 2021-08-05T16:48:54Z

web-animations-2/Overview.bs

+         * The animation is not associated with a [=progress-based timeline=],
+           amd
+         * <var>time</var> is a CSSNumericValue:
+         * The units of time are not <a href="https://www.w3.org/TR/css3-values/#time">duration units</a>.


Links to other specs should use autolinks https://tabatkins.github.io/bikeshed/#autolink-biblio

E.g. This particular case would be [[CSS3-VALUES#time|duration units]]

Well, that would produce a section link, which is only very slightly less fragile than a direct link, as it just protects you against the possibility of the Values 3 url being changed at some point. You probably actually want <a type lt="<time>">duration units</a>, to link to the definition of those units whereever they may be.

(You should be able to write <<time|duration units>>, but I haven't yet landed the fix harmonizing the parsing of all linking shorthands, ugh.)

flackr · 2021-08-11T20:45:28Z

web-animations-2/Overview.bs

+ ::  Update the description as:
+>    The [=start time=] of this animation. When associated with a
+>    [=progress-based timeline=], [=start time=] must be returned as a
+>    <a href="https://drafts.css-houdini.org/css-typed-om/#cssnumericvalue">


Here and below, replace with {{CSSNumericValue}}.

flackr · 2021-09-01T14:05:47Z

web-animations-2/Overview.bs

+
+    :    If <em>all</em> of the following conditions are true:
+         * The animation is not associated with a [=progress-based timeline=],
+           and


This is an incorrect indent. Bikeshed requires a 4-space (or tab) indent.

flackr · 2021-09-01T14:08:34Z

web-animations-2/Overview.bs

+<div class='attributes'>
+ :   <dfn attribute for=Animation>startTime</dfn>
+ ::  Update the description as:
+>    The [=start time=] of this animation. When associated with a


Since this is nested in a list this requires more indentation, The '>' needs to be aligned with Update on the line above. There's an example with delay and endDelay below, e.g.

:: Update the description as: > The [=start time=] of this animation. When associated with a > [=progress-based timeline=], [=start time=] must be returned as a > {{CSSNumericValue}} with percent units. Otherwise [=start time=] must be

The list as a whole is indented incorrectly - for some reason it has a 1-space indent before each item, which doesn't do anything but does make the subsequent 4-space indents also misaligned. It should be indented one level from the container div, and then yeah, the blockquote would need two levels to nest it under the dd properly.

Just use tab indents; humans can't reliably space-indent to save their lives. ^_^

I don't have much experience with tab indenting specs but I'll take your word that it's simpler. I guess it takes away the temptation to use anything less than a full tab indent ^_^. I take it I should update #6565 (review) or follow up with another PR to do this for the whole spec and then we can update this to match.

flackr · 2021-09-01T14:09:17Z

web-animations-2/Overview.bs

+>    be returned as a double value, representing the time in units of
+>    milliseconds. Setting this attribute follows the procedure to
+>    <a>set the current time</a> of this object to the new value.
+</div>


Newline before to separate it from the block above.

flackr · 2021-09-01T14:13:19Z

web-animations-2/Overview.bs

+>    <a>set the start time</a> of this object to the new value.
+ :   <dfn attribute for=Animation>currentTime</dfn>
+ ::  update the description as:
+>    The <a>current time</a> of this animation. When associated with a


Same indentation issues.

kevers-google

Indent issues should all be resolved now. Formatting glitches with dl.switch have also been addressed.

kevers-google · 2021-09-01T16:28:47Z

web-animations-2/Overview.bs

+>    be returned as a double value, representing the time in units of
+>    milliseconds. Setting this attribute follows the procedure to
+>    <a>set the current time</a> of this object to the new value.
+</div>


flackr · 2021-09-14T20:32:41Z

web-animations-2/Overview.bs

+dl.switch > dt > ul > li {
+  text-indent: 0;
+}
+


@tabatkins there seems to be issues with the generated bikeshed css when we switched to dl instead of just the div. You can see the issue at this link which @kevers-google is trying to work around here. Do you know if there's something wrong with the spec formatting or generator?

Could you provide more details on what's wrong? I see that the "switch icon" is on its own line (unavoidable given the way it's injected, since the dt contains block elements), but it doesn't look like that's what your CSS is trying to mess with.

The bullets in the list are also incorrectly indented. See https://drafts.csswg.org/web-animations-1/#setting-the-current-time-of-an-animation for comparison. web-animations-1 seems to be using <div class="switch">: https://github.com/w3c/csswg-drafts/blob/main/web-animations-1/Overview.bs#L957

The switch being on it's own line was resolved by the rule:

dl.switch > [data-md] > p {
display: inline;
}

This aligns with a rule we have in web-animations-1, namely:

div.switch > dl > dt > p {
display: inline;
}

The overlap resulted from the following rules in base.css:

dl.switch > dt {
text-indent: -1.5em;
}

When applied to a paragraph inside a list item, it creates an overlap between the paragraph and the marker.

web-animation-1 controls the indent within a switch via these rules:

div.switch > dl > dt:before {
margin-left: -1.3em;
}

div.switch > dl {
padding-left: 2em;
}

Oh, huh, I see that base.css is doing some weird text-indent stuff. I... don't understand why it's doing that; it appears to only work well on single-line dts, and makes structured ones like this look weird. @fantasai, I don't remember if this style was imported from Bikeshed's stylesheet or done by you, any idea?

Moving the style changes to a separate PR to unblock the remainder of the changes.

Style changes moved to #6655.

flackr · 2021-09-14T20:34:34Z

web-animations-2/Overview.bs

+                evaluating
+                <code>|timeline time| - (|seek time| / [=playback rate=])</code>
+                where <var>timeline time</var> is the current <a>time value</a>
+                of <a>timeline</a> associated with <var>animation</var>.

    </dl>


Looks like the indentation level here doesn't match the opening tag.

CSSNumberish times

cb7a666

flackr reviewed Aug 4, 2021

View reviewed changes

Address feedback

ece9697

flackr requested changes Aug 5, 2021

View reviewed changes

Kevin Ellis added 2 commits August 5, 2021 13:47

Address feedback

891e9c8

Update link

c469ff1

flackr requested changes Aug 11, 2021

View reviewed changes

Update links

c2a5d8b

kevers-google mentioned this pull request Aug 31, 2021

[web-animations-2] Makes finish and cancel event times CSSNumberish #6563

Open

flackr requested changes Sep 1, 2021

View reviewed changes

Kevin Ellis added 5 commits September 1, 2021 12:22

Fix indent errors

830bb4d

Cleanup indent

af88852

Merge branch 'main' into numberish-current-and-start-time

523a18c

Fix indentation

41b828d

Formatting

393105f

kevers-google commented Sep 10, 2021

View reviewed changes

flackr reviewed Sep 14, 2021

View reviewed changes

Kevin Ellis added 2 commits September 15, 2021 14:46

Fix alignment of closing tag.

ee4cb3c

Move style changes to separate PR

1b0d12e

flackr approved these changes Sep 21, 2021

View reviewed changes

flackr merged commit 971196b into w3c:main Sep 21, 2021

[web-animation-2] Making currentTime and startTime CSSNumerish #6479

[web-animation-2] Making currentTime and startTime CSSNumerish #6479

Uh oh!

Conversation

kevers-google commented Jul 28, 2021

Uh oh!

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!

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!

tabatkins Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kevers-google 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!

flackr Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevers-google Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tabatkins Aug 5, 2021 •

edited

Loading

flackr Sep 14, 2021 •

edited

Loading

kevers-google Sep 15, 2021 •

edited

Loading