Summary: | Make scroll-margin independent of scroll snapping and have it apply when scrolling to anchors | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||||||
Component: | CSS | Assignee: | Martin Robinson <mrobinson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, changseok, cmarcelo, dev, esprehn+autocc, ews-watchlist, fred.wang, glenn, kangil.han, kondapallykalyan, pdr, rego, simon.fraser, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=218358 | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 189265 | ||||||||||||||
Attachments: |
|
Description
Martin Robinson
2020-10-22 03:45:35 PDT
Created attachment 412517 [details]
Patch
Created attachment 412519 [details]
Patch
Created attachment 412524 [details]
Patch
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? (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 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. Created attachment 412625 [details]
Patch
Created attachment 412631 [details]
Patch
mrobinson@webkit.org 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. Committed r269144: <https://trac.webkit.org/changeset/269144> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412631 [details]. Thank you guys very much for finally quick fix! Any ideas when it may land in user lands? |