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

[cssom-view-1] Update spec text for visual viewport scroll and scrollend events #8205

Merged
merged 6 commits into from
Apr 24, 2023

Conversation

bokand
Copy link
Contributor

@bokand bokand commented Dec 8, 2022

[cssom-view-1] Update spec text for visual viewport scroll and scrollend events #8103

Adds normative text for firing the scrollend event on the VisualViewport.

scroll was already specified but incorrectly. The text as written meant that a scroll event would have to be dispatched at VisualViewport whenever the document scrolls, even if the VisualViewport didn't scroll.

This PR splits the perform a scroll steps into one for scrolling boxes (the existing one) and one for viewports (newly added). The viewport version distributes the scroll between the visual and layout viewports and the uses the scrolling box version to perform the scroll of each.

Fixes #8103

@bokand bokand changed the title Bokan visual viewport scroll events [cssom-view-1] Update spec text for visual viewport scroll and scrollend events #8103 Dec 8, 2022
@bokand bokand changed the title [cssom-view-1] Update spec text for visual viewport scroll and scrollend events #8103 [cssom-view-1] Update spec text for visual viewport scroll and scrollend events Dec 8, 2022
@bokand bokand requested a review from emilio December 8, 2022 19:00
Copy link
Contributor

@argyleink argyleink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very thorough! thanks so much, i learned a lot by reading this PR.

Issue: In what order are scrollend events dispatched? Ordered based on scroll start or scroll completion?

Great question. i initially thought first in first out, but upon more thought I think scroll completion to be better (first in last out). What were your thoughts?

@bokand
Copy link
Contributor Author

bokand commented Dec 8, 2022

Great question. i initially thought first in first out, but upon more thought I think scroll completion to be better (first in last out). What were your thoughts?

I think I'd lean that way too (FILO) though I don't have a strong opinion at this point - I'm unsure of what the possibilities are in other browsers. The way scrollend is written we flush the pending scrollend event target list whenever a scroll ends (which is also when we push to the list). In Chrome, a user scroll gesture "latches" to a single target and (at least at the time) we couldn't animate multiple scrollers simultaneously. Given that, the only case I can think of where you would have multiple targets in this list would be viewport scrolling (visual and layout viewports). In that case it maybe doesn't matter too much, as long as we're consistent with other browsers?

OTOH, I'm less sure if other browsers have similar constraints. Maybe I'm missing some cases?

When a user agent is to <dfn for="viewport" export>perform a scroll</dfn> of a <a>viewport</a> to a given position <var>position</var> and optionally a scroll behavior <var>behavior</var>
(which is "<code>auto</code>" if omitted) it must perform a coordinated viewport scroll by following these steps:

1. Let <var>doc</var> be the associated document of the <a>viewport</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other similar steps that do this use something like:

Let <var>doc</var> be the <a>viewport's</a> associated {{Document}}

Is there a reason to write it this way for this step?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, just missed it. Fixed in latest commit.

@bokand bokand force-pushed the bokanVisualViewportScrollEvents branch from 91b614f to c1f0b3c Compare December 22, 2022 17:53
@bokand
Copy link
Contributor Author

bokand commented Jan 17, 2023

Forgot I hadn't merged this yet...

@emilio: PTAL when you can

@zcorpan
Copy link
Member

zcorpan commented Feb 3, 2023

I have filed related bugs about the current spec text, see #8396 and #8397

Maybe they can be fixed separately after this PR, though.

@bokand bokand requested a review from smfr February 3, 2023 14:59
@bokand bokand force-pushed the bokanVisualViewportScrollEvents branch from c1f0b3c to d1e16b4 Compare March 8, 2023 21:53
@bokand
Copy link
Contributor Author

bokand commented Mar 8, 2023

@emilio or @smfr could either of you ptal?

Copy link
Contributor

@smfr smfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely, I had a "pending" comment from ages ago.

</ol>

Scroll is <dfn lt="scroll completed">completed</dfn> when the scroll position has no more pending updates or translations and the user has completed their gesture. Scroll position updates include smooth or instant mouse wheel scrolling, keyboard scrolling, scroll-snap events, or other APIs and gestures which cause the scroll position to update and possibly interpolate. User gestures like touch panning or trackpad scrolling aren't complete until pointers or keys have released.
When a user agent is to <dfn for="viewport" export>perform a scroll</dfn> of a <a>viewport</a> to a given position <var>position</var> and optionally a scroll behavior <var>behavior</var>
(which is "<code>auto</code>" if omitted) it must perform a coordinated viewport scroll by following these steps:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "coordinated" here imply?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in: we're coordinating a scroll distributed between the visual viewport's scrolling box and the document's viewport's (i.e. "layout viewport") scrolling box.

Basically "perform a scroll" for a scrolling box follows the previously defined steps while "perform a scroll" for a viewport is "overloaded" to perform this "coordination" by splitting up the scroll and calling "perform a scroll" on each of the viewport's scrolling boxes.

If "coordinated" isn't adding anything (other than confusion) I'm happy to remove it. Or perhaps give these steps a separate name (e.g. "perform a viewport scroll")?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smfr ping

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smfr PTAL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta comment: I think it might be clearer to refer to a “pan” when moving the visual viewport, and a “scroll” when moving the layout viewport (even though this text talks about firing a “scroll” event on the visual viewport).

When the visual viewport moves without a corresponding layout viewport change, the scrollbar doesn’t change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how would that work in terms of spec text? We're calling the "perform a scroll" steps in both cases. I could add a "perform a pan" but it would just call "perform a scroll" which might be confusing?

FWIW - in Chrome the visual viewport does affect the scrollbar. Zooming in shrinks the scrollbar and panning the visual viewport does move the scrollbars.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just mentioning for completeness, Firefox's scrollbars also reflect the visual viewport, i.e. shrink when zooming in and move when the visual viewport is scrolled.)

Copy link
Contributor

@smfr smfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with the edit as is, but please consider my suggestions.

</ol>

Scroll is <dfn lt="scroll completed">completed</dfn> when the scroll position has no more pending updates or translations and the user has completed their gesture. Scroll position updates include smooth or instant mouse wheel scrolling, keyboard scrolling, scroll-snap events, or other APIs and gestures which cause the scroll position to update and possibly interpolate. User gestures like touch panning or trackpad scrolling aren't complete until pointers or keys have released.
When a user agent is to <dfn for="viewport" export>perform a scroll</dfn> of a <a>viewport</a> to a given position <var>position</var> and optionally a scroll behavior <var>behavior</var>
(which is "<code>auto</code>" if omitted) it must perform a coordinated viewport scroll by following these steps:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta comment: I think it might be clearer to refer to a “pan” when moving the visual viewport, and a “scroll” when moving the layout viewport (even though this text talks about firing a “scroll” event on the visual viewport).

When the visual viewport moves without a corresponding layout viewport change, the scrollbar doesn’t change.

1. <a for="/">Perform a scroll</a> of <var>vv</var>'s <a>scrolling box</a> to its current scroll position + (<var>visual dx</var>, <var>visual dy</var>) with <var>element</var> as the associated
element, and <var>behavior</var> as the scroll behavior.

Note: When performing a coordinated scroll, the user agent distributes the scroll between the visual and layout viewports as needed. It computes the required total delta and applies it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mental model is that the visual viewport “bumps up against” the edge of the layout viewport, pushing it around. This is effectively the same as this “distribute the scroll”, but perhaps could lead to a cleaner description of the algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be totally awesome for the spec to have a nice animation showing how visual and layout viewports interact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to word this note with that visual in mind.

I also took a crack at an animation (added at the top, when introducing the visual viewport) and referenced it here. I'm a terrible artist but hopefully it gets the point across.

I haven't seen it before - is it ok to embed SVG directly in the spec text?

Copy link
Contributor Author

@bokand bokand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL?

</ol>

Scroll is <dfn lt="scroll completed">completed</dfn> when the scroll position has no more pending updates or translations and the user has completed their gesture. Scroll position updates include smooth or instant mouse wheel scrolling, keyboard scrolling, scroll-snap events, or other APIs and gestures which cause the scroll position to update and possibly interpolate. User gestures like touch panning or trackpad scrolling aren't complete until pointers or keys have released.
When a user agent is to <dfn for="viewport" export>perform a scroll</dfn> of a <a>viewport</a> to a given position <var>position</var> and optionally a scroll behavior <var>behavior</var>
(which is "<code>auto</code>" if omitted) it must perform a coordinated viewport scroll by following these steps:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, how would that work in terms of spec text? We're calling the "perform a scroll" steps in both cases. I could add a "perform a pan" but it would just call "perform a scroll" which might be confusing?

FWIW - in Chrome the visual viewport does affect the scrollbar. Zooming in shrinks the scrollbar and panning the visual viewport does move the scrollbars.

1. <a for="/">Perform a scroll</a> of <var>vv</var>'s <a>scrolling box</a> to its current scroll position + (<var>visual dx</var>, <var>visual dy</var>) with <var>element</var> as the associated
element, and <var>behavior</var> as the scroll behavior.

Note: When performing a coordinated scroll, the user agent distributes the scroll between the visual and layout viewports as needed. It computes the required total delta and applies it
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to word this note with that visual in mind.

I also took a crack at an animation (added at the top, when introducing the visual viewport) and referenced it here. I'm a terrible artist but hopefully it gets the point across.

I haven't seen it before - is it ok to embed SVG directly in the spec text?

@bokand bokand merged commit a64a85b into w3c:main Apr 24, 2023
@bokand
Copy link
Contributor Author

bokand commented Apr 24, 2023

I'll merge this as-is but happy to make any changes in the future.

@theres-waldo
Copy link
Contributor

Sorry for being late to this discussion. I have a question about this change:

VisualViewport has both pageLeft/pageTop and offsetLeft/offsetTop properties. IIUC this change is only considering the visual viewport to have scrolled when offsetLeft or offsetTop change, and not in situations when pageLeft or pageTop change but offsetLeft and offsetTop remain the same.

Should we be concerned about this breaking sites that might currently be listening to visual viewport scroll events as a convenient way of being notified whenever pageLeft or pageTop change (without also employing a document scroll listener), who would now need to switch to using both visual viewport and document scroll listeners?

@bokand
Copy link
Contributor Author

bokand commented Apr 24, 2023

Your understanding is correct but the spec now matches how Chrome and Safari behave (though I see Firefox does fire it for document scrolls as well).

It was intended that scroll is fired at VisualViewport only when its offsetTop or offsetLeft values change - I checked the draft spec and it seems I made a mistake there (sorry!) as that text implies Firefox's behavior.

Given Chrome and Safari both avoid firing it on document scrolls this behavior is web compatible though.

@theres-waldo
Copy link
Contributor

Ah, I missed that this is the behaviour that Chrome and Safari have implemented from the start. Ok, that seems fine then, we'll update Firefox to match.

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.

[cssom-view] VisualViewport should probably also include onscrollend
6 participants