-
Notifications
You must be signed in to change notification settings - Fork 711
[scroll-animations-1] Remove -attachment, add timeline-scope #8902
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
Conversation
w3c#7759 This is specifically the model summarized in: w3c#7759 (comment)
Heh. I actually have a partially completed fix for this on my system from yesterday also. :) We can merge the differences... |
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.
Some comments from comparing diffs...
Value: [ <<'scroll-timeline-name'>> [ <<'scroll-timeline-axis'>> || <<'scroll-timeline-attachment'>> ]? ]# | ||
Value: [ <<'scroll-timeline-name'>> [ <<'scroll-timeline-axis'>> ]? ]# |
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.
Need to remove the now-extraneous brackets.
Initial: local | ||
<pre class='propdef shorthand'> | ||
Name: view-timeline | ||
Value: [ <<'view-timeline-name'>> [ <<'view-timeline-axis'>> ]? ]# |
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.
Same here: don't need the brackets anymore.
This [=view progress timeline=] name | ||
[=attaches=] to the corresponding [=view progress timeline=] | ||
defined on this box. | ||
# Deferred Progress Timelines # {#deferred-timeline} |
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.
Same top-level comment as earlier: I think I'd prefer to cast this as a name-scoping mechanism.
Also, this ultimately needs to go into css-animations-2, not here. Might make sense for us to just draft it there, or at least in the Appendix in a way that's prepared to be moved over.
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.
Yeah, I'm not actively trying to annoy you by going in circles. I was a definitely unsure what to do here, since the issue notes appear to show an unresolved situation where everyone except you resolved on this model #7759 (comment) 🙃. But after conferring with Tab, they thought writing that model would be fine despite that.
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 think the version in #8906 is functionally saying the same thing, just describing it differently. At least it's trying to ...
|
||
<pre class='propdef'> | ||
Name: timeline-scope | ||
Value: none | [ <<dashed-ident>> ]# |
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.
This is partially mixing up the dashed-ident change into the timeline-scope change, which leaves the spec in a weird inconsistent state.
|
||
<pre class="idl"> | ||
[Exposed=Window] | ||
partial interface Animation { | ||
attribute CSSNumberish? startTime; | ||
attribute CSSNumberish? currentTime; | ||
attribute AnimationTimeline? timeline; |
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.
Afaict this is an unrelated change, so it should go into its own issue/PR.
OK, I've pushed my version to #8906 |
Closing in favor of #8906. |
#7759
This is specifically the model summarized in:
#7759 (comment)