Open stipsan opened 6 years ago
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!
Scroll padding and margin would affect two things:
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?
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
?
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 🙂
How much will this impact the bundle size?
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 🙂
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!
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.
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 😅
I need either this, or an offset
I can supply. 😕
The draft spec has signaled this is coming in the if-needed spec, I'll work on this soon 😄
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.
Hi @stipsan , Thanks for the amazing work on this lib, any news on this topic ?
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 });
}
}
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.