[webkit-reviews] review granted: [Bug 218076] Make scroll-margin independent of scroll snapping and have it apply when scrolling to anchors : [Attachment 412524] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 218076: Make scroll-margin independent of scroll snapping and have it apply
when scrolling to anchors
https://bugs.webkit.org/show_bug.cgi?id=218076

Attachment 412524: Patch

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




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 412524
  --> https://bugs.webkit.org/attachment.cgi?id=412524
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.