[scroll-animations-1] Support for multiple scroll offsets #4912 #5803

ogerchikov · 2020-12-17T00:28:37Z

Described specification for ScrollTimeline.scrollOffsets API.

kevers-google

Sorry for the tardy response. Overall looks good. Deferring to Rob and Anders for approval.

andruud

( One month later...: )

andruud · 2021-01-15T12:55:46Z

scroll-animations-1/Overview.bs

 <pre class='descdef'>
-  Name: start
+  Name: scrollOffsets


This should just be offsets in my opinion, and in any case definitely not camelCase.

How about scroll-offsets to match Javascript definition of ScrollTimeline?

Seems somewhat unnecessary to have the scroll- prefix when we're already inside @scroll-timline. IIRC we had scroll-time-range etc before, but the prefix was dropped.

But, if there are reasons for keeping the scroll- prefix on scroll-offsets specifically, then scroll-offsets WFM.

There was a concern that offset has a different meaning in animation context, to specify the position of each keyframe. We may in fact want to add offsets as well as scrolloffsets in the future so that you could map scroll offsets to arbitrary animation offsets rather than having them uniformly distributed.

Changed to scroll-offsets.

I see, makes sense.

scroll-animations-1/Overview.bs

andruud · 2021-01-15T13:02:48Z

scroll-animations-1/Overview.bs

+    1.  Run the procedure to [=resolve a scroll timeline offset=] for 'auto'
+        with the <var>is first</var> flag set to <var>first offset</var> and
+        add the resulted value into <var>effective scroll offsets</var>.
+1.  If {{ScrollTimeline/scrollOffsets}} has one element


Should this say:

If {{ScrollTimeline/scrollOffsets}} has at least one element?

This describes how to handle single scroll offset - add 'auto' as a first element, the provided offset will be added as part of the next statement.

Right. I did understand that at some point, then I confused myself.

Then probably this should say:

If {{ScrollTimeline/scrollOffsets}} has exactly one element

Just to make it clear that we're not intending the "at least" meaning. (Which would prepend 'auto' for any non-empty list I guess).

scroll-animations-1/Overview.bs

andruud · 2021-01-15T13:34:56Z

scroll-animations-1/Overview.bs

+    [=resolve scroll timeline offsets=] for {{scrollOffsets}}.
+1.  Let <var>offset id</var> correspond to the position of the last offset in
+    <var>scroll offsets</var> whose value is less than or equal to
+    <var>offset</var> and the value at the following position greater than


Does this mean that the specified scroll offsets have ordering constraints? If yes, what are they, and when/how are they enforced?

There is no ordering constrains. Explanation why last interval is chosen is explained in the note below the section. I added another note explaining why matching interval will always be found.

scroll-animations-1/Overview.bs

ogerchikov · 2021-01-15T23:33:26Z

Thanks @andruud for reviewing! I addressed your comments besides the one which refers to scrollOffsets name. Before I make the change I would like to get consensus from other reviewers. Please take a look at the latest patch.

flackr · 2021-01-27T15:41:14Z

scroll-animations-1/Overview.bs

@@ -294,8 +288,7 @@ if (window.matchMedia('(prefers-reduced-motion: no-preference)').matches) {
  var scrollTimeline = new ScrollTimeline({
    source: scrollableElement,
    orientation: "vertical",
-    scrollOffset: '0px',
-    end: '200px'
+    scrollOffsets: '0px', '200px'


This should be [CSS.px(0), CSS.px(200)] shouldn't it?

scroll-animations-1/Overview.bs

flackr · 2021-01-27T16:41:53Z

scroll-animations-1/Overview.bs

 <pre class='descdef'>
-  Name: start
+  Name: scrollOffsets


There was a concern that offset has a different meaning in animation context, to specify the position of each keyframe. We may in fact want to add offsets as well as scrolloffsets in the future so that you could map scroll offsets to arbitrary animation offsets rather than having them uniformly distributed.

flackr · 2021-01-27T19:52:16Z

scroll-animations-1/Overview.bs

@@ -156,15 +156,13 @@ Using the CSS markup:
  @scroll-timeline collision-timeline {
    source: selector(#container);
    orientation: block;
-    start:  200px;
-    end: 300px;
+    scrollOffsets:  200px, 300px;


nit: remove double space before offset.

scroll-animations-1/Overview.bs

flackr · 2021-01-27T20:08:00Z

scroll-animations-1/Overview.bs

+calculation.
+
+Note: The rationale behind choosing last matching offset in the array is to
+be consistent with CSS property overrides.


I think we should link to the rule for finding which KeyframeRule applies in a comma separated list of keyframes with overlap as justification: https://drafts.csswg.org/css-animations-1/#interface-csskeyframesrule-findrule

ogerchikov · 2021-01-28T19:28:02Z

Thanks for reviewing! All your comments are addressed in the recent commit.

birtles

If you've got review from @kevers-google, @andruud, and @flackr then I'm pretty sure you don't need mine! (I did skim it however, and it looked very thorough.)

foolip · 2021-02-13T23:04:41Z

Was there a Chromium bug filed for this? CSSScrollTimelineRule was changed to remove start and end properties, but those are still in Chromium.

ogerchikov · 2021-02-13T23:37:04Z

@foolip Yes, https://bugs.chromium.org/p/chromium/issues/detail?id=1094014 includes removal of start and end properties.

foolip · 2021-02-13T23:39:52Z

Thanks @ogerchikov! I noticed the removal when updating IDL for https://mdn-bcd-collector.appspot.com/, and wasn't sure if I should add back some custom tests for this or not. I won't then, if it's not going to be left long in Chromium.

Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8

Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <kevers@chromium.org> Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Cr-Commit-Position: refs/heads/master@{#855469}

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <kevers@chromium.org> Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Cr-Commit-Position: refs/heads/master@{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: ea854b5dc8b4d98887c5b22aaee2336cc02a7668

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <kevers@chromium.org> Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Cr-Commit-Position: refs/heads/master@{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: 9761161af35192bf75436da4a6a9959898d293dc

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: 526bca8065caa2644809862cdaedf2b34702e9d6

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: 9761161af35192bf75436da4a6a9959898d293dc

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: 526bca8065caa2644809862cdaedf2b34702e9d6

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: 9761161af35192bf75436da4a6a9959898d293dc

…th scrollOffsets attribute, a=testonly Automatic update from web-platform-tests Replaced start and end scroll offsets with scrollOffsets attribute Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <keverschromium.org> Commit-Queue: Olga Gerchikov <gerchikomicrosoft.com> Cr-Commit-Position: refs/heads/master{#855469} -- wpt-commits: 69bf427d7d2ae83f7c60ef6751ddf0e26d2892fa wpt-pr: 27665 UltraBlame original commit: 526bca8065caa2644809862cdaedf2b34702e9d6

Made modifications spec'ed in w3c/csswg-drafts#5803: - Removed startOffset and endOffset attributes. - Updated procedures based on spec changes. Bug: 1094014 Change-Id: Id08c9ce1818bd561ac291ca3e1f99c22dbb15674

Made modifications spec'ed in w3c/csswg-drafts#5803: - Removed startOffset and endOffset attributes. - Updated procedures based on spec changes, including allowing single scroll offset of 'auto'. Bug: 1094014 Change-Id: Id08c9ce1818bd561ac291ca3e1f99c22dbb15674

Made modifications spec'ed in w3c/csswg-drafts#5803: - Removed startOffset and endOffset attributes. - Updated procedures based on spec changes, including allowing single scroll offset of 'auto'. Bug: 1094014 Change-Id: Id08c9ce1818bd561ac291ca3e1f99c22dbb15674 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2801290 Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#874955}

…erties of ScrollTimeline, a=testonly Automatic update from web-platform-tests Removed scroll start and end offset properties of ScrollTimeline Made modifications spec'ed in w3c/csswg-drafts#5803: - Removed startOffset and endOffset attributes. - Updated procedures based on spec changes, including allowing single scroll offset of 'auto'. Bug: 1094014 Change-Id: Id08c9ce1818bd561ac291ca3e1f99c22dbb15674 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2801290 Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#874955} -- wpt-commits: 44d0f24ba4dc413c7142bdb7d1daf9186bbd680d wpt-pr: 28358

Based on pr[1] replaced all usages of startScrollOffset and endScrollOffset with scrollOffsets attribute. Among the changes; -Moved content of constructor.tentative.html to constructor.html. -Deleted constructor.tentative.html [1] w3c/csswg-drafts#5803 Bug: 1094014 Change-Id: I3d609555bdd7c65b2b9561a88d4c130a5566a9a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2701333 Reviewed-by: Kevin Ellis <kevers@chromium.org> Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Cr-Commit-Position: refs/heads/master@{#855469} GitOrigin-RevId: 60f1bc9ae64a4412def6fdc67ac41b95ab5739aa

Made modifications spec'ed in w3c/csswg-drafts#5803: - Removed startOffset and endOffset attributes. - Updated procedures based on spec changes, including allowing single scroll offset of 'auto'. Bug: 1094014 Change-Id: Id08c9ce1818bd561ac291ca3e1f99c22dbb15674 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2801290 Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com> Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: Kevin Ellis <kevers@chromium.org> Cr-Commit-Position: refs/heads/master@{#874955} GitOrigin-RevId: c9eb819fca15eca5ba6409ed4135247cbcce2bf5

ogerchikov added 2 commits December 16, 2020 15:49

Initial revision

963f06b

Fixed formatting.

191ec51

ogerchikov requested review from andruud, flackr and kevers-google December 17, 2020 00:33

kevers-google reviewed Jan 5, 2021

View reviewed changes

andruud reviewed Jan 15, 2021

View reviewed changes

Addressed review comments.

7ef87e1

flackr reviewed Jan 27, 2021

View reviewed changes

Addressed review comments.

6e5ed35

ogerchikov requested a review from birtles January 28, 2021 19:34

birtles approved these changes Jan 29, 2021

View reviewed changes

andruud approved these changes Jan 29, 2021

View reviewed changes

flackr approved these changes Feb 1, 2021

View reviewed changes

ogerchikov merged commit aa78c7b into w3c:master Feb 2, 2021

chromium-wpt-export-bot mentioned this pull request Feb 17, 2021

Replaced start and end scroll offsets with scrollOffsets attribute web-platform-tests/wpt#27665

Merged

ogerchikov mentioned this pull request Mar 5, 2021

Implement support for multiple scroll offsets. flackr/scroll-timeline#28

Merged

chromium-wpt-export-bot mentioned this pull request Apr 2, 2021

Removed scroll start and end offset properties of ScrollTimeline web-platform-tests/wpt#28358

Merged

bramus mentioned this pull request Apr 22, 2021

[scroll-animations] Support series of scroll offsets #4912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[scroll-animations-1] Support for multiple scroll offsets #4912 #5803

[scroll-animations-1] Support for multiple scroll offsets #4912 #5803

ogerchikov commented Dec 17, 2020

kevers-google left a comment

andruud left a comment

andruud Jan 15, 2021

ogerchikov Jan 15, 2021

andruud Jan 19, 2021

flackr Jan 27, 2021

ogerchikov Jan 28, 2021

andruud Jan 29, 2021

andruud Jan 15, 2021

ogerchikov Jan 15, 2021 •

edited

Loading

andruud Jan 19, 2021

ogerchikov Jan 28, 2021

andruud Jan 15, 2021

ogerchikov Jan 15, 2021

ogerchikov commented Jan 15, 2021

flackr Jan 27, 2021

ogerchikov Jan 28, 2021

flackr Jan 27, 2021

flackr Jan 27, 2021

ogerchikov Jan 28, 2021

flackr Jan 27, 2021

ogerchikov Jan 28, 2021

ogerchikov commented Jan 28, 2021

birtles left a comment

foolip commented Feb 13, 2021

ogerchikov commented Feb 13, 2021

foolip commented Feb 13, 2021

[scroll-animations-1] Support for multiple scroll offsets #4912 #5803

[scroll-animations-1] Support for multiple scroll offsets #4912 #5803

Conversation

ogerchikov commented Dec 17, 2020

kevers-google left a comment

Choose a reason for hiding this comment

andruud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogerchikov Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogerchikov commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogerchikov commented Jan 28, 2021

birtles left a comment

Choose a reason for hiding this comment

foolip commented Feb 13, 2021

ogerchikov commented Feb 13, 2021

foolip commented Feb 13, 2021

ogerchikov Jan 15, 2021 •

edited

Loading