Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[web-animations-1] [scroll-animations-1] Support automatic duration scroll animations #4862 #4890
[web-animations-1] [scroll-animations-1] Support automatic duration scroll animations #4862 #4890
Changes from 8 commits
c7bd477
d2902ce
37e981f
9720857
46b051c
2449252
3928ae8
fc24414
d148415
5324d25
c632aa9
b4a2b2a
e572251
e713294
8c19960
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this lose information? i.e. it doesn't roundtrip and it's not possible to tell whether 1.0 represents a percent or a millisecond time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's expected that getComputedTiming can lose information though, right? I thought we had settled on the progress being a double in the [0-1] range in our previous discussions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you just required the duration to be a percentage, do you need to mention non-auto?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just required the timeline duration to be a percentage, however the non-auto duration refers to the animation duration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should update the paragraph before this, and then describe the syntax for percentages (and how they are encoded as strings).
Also, it seems like the duration of a timeline can't be a percentage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my thinking was that internally we would recognize that the timeline duration is a percentage even though the value by inspection would be 1.0 (consistent with getComputedStyle). However, I'm guessing the issue is that if it's not represented in the WebIDL return value we can't recognize this.
Some options:
Any other ideas? Any recommendations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good point. I think it's probably helpful to ask what we want
getTiming()
andgetComputedTiming()
to return when we later introduce effects that are a percentage of their parent group's time (or ancestor timeline if we introduced fixed duration document timelines for example).I think in that case we might want something like:
getTiming().duration
to returnCSSNumberish
, i.e. adouble
in the case of a fixed time (for compatibility) or aCSSNumberValue
to represent a percentage.getComputedTiming().duration
to return a number which is either the resolved duration in ms (in part for compatibility, and in part so we can determine the actual duration for complex group effects) or a value between [0, 1] or [0, 100] to represent a percentage of an auto duration timeline.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, if we do the same for the timeline duration then we can know when a defined duration on the timeline is a time (where time-based values in the animations don't need to be dropped) or a percentage (where we convert time based animations to their respective proportions). I'll work on making this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we already specify that the return value from getTiming() must be auto if the duration is specified as auto so we should be returning the string 'auto' from getTiming which will round trip. I still like the idea of using CSSNumberish for the timeline duration though so that we clearly know whether the duration is time-based or progress (percentage) based.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider returning CSSNumberish from computed timing - that would sidestep the issue of converting the percentages to a double value. It will mean changing a lot of values to be CSSNumberish, but this might be cleaner in the long run. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure. Once we are able to use percentages that resolve to times (e.g.
duration
is 50% of my parent group) I wonder if it would be useful to havegetComputedTiming()
return the actual number of seconds it resolves to? In which case, should percentages that resolve against scroll timelines resolve to pixels?Unfortunately
getComputedStyle
doesn't give an entirely clear precedent here since it sometimes resolves percentages (e.g. forwidth
andheight
) but otherwise doesn't. I guess the intention is that it should always return percentages. So maybe we should return CSSNumberish instead?It would be interesting to know if anyone else has an opinion on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see this being useful, and might be worth doing.
This depends on what you consider to be the underlying value of the ScrollTimeline right? It could be pixels but it could also only operate in percentage of scroll or percentage of duration?
I think returning CSSNumberish and keeping percentages in the computed timing makes sense. One thing that I believe supports this is this note from css-cascade (emphasis mine):
cc/ @andruud
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving percentages intact sounds entirely reasonable.
Although depending on layout in this case is probably less painful than normal, given the snapshotting described in Avoiding cycles with layout, which means we can resolve the percentages against whatever is known from the previous snapshot?
Note that (from brief inspection of css-typed-om & Blink): there seems to be an "understanding" that a return value of
CSSNumberish
means you always get aCSSNumericValue
. So readonlyCSSNumberish
attributes would probably ideally beCSSNumericValue
attributes. So since this would be the first deviation from this (I think), we should probably specify under which circumstances we can expect a raw double.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable. If we were designing this from scratch I think we would always return a
CSSNumericValue
and it is only compatibility constraints that mean we sometimes need to return an actual number.