[cssom-view-1] Scroll methods in Element and Window return Promises

[mustaqahmed]

Switch the return type for the scroll methods from undefined to Promise<undefined> as per the discussion and resolution in #1562 (comment).

Fixed: #1562.

[flackr]

In the discussion we talked about the argument to the promise being an object which would tell the developer the result of the scroll, e.g. to know if it was cancelled / interrupted as was brought up a few times as a common thing to need to know.

[mustaqahmed]

In the discussion we talked about the argument to the promise being an object which would tell the developer the result of the scroll, e.g. to know if it was cancelled / interrupted as was brought up a few times as a common thing to need to know.

That's correct. The problem is that it not yet defined how the promise would be resolved to mark scroll completion vs interruption. I can update this PR to resolve with a true to mark completion and false "otherwise" but I am hesitant because of a few reasons:

1. It is perhaps not okay to add normative things w/o discussion, right? E.g. we don't know if we should use a Boolean here vs an enum.

2. What does that "otherwise" mean to be precise? Does one programmatic scroll request interrupt all pending promises? Or only the pending ones for the same scroller?

3. We may need additional text in the current scroll algorithms to "collect" pending scrolling requests. To me a follow-up PR after this one seems cleaner.

I am proposing to land this PR as "closing" #1562, staring a new spec issue to discuss 1+2 above, and then hash out 3 in the PR for that new issue.

[flackr]

In the discussion we talked about the argument to the promise being an object which would tell the developer the result of the scroll, e.g. to know if it was cancelled / interrupted as was brought up a few times as a common thing to need to know.

That's correct. The problem is that it not yet defined how the promise would be resolved to mark scroll completion vs interruption. I can update this PR to resolve with a true to mark completion and false "otherwise"

I would suggest that it not resolve with a boolean but rather with an object, e.g. define a ScrollResult, that we can add to if we later discover that we want to pass additional information. That said, having a boolean there and opening an issue for whether we should change it is a good thing.

but I am hesitant because of a few reasons:

1. It is perhaps not okay to add normative things w/o discussion, right? E.g. we don't know if we should use a Boolean here vs an enum.

This is normal, the resolution we have is effectively to pursue specifying and prototyping the feature. It's fully expected that in the process of doing this the implementor may make some opinionated decisions in writing the spec and bring those decisions back to the working group. Having something implemented will make it easier to have examples where we can show how the value would be used.

2. What does that "otherwise" mean to be precise? Does one programmatic scroll request interrupt all pending promises? Or only the pending ones for the same scroller?

It would be valuable to define this to mean what we think makes the most sense. IMO this is anything that prevents completing the original scroll request. A programmatic scroll request on an unrelated scroller probably should not interrupt it. A scroll on the same scroller would mean that we are no longer actively scrolling to the originally requested location.

3. We may need additional text in the current scroll algorithms to "collect" pending scrolling requests. To me a follow-up PR after this one seems cleaner.

I am proposing to land this PR as "closing" #1562, staring a new spec issue to discuss 1+2 above, and then hash out 3 in the PR for that new issue.

It is better to have the boolean there and to add an inline issue in the spec that says we need to agree on the type and when it is successful / unsuccessful. This shows spec readers that the API is likely to change, demonstrates the use case and can even point to the issue for people to add their feedback. Here's an example from css-images-5:

csswg-drafts/css-images-5/Overview.bs

Line 55 in b5312ce

ISSUE: Make sure behavior is <a href="https://github.com/w3c/csswg-drafts/issues/7058#issuecomment-1057553833">properly consistent for SVG</a>.

[mustaqahmed]

Updated the PR with promise resolution with true/false, and filed #12495 for next steps. Please take a look.

[mustaqahmed]

Here is a pdf showing the dependency of the scroll algorithms in this spec. Hopefully this will help the review here.

[emilio]

The boolean stuff is rather confusing. Do we want / is it worth resolving differently for completion vs interruption? Can't stuff technically scroll after the UA resolves the promise, kinda defeating the point of it?

[dlrobertson]

What is the relation of this new scroll promise to the scrollend event? And are there reasons to use one over the other?

FWIW scrollend happens to be in interop 2025. The original issue #1562 states a need to do the following:

let element = document.getElementById('scroll-container');
element.scroll(0, 400).then(() => {
   // ...scroll has finished
});

After interop 2025 completes, the following or something like it should be relatively reliable.

let element = document.getElementById('scroll-container');
let scrollPromise = new Promise(resolve => element.addEventListener("scrollend", resolve, {once: true}));
element.scroll(0, 400);
scrollPromise.then(() => {
   // ...scroll has finished
});

[mustaqahmed]

Below are my responses, sorry for the delay (I was on vacation for several weeks).

The boolean stuff is rather confusing. Do we want / is it worth resolving differently for completion vs interruption? Can't stuff technically scroll after the UA resolves the promise, kinda defeating the point of it?

@emilio I agree the original resolution does not include such details, and in our first patch we had the IDL type Promise<undefined>. But we found the spec language "resolve a Promise with true/false" far more expressive than "resolve a Promise (with nothing)". Moreover, this little addition gives the developers as easy way to test a successful call:

let scrolled = await elem.scrollBy(10);

I think we correctly prevented the possibility of scrolling after promise resolution through the last few steps in our #perform-a-scroll algorithm. Note that all other scroll algorithms ultimately rely on this one, and they never resolve any promise with true unless the innermost call does the same.

Please correct me if I missed any call dependency in the spec!

What is the relation of this new scroll promise to the scrollend event? And are there reasons to use one over the other?

@dlrobertson: When multiple scrollers are involved, writing a scrollend event based promise is non-trivial. For example, it is not easy to determine which elements would be affected by the call elem.scrollIntoView(), and then wait for all of them to emit scrollend events.

[emilio]

Sure, but promise resolution is async and, in your example scrolled is actually not really whether something scrolled, right? E.g a smooth scroll interrupted by the user will resolve with false but it's not clear what behavior the developer would want... It's more like completedScroll, and it seems rather subtle / hard to use right in presence of user scrolls or what not, right? Or am I missing something?

[flackr]

The boolean stuff is rather confusing. Do we want / is it worth resolving differently for completion vs interruption? Can't stuff technically scroll after the UA resolves the promise, kinda defeating the point of it?

@emilio I agree the original resolution does not include such details, and in our first patch we had the IDL type Promise<undefined>. But we found the spec language "resolve a Promise with true/false" far more expressive than "resolve a Promise (with nothing)".

During the discussion in which we took the resolution, this was raised as an important point, from #1562 (comment):

Rossen_: Meta point, I don't think anyone objects use case of promise pattern. I think the addition to this is adding some data that describes how this was fulfilled.
TabAtkins: I can see fulfill with an object that's an enum of completed or interrupted or something like that.

Sure, but promise resolution is async and, in your example scrolled is actually not really whether something scrolled, right? E.g a smooth scroll interrupted by the user will resolve with false but it's not clear what behavior the developer would want... It's more like completedScroll, and it seems rather subtle / hard to use right in presence of user scrolls or what not, right?

If a user scrolls before we reach the target then it immediately resolves with false. This is because while we have completed the scrolling that is going to happen for that requested scroll, it was interrupted. In the use cases in the OP it was suggested that developers would use this for things like:

it would be even more useful if scroll method returned a promise so the developer could wait until the scroll finishes before moving on to next set of tasks for things like triggering analytics data after scrolling, unloading resources when they've been scrolled out of view, parallax scrolling, etc.

Having this completion state allows the developer to do something different for interrupted scrolls without needing to read back the scroll offset and try to figure out whether it was interrupted which can be hard when using scrollIntoView to determine exactly what offset you should expect to scroll to.

Personally, I don't think it should be a boolean but rather a dictionary with an enum so that we could add additional arguments if we think of other things you might want to know about the completed scroll.

[mustaqahmed]

It's more like completedScroll, and it seems rather subtle / hard to use right in presence of user scrolls or what not, right? Or am I missing something?

You are correct, completedScroll would have been a better name in my hastily written example above.

Our discussion seems to deserve a separate resolution in the WG. If you agree, I would suggest moving this discussion over to #12495 for a wider audience and settle this PR in one of the following ways:

1. Land the PR as-is, quoting [cssom-view] How to fulfill programmatic scroll promises? #12495.
2. Change this PR to "always resolve with false or undefined", quoting that same issue.
3. Change this PR to resolve with an enum value which will be defined later on through that issue.

@emilio What do you think?

[emilio]

I'd probably resolve with undefined for now. Looks good with that. Thanks!

[mustaqahmed]

I have switched the PR to Promise<undefined>.

Note that #perform-a-scroll still needs a promise that resolves with a Boolean because both #viewport-perform-a-scroll and #scroll-a-target-into-view need to know when all affected scrolls have completed. Again, the dependency of the scroll algorithms would be helpful I believe.

[flackr]

Note that #perform-a-scroll still needs a promise that resolves with a Boolean because both #viewport-perform-a-scroll and #scroll-a-target-into-view need to know when all affected scrolls have completed. Again, the dependency of the scroll algorithms would be helpful I believe.

In this model, we shouldn't need the boolean at all. ScrollIntoView just needs to wait until the promises for each of the scrolled scrollers resolve. If another scroll interrupts one or more of those scrolls then those interrupted scroll promises will be resolved immediately.

[mustaqahmed]

In this model, we shouldn't need the boolean at all. ScrollIntoView just needs to wait until the promises for each of the scrolled scrollers resolve. If another scroll interrupts one or more of those scrolls then those interrupted scroll promises will be resolved immediately.

My question is: do the uninterrupted scrollers still continue their scrolling even after one got interrupted? This is about the observed behavior and not about the returned promise. To explain in a different way, suppose a ScrollIntoView call starts smooth scrolling in scrollers A, B and C, and then a B.scrollBy(0) call interrupts the smooth scrolling of B. Shouldn't A and C immediately stop scrolling?

[flackr]

My question is: do the uninterrupted scrollers still continue their scrolling even after one got interrupted? This is about the observed behavior and not about the returned promise. To explain in a different way, suppose a ScrollIntoView call starts smooth scrolling in scrollers A, B and C, and then a B.scrollBy(0) call interrupts the smooth scrolling of B. Shouldn't A and C immediately stop scrolling?

This is getting beyond the issue of when the promises should be resolved. Whether or not any of the ongoing scrolls is interrupted, the scrollIntoView call should resolve when all of the dependent scrolls' promises have resolved. If they are interrupted, then the overall promise may resolve sooner. We can separately debate how interrupting a scroll should affect ongoing scrolls but I don't see what that has to do with the promise resolution.

That said, this is a simple thing to test, I put together a test case https://codepen.io/flackr/pen/pvgJQxP . Chrome and Safari only interrupt the scroll of B, Firefox stops the scroll of B and C. I think that it doesn't make sense to stop scrolling A since you would still be able to scroll to the position that brings the B scroller into view. Stopping scrolling C (as firefox does) might make sense since it's no longer necessarily going to come into view. Though I could also see an argument for continuing it. Either way, this seems like a completely separate issue.

[mustaqahmed]

Yeah, my comment about the observed behavior should have been in #12495.

Done using the simplest possible promise resolution logic, see my latest commit.

[flackr]

A couple minor changes but otherwise looks good.

[mustaqahmed]

@emilio Please let me know if you would like to see any other changes.