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

[css-scroll-snap] Multiple nested scrollers and a "default" scrollIntoView()? #2593

Closed
tabatkins opened this Issue Apr 17, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@tabatkins
Member

tabatkins commented Apr 17, 2018

Our intention in #1708 is that scrollIntoView()'s alignment arg is honored if specified, but if it's unspecified we honor scroll-snap; only when both are unspecified do we use the "default" sIV() alignment.

What do we do when the targetted element is nested into multiple scrollable elements, tho? Each one of them needs to be scrolled to bring the element into view. When sIV() doesn't specify a particular alignment, how should we treat all the nested scrollers? In particular, what if some of them have scroll-snap specified, while others don't?

Honoring scroll-snap on the scrollers that have it, and default-aligning the rest, seems like probably the best behavior, but it's also significantly more complicated for implementations, as they have to collect and track more data as they execute the scroll operation. What should we do?

@majido

This comment has been minimized.

Contributor

majido commented Apr 25, 2018

The proposed solution seems reasonable though as pointed out it adds complexity.

This means that we are now allowing snap and align to be simultaneously honored depending on the container. This is fine but it is odd that the only possible alignment option is the default alignment. I think this is not very user friendly and provides for an odd API.

This issue is actually caused by the original resolution which makes snap application depend on presence of alignment.

I also have a second concern with original proposal. The way it works if a user specifies an alignment we
ignore the snap position. But that means the next user scroll will snap. I understand that this may be the
desired behavior in some usecases but I don't feel escaping snapping and this secondary snap induced scroll is the right default.

Alternative proposal

Since the spec has not been updated and no one has implemented then I suppose we have a chance to reconsider the original resolution. Here is an alternative idea:

Introduce a snap property to ScrollIntoViewOptions that explicitly controls snapping behavior independent of the alignment. By default it is true but then it can be set to false to disable snapping.

dictionary ScrollIntoViewOptions : ScrollOptions {
  ScrollLogicalPosition block = "start";
  ScrollLogicalPosition inline = "nearest";
  boolean snap = true; 
};

I think giving authors an explicit way to disable snapping provides a better API. Also it is better suited if in future we decide to provide a similar option for other scrolling API (e.g., focus(), scrollBy()).

So in this particular case:

  • If author has explicitly requested disabling snap via snap:false we don't snap any ancestor container and apply the requested alignment (which can be other than default).
  • Otherwise, snap for ancestors that are snap containers, and align for ancestors that are not based on the specified or the default alignment.
@tabatkins

This comment has been minimized.

Member

tabatkins commented Apr 25, 2018

If you want to disable snapping, we already have an API for that - just set scroll-snap-type: none on the container. We don't need to add more ways to do this, I don't think; in particular, I think it's quite weird to have a scrolling operation have the side effect of also permanently disabling snapping for the container.

Overall I'm kinda confused tho - you introduce the proposal with the problem that an aligned sIV() won't honor the snap, but the next scroll will; but then your proposed addition just addresses the original problem in my first post, that of what to do about intermediate snap containers during an unaligned sIV().

(Also, boolean options should default to falsey values, by convention. So if we did do this, we'd want to reverse the naming/meaning of the flag.)

@majido

This comment has been minimized.

Contributor

majido commented Apr 26, 2018

If you want to disable snapping, we already have an API for that - just set scroll-snap-type: none on the container. We don't need to add more ways to do this, I don't think; in particular, I think it's quite weird to have a scrolling operation have the side effect of also permanently disabling snapping for the container.

I should have been more clear. This is only disabling snapping for the scrollIntoView() operation only allowing the author to explicitly ask for their specified alignment to win when it is in conflict with snapping. It should not permanently disable snapping for those reason you pointed out.

Overall I'm kinda confused tho - you introduce the proposal with the problem that an aligned sIV() won't honor the snap, but the next scroll will; but then your proposed addition just addresses the original problem in my first post, that of what to do about intermediate snap containers during an unaligned sIV().

The general problem that escaping snapping is going to be reverted on next scroll is unavoidable IMHO if we were to allow any form of scroll snapping. The proposed solution at least helps by making it so that the default behavior does not escape snapping. The author has to explicitly ask for it.

Original resolution meant that by default any scrollIntoView() with alignment will trigger the above whether it actually wanted to escape snapping or not.

(Also, boolean options should default to falsey values, by convention. So if we did do this, we'd want to reverse the naming/meaning of the flag.)

Good to know 😃. So perhaps preventSnap : false which is similar to preventScroll: false that exists in FocusOptions?

@tabatkins

This comment has been minimized.

Member

tabatkins commented Apr 26, 2018

Ah, okay, then yeah, {preventSnap} in the ScrollOptions sounds good.

@fantasai

This comment has been minimized.

Contributor

fantasai commented Apr 27, 2018

I do want to point out that one of the design constraints we agreed on was that script cannot put a scroller into a scroll position that the user cannot then return to.

@tabatkins

This comment has been minimized.

Member

tabatkins commented Apr 27, 2018

That is directly contradicting our other resolution that an explicit alignment directive in a scrolling method should align accordingly, overriding any scroll-snapping.

Script shouldn't put the scroller into a user-unreachable position by default, but when you explicitly request it, we will do so.

@fantasai

This comment has been minimized.

Contributor

fantasai commented Apr 27, 2018

No, the other resolution was that the explicit alignment directive would set the scroll target, but it would then be subject to any snapping constraints. If there's none that conflict, then it's fine, you get the alignment you asked for. But if you've got mandatory snapping in a way that conflicts, then it wins, just as if you manually scrolled to the same alignment the script specified and let go.

@majido

This comment has been minimized.

Contributor

majido commented May 1, 2018

I also read the original resolution differently as it meant that snap only wins when there is no alignment.

I am fine with fantasai's explanation above. It basically amounts to snap always be in effect which is consistent and easy to reason about.

I don't think we need to talk about wining at all rather snapping operates after all other scrolling operations so it always applies. In the case of sIV() if the alignment brings the scroll offset to
a location which is a valid snap point then the alignment remains otherwise we adjust it to honor snapping even if it means the alignment is ignored.

So effectively:

  1. Apply scroll operation (user scroll or programmatic scrolls e.g., scrollTo, scrollBy, scrollIntoView, etc.)
  2. Apply scroll snapping
@tabatkins

This comment has been minimized.

Member

tabatkins commented May 1, 2018

I'm down with that, it's just not how I interpreted the previous resolutions. ^_^

So yeah, that makes this stuff easier, then. Just scroll as normal, with all the nested scrollers aligned as normal, then snap some of them as needed.

@css-meeting-bot

This comment has been minimized.

Member

css-meeting-bot commented May 2, 2018

The Working Group just discussed Multiple nested scrollers and a "default" scrollIntoView()?, and agreed to the following resolutions:

  • RESOLVED: Have the default scrolling behavior for nested scrollers to have scroll snap
The full IRC log of that discussion <dael> Topic: Multiple nested scrollers and a "default" scrollIntoView()?
<dael> github: https://github.com//issues/2593
<dael> TabAtkins: I suspect it's resolved at this point, but making sure. majidvp and our other impl that are getting our scroll snap up to date found an interaction. Spec defines and interaction between scroll methods alignment and scroll snap. You try to align via howt he scroll method says and if you need to snap after you do.
<dael> TabAtkins: Nested scrollers when you have to go multi deep to get hte element in view. Conclusion is you first align all the scrollers as you go down according to scroll method alignment. If any scrollers have scroll snap you honor that to bring them into alignment. And that's about it.
<dael> TabAtkins: Unless anyone disagrees that's what we concluded in the thread which means no edits to the spec except we might want to clarify the wording.
<dael> Rossen_: Sounds reasonable. Any other opinions or objections?
<dael> prop: Have the default scrolling behavior for nested scrollers to have scroll snap
<dael> TabAtkins: Yeah, sure.
<dael> RESOLVED: Have the default scrolling behavior for nested scrollers to have scroll snap
@fantasai

This comment has been minimized.

Contributor

fantasai commented Jun 19, 2018

Marking needs edits for applying clarifications.

tabatkins added a commit that referenced this issue Jun 21, 2018

@fantasai fantasai removed the Needs Edits label Jul 3, 2018

@fantasai fantasai closed this Jul 3, 2018

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 17, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should always snap regardless of whether it has alignment
specified or not. And all the affected scrollers should also land on a
snap position if one exists.

This patch adds top_alignment to the WebScrollIntoViewParams to specify
the target element's snap alignment. We always retrieve the alignment
from top_alignment during scrollIntoView. Once a scroller has been
scrolled, the previous top_alignment is popped out and the variable is
updated with the alignment specified from scrollIntoView. If the element
doesn't have scroll_snap_align, the top_alignment is always the same
with the alignment from scrollIntoView.

Whenever a scrollable_area is added to the smoothScrollSequencer with
its new scroll_offset, we will check if it has a valid snap offset
around, and update the final offset accordingly.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 24, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should always snap regardless of whether it has alignment
specified or not. And all the affected scrollers should also land on a
snap position if one exists.

This patch adds top_alignment to the WebScrollIntoViewParams to specify
the target element's snap alignment. We always retrieve the alignment
from top_alignment during scrollIntoView. Once a scroller has been
scrolled, the previous top_alignment is popped out and the variable is
updated with the alignment specified from scrollIntoView. If the element
doesn't have scroll_snap_align, the top_alignment is always the same
with the alignment from scrollIntoView.

Whenever a scrollable_area is added to the smoothScrollSequencer with
its new scroll_offset, we will check if it has a valid snap offset
around, and update the final offset accordingly.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 24, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on asnap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 25, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on asnap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Oct 26, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d
Reviewed-on: https://chromium-review.googlesource.com/c/1188746
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607019}

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d
Reviewed-on: https://chromium-review.googlesource.com/c/1188746
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607019}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this issue Nov 9, 2018

Snap at ScrollIntoView.
According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d
Reviewed-on: https://chromium-review.googlesource.com/c/1188746
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607019}

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 14, 2018

Bug 1491960 [wpt PR 13038] - Snap at ScrollIntoView., a=testonly
Automatic update from web-platform-testsSnap at ScrollIntoView.

According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try​:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d
Reviewed-on: https://chromium-review.googlesource.com/c/1188746
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607019}

--

wpt-commits: a0610cf468e6208e74b2cfda77a565ab90fbd74e
wpt-pr: 13038

staktrace pushed a commit to staktrace/gecko that referenced this issue Nov 15, 2018

Bug 1491960 [wpt PR 13038] - Snap at ScrollIntoView., a=testonly
Automatic update from web-platform-testsSnap at ScrollIntoView.

According to the spec,
w3c/csswg-drafts#2593 (comment)
scrollIntoView should
1) Always snap to the target element's snap alignment, and
2) All the affected scrollers should also land on a snap position if
one exists.

This patch does part 2). Before a scrollable_area is added to
the smoothScrollSequencer with its new scroll_offset, we will check if
it has a valid snap offset around, and update the final offset
accordingly.

We'll implement part 1) in a separate patch.

Bug: 842317
Cq-Include-Trybots: luci.chromium.try​:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ica22134ed0b1f9d36f5a017c8dd0be54e907dd3d
Reviewed-on: https://chromium-review.googlesource.com/c/1188746
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Commit-Queue: Sandra Sun <sunyunjia@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607019}

--

wpt-commits: a0610cf468e6208e74b2cfda77a565ab90fbd74e
wpt-pr: 13038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment