Bug 210469 - Scroll snap specified on :root doesn't work
Summary: Scroll snap specified on :root doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks: 218115
  Show dependency treegraph
 
Reported: 2020-04-13 17:08 PDT by Simon Fraser (smfr)
Modified: 2020-11-06 01:45 PST (History)
16 users (show)

See Also:


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 Details | Formatted Diff | Diff
Patch (41.46 KB, patch)
2020-11-05 11:14 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (41.53 KB, patch)
2020-11-05 11:16 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (41.51 KB, patch)
2020-11-06 01:01 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-04-13 17:08:37 PDT
Created attachment 396361 [details]
test

This doesn't work:

        :root {
            scroll-snap-type: mandatory;
            margin: 20px;
        }
Comment 1 Radar WebKit Bug Importer 2020-04-13 17:09:10 PDT
<rdar://problem/61746676>
Comment 2 Simon Fraser (smfr) 2020-04-13 22:19:06 PDT
Created attachment 396380 [details]
Work in progress
Comment 3 Simon Fraser (smfr) 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).
Comment 4 Martin Robinson 2020-11-05 11:14:33 PST
Created attachment 413329 [details]
Patch
Comment 5 Martin Robinson 2020-11-05 11:16:10 PST
Created attachment 413330 [details]
Patch
Comment 6 Simon Fraser (smfr) 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)?
Comment 7 Martin Robinson 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.
Comment 8 Martin Robinson 2020-11-06 01:01:00 PST
Created attachment 413411 [details]
Patch
Comment 9 EWS 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].