Skip to content

[cssom-view-1] Option for scrollIntoView that doesn't propagate to ancestors #9452

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

Closed
flackr opened this issue Oct 10, 2023 · 11 comments
Closed

Comments

@flackr
Copy link
Contributor

flackr commented Oct 10, 2023

The scrollIntoView API invokes the behavior to scroll an element into view which:

means to run these steps for each ancestor element or viewport that establishes a scrolling box scrolling box, in order of innermost to outermost scrolling box

However, in many circumstances it can be counter-intuitive that a scroll into view call scrolls ancestor scrollers. E.g. If a slideshow or carousel uses a scrolling box for slides and uses scrollIntoView to advance to the next slide, this will also force the top level scroller to scroll the slideshow into view. This is often surprising, especially when scrollIntoView is used as a convenience to automatically calculate the scroll position rather than using scrollTo on the scroller. @argyleink has examples where this has been problematic.

As such, I propose we add the ability to scroll an element into view that only scrolls the nearest scrolling box. There are two ways to do this:

  1. Add an option to ScrollIntoViewOptions, e.g. scroll = "nearest" | "ancestors" with a default of ancestors or specify a boundary scroller (happy to bikeshed names), or
  2. Add the necessary options to ScrollToOptions to scroll a particular element into view. E.g. we could provide a target and alignment options (similar to ScrollIntoViewOptions). It would be hard to define reasonable interactions when for example a target is specified and a left value is specified. Further, it would be odd when there is an intermediate scroller that does not have the element visible, do we scroll to where the element is outside of the intermediate scroller's scrollport or do we scroll to the intermediate scroller?

I think option 1 is simpler and a reasonable api.

@flackr
Copy link
Contributor Author

flackr commented Oct 10, 2023

Developers could also use scrollParent from #1522 if they wanted to for example scroll every scroller from the target to some particular ancestor. E.g.

function scrollElementIntoView(ancestor, target) {
  while (target && target != ancestor) {
    target.scrollIntoView({scroll: "nearest"});
    target = target.scrollParent;
  }
}

@argyleink
Copy link
Contributor

Watch a video reproducing the issue
scrollIntoView-issue.mp4

Try this demo here

Since scrollIntoView() does so much jumping and aggressive recursive scroll adjusting, the API was avoided entirely in this carousel demo, where I manually (and included logical directionality) used scrollTo().

As mentioned by Michael here on Twitter, scrollIntoView() as it currently is, is hardly usable.

I prefer option 1 as the solution 👍🏻
would make the API usable for me in a component driven environment

@bakura10
Copy link

Hi,

I am Michael from Twitter. The scrollIntoView API has indeed be problematic for us since the start, and most of the use cases where I wanted to use it, I could not because of the full page scroll.

I have developed a scroll-snap based carousel and due to our constraints (we are creating Shopify themes) we have to support a lot of different scenario: various alignments (start, end, center), support for scroll-margin/padding, taking into account borders, full support for RTL...

All of this is possible by using scrollTo and manually calculating the offset, but it quickly becomes extremely complex (especially RTL, where things like scroll offsets behave very strange). Having a way to simply reuse scrollIntoView and making sure the scroll does not propagate to the whole page would help to remove ton of code, and also make it much more robust.

Lately, I started using this library (https://github.com/scroll-into-view/compute-scroll-into-view) to calculate all the offset (I recently pushed a PR to add RTL support and it was messy) and apply scrollTo, and I really like the boundary setting, I think it could be used in a similar way:

slideCell.scrollIntoView({ inline: 'start', boundary: carousel });

By doing this, the scroll will propagate until the carousel itself and stop there. This is by far the most flexible approach and would fix all my grips with scrollIntoView :D.

@thunderfreak
Copy link

You can also do this to reset the window to the position it was before, once the item is scrolled into view.

const y = window.scrollY;
const x = window.scrollX;
if (isFocused && item.current) {
  item.current.scrollIntoView({ block: 'center' });
}
window.scroll(x, y);

@nickcoury
Copy link

I just ran into this. I have a navigational carousel at the top of the page that centers the clicked item and loads a new page. On back/forward button press, I center the previously selected item using { inline:"center" }. If the destination page was previously scrolled, the page will unexpectedly jump to the top and conflict with scroll restoration, which is disorienting.

It wouldn't be as flexible as other solutions here, but I found myself just wanting { inline: "center", block: "none" } to prevent scrolling in one direction but allow it in the other.

@flackr flackr changed the title [cssom-view-1] Provide option for scrollIntoView that only scrolls a single scroll container. [cssom-view-1] Option for scrollIntoView that doesn't propagate to root Oct 30, 2024
@flackr
Copy link
Contributor Author

flackr commented Nov 1, 2024

I propose we go with option 1, and for flexibility add a container option to ScrollIntoViewOptions:

dictionary ScrollIntoViewOptions : [ScrollOptions](https://www.w3.org/TR/cssom-view-1/#dictdef-scrolloptions) {
  ScrollLogicalPosition block = "center";
  ScrollLogicalPosition inline = "center";
  Element? container = null,
};

By default, the null container will scroll all scroll containers all the way to the root, however if container is specified, it will only scroll the containers from the target element up to container inclusive.

Additionally, I think it would be useful to add scrollParent so that you could easily get the scroll container for the target, e.g.

target.scrollIntoView({container: target.scrollParent});

@tabatkins
Copy link
Member

There's some corner cases to consider:

  1. What's the behavior if the indicated container isn't a scroll container?
  2. What's the behavior if the indicated element isn't an ancestor of the target?

We could make both of those be an error, but I think there's an easy option that solves both: find the nearest scroll container inclusive ancestor of the given "container" element which contains the target as a descendant.

This way, specifying a non-scrolling ancestor isn't an error, it'll just move up the tree until it finds something it can scroll. And specifying a non-ancestor isn't an error, it'll just move up the tree until it finds a common ancestor (possibly the root scroller).

(This also happens to avoid the dependency on .scrollParent, as you could just supply the target itself as the "container" to auto-select its nearest scrollable ancestor. But I do think we should pursue .scrollParent as well; it's useful for more than just this, and is less hacky-feeling for this use-case even if it would be technically unnecessary.)

@devongovett
Copy link
Contributor

This would be fantastic. We currently maintain a very complicated scroll into view function in React Aria because the native one causes the body to scroll when modals/popovers are open and scroll locking is enabled (the body has overflow: hidden). This is hard because we need to keep up with additions to CSS that can affect scrolling, such as scroll-padding and scroll-margin. Would be great to have a solution built into browsers.

I like the idea of a boundary element, and Tab's suggestions make sense to me.

@flackr
Copy link
Contributor Author

flackr commented Jan 14, 2025

@tabatkins I was originally imagining that anything "above" container would not be scrolled which would be similar but not enable passing the element itself to scroll its nearest container. This would likely be implemented by finding container's nearest scroll ancestor and stopping propagation at that node (notably not scrolling that node). What you've proposed is nice behavior though and I could see it solving other use cases as well (e.g. scroll up to the common ancestor scroll container of the target and the button i've clicked to make target visible).

@smfr smfr changed the title [cssom-view-1] Option for scrollIntoView that doesn't propagate to root [cssom-view-1] Option for scrollIntoView that doesn't propagate to ancestors Jan 22, 2025
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [cssom-view-1] Option for scrollIntoView that doesn't propagate to ancestors, and agreed to the following:

  • RESOLVED: enum ScrollIntoViewContainer { "all", "nearest" }, defaulting to `all`, expanding to element in the future if needed
The full IRC log of that discussion <joshtumath> flackr: scrollIntoView is a useful API
<joshtumath> ... does the right thing for common use cases. but propogates all the way up chain to scroll root
<joshtumath> ... so the proposal is to add a container property to the dict added to scrollIntoView
<joshtumath> ... it would use Tab's proposal here for the common ancestor about what to scroll into view
<joshtumath> ... the main reason was to make it easy to refer to your own scrolling ancestor.
<joshtumath> ... so you'd say container.target and it would refer to nearest ancestor
<emilio> q+
<joshtumath> ... I'm also fine with it just using the container. but then we have to answer what happens if the container is not actually an ancestor
<joshtumath> ... you could just do nothing because that's what you've asked for
<joshtumath> ... TabAtkin's suggestion is simple to use, but container is not quite the right name
<joshtumath> Rossen: the top scrolling ancestor of the container?
<smfr> q+
<TabAtkins> X.scrollIntoView(container:Y), finds the nearest common scollable ancestor of X and Y
<Rossen5> ack emilio
<joshtumath> emilio: I think I was going to drop a similar question. Ancestor in the DOM sense or layout tree sense?
<joshtumath> ... or maybe we need to... we have a switch to not scroll
<joshtumath> ... I think what you want is to scroll to the first scrollable thing
<joshtumath> ... let's say you have a fixedpos element
<joshtumath> ... it does not scroll with the rest of the content
<joshtumath> flackr: it would follow the scroll parent chain?
<joshtumath> emilio: a layout ancestor.
<joshtumath> ... so let's say if you scroll it on a popover.
<joshtumath> ... that seems reasonable, but container is a misnomer. maybe scroll-parent?
<joshtumath> ...but we have the scroll parent API
<bramus> https://github.com//issues/1522 is that issue
<joshtumath> flackr: we have a separate issue to add scroll parent to the element API
<joshtumath> ... we have offset parent, which is close
<joshtumath> emilio: Should we add something to skip over overflow: hidden stuff, but I can see you want both
<joshtumath> bramus: they should use overflow: clip in that case
<Rossen5> ack smfr
<joshtumath> smfr: I agree with emilio. Should shadow roots be a boundary as well?
<joshtumath> ... and at least in WebKit, scrolling goes across iframe boundaries
<emilio> q+
<joshtumath> ... it surprises me because ???
<joshtumath> flackr: I thought chrome changed that to not propergate across iframe boundaries.
<joshtumath> argyle: on codepen, the reproduce ability is available
<joshtumath> ... you'll see the iframe scroll into view and you'll know it's the child iframe. it's kind of the issue
<joshtumath> smfr: I have a feeling that's cross origin. maybe I'm thinking of jsbin
<joshtumath> bramus: yes I think so
<smfr> if Chrome did change the cross-origin iframe behavior, I'd like that to be raised in a CSS issue and specified
<joshtumath> Rossen: but the point is righteous. allowing to escape iframes or cross-origin iframes, sounds like reasonably in scope
<joshtumath> flackr: I think it's a different issue
<bramus> +1 on it being a different issue
<argyle> +1, two great different issues we should fix
<joshtumath> ... the issue of smfr is iframes, it's scrolling and hijacking frames that are unrelated.
<joshtumath> Rossen: but should be tackled in a separate issue. very related
<Rossen5> ack emilio
<joshtumath> emilio: so we could still at least agree that this doesn't work when the container you pass in comes from a separate document
<joshtumath> ... smfr had a question about shadow root
<joshtumath> ... they don't have boxes so they don't apply here?
<joshtumath> ... depending on how we do this, scroll parent API has to retarget stuff from inside to outside shadow trees
<joshtumath> ... the scroll only on the first ancestor would get you ???
<joshtumath> ... so i wonder if we should do more research for that situation
<joshtumath> ... the closest scroller, if it's in the shadow tree, do you want the container inside your tree
<joshtumath> ... I don't think you want to get it without conversions, to get an element from a shadow tree
<joshtumath> flackr: I guess in a way, you can scroll up to the ????
<argyle> q+
<joshtumath> emilio: you need a way of avoiding scroll parent in the shadow root
<joshtumath> ... that may be an issue with shadow parent rather than this issue
<joshtumath> flackr: we should have an API for that that expands to specifying a container in the future
<bramus> q+
<Rossen5> ack argyle
<joshtumath> argyle: 100+ on this. it makes scrollIntoView unusable because of the side effects
<emilio> maybe we have an enum Container { "all", "first" }, then optionally make it `(Container or Element) container;` in the future
<joshtumath> ... maybe tag the containers with CSS. we indicate a scroll into view mechanism
<emilio> q+
<flackr> I would say nearest instead of first
<joshtumath> ... then you declare on any container that you know of.
<joshtumath> flackr: I disagree because that would change things like scrolling to an anchor link
<emilio> q-
<joshtumath> ... could spec it not to but maybe it would be weird, affect some APIs not others
<joshtumath> ... agree with emilio's suggestion changing first to nearest
<joshtumath> emilio: sounds good
<joshtumath> bramus: could have default value being document root element
<joshtumath> ... as smfr said in IRC, could continue this at F2F
<joshtumath> flackr: that could be breaking. we could change what all does
<joshtumath> Rossen: I want to see if we can resolve
<joshtumath> ... instead of deferring to F2F
<flackr> Proposed resolution: Add container: 'all' | 'nearest' with default value 'all'
<joshtumath> ... we can agree to adopt it as currently specified
<emilio> PROPOSED: enum ScrollIntoViewContainer { "all", "nearest" }, defaulting to `all`, expanding to element in the future if needed
<joshtumath> flackr: scroll parent ancestry
<joshtumath> emilio: not if the block is not scrollable
<joshtumath> smfr: does it work in the popover case?
<joshtumath> emilio: it would scroll the viewport, but that would be outside of the scroller
<joshtumath> ... we should define this so that it works properly in that case.
<joshtumath> smfr: OK
<joshtumath> Rossen: sounds like it's resolved
<joshtumath> RESOLVED: enum ScrollIntoViewContainer { "all", "nearest" }, defaulting to `all`, expanding to element in the future if needed
<emilio> We should probably follow-up with issues for iframe and overflow-hidden tweaks

flackr added a commit to flackr/csswg-drafts that referenced this issue Feb 7, 2025
Adds the container option to ScrollIntoViewOptions
allowing scrolling only the nearest containing scrolling box
as resolved in w3c#9452.
tabatkins added a commit that referenced this issue Feb 7, 2025
* [cssom-view-1] Add container option to scrollIntoView options.

Adds the container option to ScrollIntoViewOptions
allowing scrolling only the nearest containing scrolling box
as resolved in #9452.

* Update cssom-view-1/Overview.bs

---------

Co-authored-by: Tab Atkins Jr. <[email protected]>
@flackr flackr removed the Needs Edits label Feb 7, 2025
@flackr
Copy link
Contributor Author

flackr commented Feb 7, 2025

Spec edits landed in #11673

@flackr flackr closed this as completed Feb 7, 2025
Logyle12 added a commit to Logyle12/odin-calculator that referenced this issue May 14, 2025
- The default scrollIntoView caused the document to shift upward.
- It scrolls ancestor elements like <body> if target element isn't
  visible in its container.
- When saving, We now use scrollTop to jump to the latest entry.
- When loading, we center the entry to avoid layout shifts.
- See for reference: w3c/csswg-drafts#9452.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants