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

Add support for scroll-margin and scroll-padding #386

Open
stipsan opened this issue Aug 5, 2018 · 12 comments
Open

Add support for scroll-margin and scroll-padding #386

stipsan opened this issue Aug 5, 2018 · 12 comments

Comments

@stipsan
Copy link
Member

stipsan commented Aug 5, 2018

Documented in this spec: https://drafts.csswg.org/css-scroll-snap-1/#propdef-scroll-margin

This is how it'll be possible to set custom offsets without reimplementing the default scrolling behavior or changing the HTML structure to add wrapping elements with padding to accomplish the same thing.

@fpapado
Copy link

fpapado commented Oct 22, 2018

Hi! I've been wondering about how to implement this. Trying to get things to scroll correctly with a sticky header has been uneven, and I like the idea of supporting these properties to achieve consistency.

Here's a rundown of my thoughts so far; I'd love your input if you have time!

Things affected

Scroll padding and margin would affect two things:

  • The amount to scroll, depending on scroll-margin on the element, or scroll-padding on an ancestor;
  • Whether the element should be considered "in view", for the purpose of that scrolling behaviour.

Communicating those properties

Reading the spec, it seems that eventually we'd be able to read the computed style, same as currently done in compute-scroll-into-view. Until browsers implement it, that seems unlikely. Similarly, as a polyfill, I think this means we have to pass those properties as function arguments?

Code bits

If the above is true, it seems that we'd have to modify both scroll- and compute-into-view. I see perhaps how scroll-margin would work. Since the polyfill works only on the element, is it even possible to implement scroll-padding?

@stipsan
Copy link
Member Author

stipsan commented Nov 30, 2018

Hey @fpapado! I appreciate your thoughtful feedback! I'll do my best to respond here.

When I set out to refactor this package into what became v2 more complicated things like sticky + fixed positioning were one of the things I had in mind. It's difficult to get things like that right, as it depends on more information than the computed style of the element you are wanting to scroll, or the offset parent. You have to walk up the full tree to properly scroll the element into view, on possibly multiple scroll boxes.

That's why the compute-scroll-into-view package is actually calculating possible scroll coordinates for anything that isn't within the boundary.
It runs this check today to decide wether something can overflow = scroll boundary: https://github.com/stipsan/compute-scroll-into-view/blob/b850561acfb87b7b67a52f863f4c029bcad6f894/src/index.ts#L56-L77

I haven't dived into the spec on scroll-padding yet so I can only assume that scroll padding only makes sense on elements that are scrollable. If that's the case then it's possible to implement scroll-padding using the work that's already done to implement border-width correctly. In fact, border widths appear to affect scrolling on children elements much the same way scroll padding does.

scroll-margin would also need to be implemented similar to how border widths are taken into account with the scroll target.

You're also right that reading these new CSS property can prove to be a challenge. Unlike border widths it's unlikely they'll show up in getComputedStyle in browsers that don't implement them.

I really hope we can avoid needing to pass these values as properties in function call itself. Especially since today's API don't let you specify other elements than the target element and the boundary. Specifying scroll margins and paddings on elements between the boundary and scroll target would require the API to be much more complex. I hope it's possible to somehow have it stay in CSS so that the migration path away from this polyfill stays is as simple as: scrollIntoView(target) => target.scrollIntoView().

Another question is wether compute-scroll-into-view should try to implement scroll-margin and scroll-padding directly, or if it should expose the hooks necessary for scroll-into-view-if-needed to do the bulk of the work to keep the compute-scroll-into-view package as small as possible.

Since the bundlesize of downshift is directly affected by this I'd like to hear what @kentcdodds thinks, and perhaps ask for feedback from the downshift community as well 🙂

@kentcdodds
Copy link

How much will this impact the bundle size?

@stipsan
Copy link
Member Author

stipsan commented Dec 3, 2018

I don't know yet what the impact will be. I'll run a POC and ping you when I have some numbers to look at 🙂

@fpapado
Copy link

fpapado commented Dec 7, 2018

Hey, @stipsan! Loved that response, it matches much of my own dilemmas when considering where and how this could be supported. I think exposing hooks in compute-scroll-into-view might be nice, in case some other consumer of the library wants to add more custom calculations/offsets.

A random thought that came to mind: could we use a custom property as a halfway point between the "real" CSS properties and the ponyfill? Those would show up in getComputedStyle iirc. Something like --scroll-into-view-padding, --scroll-into-view-margin. It wouldn't "work" on IE11, but it acts as a progressive enhancement, and would not regress wrt the current behaviour anyway. That might allow us to keep things declarative and forward-looking, without extra parameters on the function.

I need to think a bit more about the other constraints, but I'm hopeful!

@thedamon
Copy link

A scroll-padding css property would be a pretty beautiful to achieve a lot of this. That way it could be reflective of specific elements, and also react to a css-custom property that might be updated as fixed things sit on top of other things...
It might make the Standards cry, though.

@stipsan
Copy link
Member Author

stipsan commented May 14, 2019

The best way to support IE11 is to use a markup structure that can accomplish the same things without using scroll-padding or scroll-margin.

With that in mind I think it's fine to rely on custom properties to "bridge the gap". As in we can use getComputedStyle to first check for scroll-margin-* and scroll-padding-* properties and if there isn't any it can fall back to --scroll-margin-* and --scroll-padding-*.
You could even write a post-css plugin that emits the custom properties for you based on the native properties so you don't need to manually keep duplicates in sync.

Again, this would mean you can't use this feature if you need to support IE11. But I don't believe we can make it work in IE11 without ending up with an unacceptable bundle size or impractical complexity.

With regards to making standards cry... I'd rather use custom properties than introduce even more properties in the JS API that isn't in the spec if you know what I mean 😅

@Svish
Copy link

Svish commented Sep 28, 2020

I need either this, or an offset I can supply. 😕

@stipsan
Copy link
Member Author

stipsan commented Nov 28, 2020

The draft spec has signaled this is coming in the if-needed spec, I'll work on this soon 😄

@bakura10
Copy link

bakura10 commented Mar 7, 2022

Hi @stipsan :).

Is there any progress on supporting scroll-padding/margin to this library? It seems to work pretty well, but we have a use case where we need to actually "flush" the container to fill the whole width, and adding the scroll margin/padding is currently necessary.

@lbdremy
Copy link

lbdremy commented Sep 8, 2022

Hi @stipsan ,
Thanks for the amazing work on this lib, any news on this topic ?

@graup
Copy link

graup commented Mar 12, 2024

I also needed this. (Chrome supports scroll-padding with scrollIntoView but has some bugs preventing me to use it.)

Here's my implementation (supporting top and bottom padding). For scrollPaddingTop, we can just subtract it from the computed top. For scrollPaddingBottom, we then need to make sure that the computed top doesn't put the target too low.

Would be happy to see support for this in scroll-into-view-if-needed.

import { compute } from "compute-scroll-into-view";

const getScrollPaddings = (target: Element) => {
  const computedStyle = window.getComputedStyle(target);
  return {
    top: parseFloat(computedStyle.scrollPaddingTop) || 0,
    right: parseFloat(computedStyle.scrollPaddingRight) || 0,
    bottom: parseFloat(computedStyle.scrollPaddingBottom) || 0,
    left: parseFloat(computedStyle.scrollPaddingLeft) || 0,
  };
};

export function scrollIntoView<T = unknown>(
  target: HTMLElement,
  options: Parameters<typeof compute>[1] & { behavior: ScrollBehavior },
): T | void {
  for (const { el, top, left } of compute(target, options)) {
    // Get scrolling element's scroll-padding
    const scrollPaddings = getScrollPaddings(el);

    // Subtract padding from computed top
    const topWithPadding = top - scrollPaddings.top;

    // Make sure to not scroll too far down
    const safeMin =
      target.offsetTop +
      target.getBoundingClientRect().height +
      scrollPaddings.bottom -
      el.clientHeight;

    const adjustedTop = Math.max(topWithPadding, safeMin);

    el.scroll({ top: adjustedTop, left, behavior: options?.behavior });
  }
}

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

8 participants