Bug 210469

Summary: Scroll snap specified on :root doesn't work
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: ScrollingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: cathiechen, changseok, cmarcelo, esprehn+autocc, ews-watchlist, fred.wang, glenn, jamesr, kondapallykalyan, luiz, mrobinson, pdr, simon.fraser, tonikitoo, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: BrowserCompat, InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200643
https://bugs.webkit.org/show_bug.cgi?id=210476
Bug Depends on:    
Bug Blocks: 218115    
Attachments:
Description Flags
test
none
Work in progress
none
Patch
none
Patch
none
Patch none

Simon Fraser (smfr)
Reported 2020-04-13 17:08:37 PDT
Created attachment 396361 [details] test This doesn't work: :root { scroll-snap-type: mandatory; margin: 20px; }
Attachments
test (592 bytes, text/html)
2020-04-13 17:08 PDT, Simon Fraser (smfr) 		no flags
Details
Work in progress (5.35 KB, patch)
2020-04-13 22:19 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (41.46 KB, patch)
2020-11-05 11:14 PST, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (41.53 KB, patch)
2020-11-05 11:16 PST, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Patch (41.51 KB, patch)
2020-11-06 01:01 PST, Martin Robinson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-04-13 17:09:10 PDT
<rdar://problem/61746676>
Simon Fraser (smfr)
Comment 2 2020-04-13 22:19:06 PDT
Created attachment 396380 [details] Work in progress
Simon Fraser (smfr)
Comment 3 2020-04-13 22:22:24 PDT
Cathie or Frederic, feel free to pick this up. I'm a bit concerned about ignoring scroll-snap style on the body for compat reasons (bug 200643), but I think it's important to make it work on :root { } and html { }. The element argument passed to updateSnapOffsetsForScrollableArea is pretty suspect, and we should certainly not be using the body's geometry. Tests should include an example where the body is independently scrollable from the document (overflow:scroll style on the body).
Martin Robinson
Comment 4 2020-11-05 11:14:33 PST
Created attachment 413329 [details] Patch
Martin Robinson
Comment 5 2020-11-05 11:16:10 PST
Created attachment 413330 [details] Patch
Simon Fraser (smfr)
Comment 6 2020-11-05 11:58:59 PST
Comment on attachment 413330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413330&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:166 > + const HashSet<const RenderBox*>& boxesWithScrollSnapPositions = scrollingElementBox.view().boxesWithScrollSnapPositions(); auto > Source/WebCore/rendering/RenderLayer.cpp:3595 > + updateSnapOffsetsForScrollableArea(*this, *box, box->style(), box->paddingBoxRect()); Does paddingBoxRect() do the right thing if scrollbars are visible? (e.g. bottom snap with vertical and horizontal scrollbar)?
Martin Robinson
Comment 7 2020-11-06 00:58:54 PST
(In reply to Simon Fraser (smfr) from comment #6) Thanks for the review. > > Source/WebCore/rendering/RenderLayer.cpp:3595 > > + updateSnapOffsetsForScrollableArea(*this, *box, box->style(), box->paddingBoxRect()); > > Does paddingBoxRect() do the right thing if scrollbars are visible? (e.g. > bottom snap with vertical and horizontal scrollbar)? It seems like this is working properly.
Martin Robinson
Comment 8 2020-11-06 01:01:00 PST
Created attachment 413411 [details] Patch
EWS
Comment 9 2020-11-06 01:45:00 PST
Committed r269506: <https://trac.webkit.org/changeset/269506> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413411 [details].
Note You need to log in before you can comment on or make changes to this bug.