[cssom-view] VisualViewport should probably also include onscrollend

[dlrobertson]

Summary

The VisualViewport defines onscroll, but does not include onscrollend.

Implementation notes

Currently the scrollend implementations for Chrome and Firefox will fire a scrollend event for a user scroll that stays within the layout viewport to the Document, but should probably fire to Window.visualViewport instead. Currently this is a case where scrollend can be fired to the window without a preceding scroll.

[dlrobertson]

CC @argyleink

[argyleink]

if it has scroll events, it should have a scrollend event 👍🏻 i can add that to the spec

i'm not sure i 100% follow Currently this is a case where scrollend can be fired to the window without a preceding scroll though

[dlrobertson]

i can add that to the spec

Awesome! Thanks for the quick response!

i'm not sure i 100% follow Currently this is a case where scrollend can be fired to the window without a preceding scroll.

Sorry, I probably should have been more clear. I'm just making a note about the current implementations (Firefox and Chrome) here. Assuming a user:

1. pinch zoom to make the visual viewport smaller than the layout viewport
2. scroll but stay within the layout viewport

These steps would not trigger a scroll event to be fired to the window, but would trigger scroll events to be fired to the window.visualViewport to be possibly handled by VisualViewport.onscroll. Currently, both implementations of scrollend do not behave like the scroll event in the above scenario. The latest versions of Firefox and Chrome that I've tested fire scrollend events with the window as the target after step 2. After onscrollend is added to the VisualViewport, this should change.

[argyleink]

^ commit made which adds onscrollend to the visual viewport. let me know how I did and close this out if resolved 👍🏻

[dlrobertson]

Thanks! This does solve the issue of adding the handler, but I wonder if we need something in the actual firing section. E.g. scroll has:

If target is a Document, fire an event named scroll that bubbles at target and fire an event named scroll at the VisualViewport that is associated with target.

For scrollend, we only have:

If scrolling was done on a viewport, let doc be the viewport’s associated Document and target be the viewport. Otherwise, scrolling is done on an element and let doc be the element’s node document and target be the element.

I think we need to specify that in some cases we need to fire a scrollend event for the VisualViewport. The wording of the onscroll steps still confuse me a bit. I'm still not sure how they specify that in some cases we should only fire Document.scroll and in some cases only VisualViewport.scroll. I'm okay with splitting off this addition of some documenting text for firing the scrollend event into another bug though.

[argyleink]

I think we need to specify that in some cases we need to fire a scrollend event for the VisualViewport.

makes sense to me 👍🏻 i'll do some digging and come up with something good

[argyleink]

how's this?! 2e46f83

[dlrobertson]

how's this?! 2e46f83

I'm uncertain if this should be specified in scrolling vs. scrolling-events. My intuition leans towards adding steps to scrolling-events, but I'm a bit out of my depth here.

CC @bokand git blame indicates you authored 29eaa5c which added the scrolling event loop section for VisualViewport scroll events. Does this addition look reasonable for VisualViewport scrollend events?

[bokand]

+1, it'd be nice for this to be in normative text. I'm also not a spec expert but I think adding text in the "scrolling is completed" steps is probably correct.

I added similar text in run the scroll steps for the visual viewport:

If target is a Document, fire an event named scroll that bubbles at target and fire an event named scroll at the VisualViewport that is associated with target.

Rereading this now, I think this says that whenever the document is scrolled we should also fire scroll at the VisualViewport, even if the VisualViewport offset doesn't change. I think that's wrong, we should only fire scroll at VisualViewport if its offset changes.

I think we want to add VisualViewport to the pending scroll event targets:

Whenever a VisualViewport gets scrolled (whether in response to user interaction or by an API), the user agent must run these steps:...

And then the run scroll steps should be:

 * If target is a Document, fire an event named scroll that bubbles at target
 * If target is a VisualViewport, fire an event named scroll at it.

And then add similar language for the scrollend steps.

@argyleink - if you're already doing work here would you mind making these changes? If not, I could probably squeeze in a PR later this week.

[argyleink]

I could attempt to translate the thoughts you dropped above into the spec sure, but I might fall short somewhere.. I'm certainly interested in what you write and I agree with your above assertions, like "we should only fire scroll at VisualViewport if its offset changes". I'd love help, as I'm doing my best but can feel like i'm not super confident.

[bokand]

No problem. I should have some time tomorrow, will take a crack at it. Thanks!

[bokand]

Turns out I had less time than I thought :/

Anyway, I did finally get to it - please feel free to leave comments on the review!

[dlrobertson]

Anyway, I did finally get to it - please feel free to leave comments on the review!

Thanks for working on this! Read through once, but I'll have to read it a few times to get to the point where I'm comfortable leaving comments 😆