Bug 218076 – Make scroll-margin independent of scroll snapping and have it apply when scrolling to anchors

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Martin Robinson]

The latest version of the CSS Scroll Snap specification says that scroll-margin should be available independently of scroll snap and it should apply when scrolling to anchors and targets of scrollIntoView(). See https://drafts.csswg.org/css-scroll-snap-1/#scroll-margin.

[Martin Robinson]

Created attachment 412517 [details]
Patch

[Martin Robinson]

Created attachment 412519 [details]
Patch

[Martin Robinson]

Created attachment 412524 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 412524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412524&action=review

> Source/WebCore/rendering/RenderBox.cpp:5070
> +    const LayoutSize boxSize = size();
> +    const LayoutBoxExtent margin(
> +        valueForLength(scrollMargin.top(), boxSize.height()),
> +        valueForLength(scrollMargin.right(), boxSize.width()),
> +        valueForLength(scrollMargin.bottom(), boxSize.height()),
> +        valueForLength(scrollMargin.left(), boxSize.width()));
> +    anchorRect.expand(margin);

This doesn't take into account transforms in the ancestor chain. Seems like you need to take margins into account before the localToAbsolute computation, I think? What's the expected behavior with transforms?

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 412524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412524&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:5070
> > +    const LayoutSize boxSize = size();
> > +    const LayoutBoxExtent margin(
> > +        valueForLength(scrollMargin.top(), boxSize.height()),
> > +        valueForLength(scrollMargin.right(), boxSize.width()),
> > +        valueForLength(scrollMargin.bottom(), boxSize.height()),
> > +        valueForLength(scrollMargin.left(), boxSize.width()));
> > +    anchorRect.expand(margin);
> 
> This doesn't take into account transforms in the ancestor chain. Seems like
> you need to take margins into account before the localToAbsolute
> computation, I think? What's the expected behavior with transforms?

I was a bit surprised by this at first, but, unless I am misunderstanding, it seems like this margin should be added in the coordinate system of the scroll container. The scroll-margin spec [1] says:

"Values represent outsets defining the scroll snap area that is used for snapping this box to the snapport. The scroll snap area is determined by taking the transformed border box, finding its rectangular bounding box (axis-aligned in the scroll container’s coordinate space), then adding the specified outsets."

1. https://drafts.csswg.org/css-scroll-snap-1/#scroll-margin

[Simon Fraser (smfr)]

Comment on attachment 412524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412524&action=review

>>> Source/WebCore/rendering/RenderBox.cpp:5070
>>> +    anchorRect.expand(margin);
>> 
>> This doesn't take into account transforms in the ancestor chain. Seems like you need to take margins into account before the localToAbsolute computation, I think? What's the expected behavior with transforms?
> 
> I was a bit surprised by this at first, but, unless I am misunderstanding, it seems like this margin should be added in the coordinate system of the scroll container. The scroll-margin spec [1] says:
> 
> "Values represent outsets defining the scroll snap area that is used for snapping this box to the snapport. The scroll snap area is determined by taking the transformed border box, finding its rectangular bounding box (axis-aligned in the scroll container’s coordinate space), then adding the specified outsets."
> 
> 1. https://drafts.csswg.org/css-scroll-snap-1/#scroll-margin

Huh, I guess that's clear. Probably add a comment linking to that then.

[Martin Robinson]

Created attachment 412625 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70803885>

[Martin Robinson]

Created attachment 412631 [details]
Patch

[EWS]

[e-mail] does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 412631 [details] from commit queue.

[EWS]

Committed r269144: <https://trac.webkit.org/changeset/269144>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412631 [details].

[dakur]

Thank you guys very much for finally quick fix! Any ideas when it may land in user lands?