Skip to content
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

[scroll-animations-1] Remove -attachment, add timeline-scope #8902

Closed
wants to merge 1 commit into from

Conversation

andruud
Copy link
Member

@andruud andruud commented Jun 1, 2023

#7759

This is specifically the model summarized in:
#7759 (comment)

@fantasai
Copy link
Collaborator

fantasai commented Jun 1, 2023

Heh. I actually have a partially completed fix for this on my system from yesterday also. :) We can merge the differences...

Copy link
Collaborator

@fantasai fantasai left a 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...

Comment on lines -452 to +374
Value: [ <<'scroll-timeline-name'>> [ <<'scroll-timeline-axis'>> || <<'scroll-timeline-attachment'>> ]? ]#
Value: [ <<'scroll-timeline-name'>> [ <<'scroll-timeline-axis'>> ]? ]#
Copy link
Collaborator

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'>> ]? ]#
Copy link
Collaborator

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}
Copy link
Collaborator

@fantasai fantasai Jun 1, 2023

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.

Copy link
Member Author

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.

Copy link
Collaborator

@fantasai fantasai Jun 2, 2023

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>> ]#
Copy link
Collaborator

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;
Copy link
Collaborator

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.

@fantasai
Copy link
Collaborator

fantasai commented Jun 1, 2023

OK, I've pushed my version to #8906
Lmk what you think.

@andruud
Copy link
Member Author

andruud commented Jun 5, 2023

Closing in favor of #8906.

@andruud andruud closed this Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants