Bug 142503 – [Win] Implement scroll-snap-points on Windows

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

We should support scroll-snap-points on Windows.

[Radar WebKit Bug Importer]

<rdar://problem/20093603>

[Martin Robinson]

Created attachment 424183 [details]
Patch

[Martin Robinson]

Comment on attachment 424183 [details]
Patch

Removing review flag. This needs a bit of work.

[Don Olmstead]

Moving some comments on Slack to here since comments go away eventually.

Anything setting ENABLE_CSS_SCROLL_SNAP in Source/cmake/Options*.cmake should be deleted.

https://github.com/WebKit/WebKit/search?q=ENABLE_CSS_SCROLL_SNAP shows a few places where it is manually set.

Along with that there's a convention to set ENABLE values to their defaults in PlatformEnable.h and PlatformEnable<Port>.h. If you look currently ENABLE_CSS_SCROLL_SNAP is set to ON by default for Mac.

https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/PlatformEnableCocoa.h#L143-L145

Since you're turning it on for all ports then you want to move that block of code over to PlatformEnable.h. You'll see a section for this around https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/PlatformEnable.h#L103 just cut the code from PlatformEnableMac.h and paste it in there alphabetically.

The AppleWin failures are around tests for this feature not passing. The patch builds successfully. You'll want to talk with someone who works directly on AppleWin for that. These also might fail on WinCairo too but there's no bot for that.

Thanks for getting this going!

[Martin Robinson]

Created attachment 424264 [details]
Patch

[Don Olmstead]

Comment on attachment 424264 [details]
Patch

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

r=me

If the Windows tests are causing too much of a problem maybe just bust it out into a separate bug where you enable the tests and do any required gardening.

> Source/WTF/ChangeLog:3
> +        [Win] Implement scroll-snap-points on Windows

Better bug title might be "Enable CSS_SCROLL_SNAP by default" since that's what this patch is really doing.

[Martin Robinson]

Created attachment 424323 [details]
Patch

[Don Olmstead]

Comment on attachment 424323 [details]
Patch

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

Martin to get this unblocked I think you should rename this bug to `ENABLE_SCROLL_SNAP by default` since its not really a Windows bug and split the test changes into another bug.

> Source/WebCore/testing/Internals.cpp:4659
> +

Not sure why this whitespace change snuck in.

> LayoutTests/platform/win/TestExpectations:-532
> -# TODO ENABLE(CSS_SCROLL_SNAP) is disabled.
> -css3/scroll-snap/ [ Skip ]
> -imported/w3c/web-platform-tests/css/css-scroll-snap [ Skip ]
> -

I think you should just split this into a separate patch considering the AppleWin build is red. You might have to work with someone on the Apple side to turn on the tests.

[Martin Robinson]

Created attachment 426554 [details]
Patch

[Don Olmstead]

Created attachment 426565 [details]
Proposal

[Don Olmstead]

Created attachment 426566 [details]
Proposal

[Don Olmstead]

Comment on attachment 426566 [details]
Proposal

Hey Martin maybe I wasn't communicating things right. Here's what I was thinking you do to get this landed.

Rename the bug so it matches what you're doing. In this case I changed the title to "Enable CSS Scroll Snap by Default".

Remove the part around LayoutTests and land that in a separate bug. You'll either need to get someones attention on the AppleWin side or just land it for WinCairo.

[Martin Robinson]

Don, sorry for the confusion here. I totally agree about splitting the parts of this patch. I think you are correct that the first version did too much.  My take on your plan is to:

1. Turn on scroll snap for Windows in this bug (as long as I can get around the testing issue). This would avoid having to rename the bug.
2. Open a new bug ("Enable CSS Scroll Snap by Default") that would be the "Proposal" patch that you have uploaded.

[Don Olmstead]

(In reply to Martin Robinson from comment #13)
> Don, sorry for the confusion here. I totally agree about splitting the parts
> of this patch. I think you are correct that the first version did too much. 
> My take on your plan is to:
> 
> 1. Turn on scroll snap for Windows in this bug (as long as I can get around
> the testing issue). This would avoid having to rename the bug.
> 2. Open a new bug ("Enable CSS Scroll Snap by Default") that would be the
> "Proposal" patch that you have uploaded.

Sounds great! I would probably do the "Enable CSS Scroll Snap by Default" first. You can explicitly turn it off for AppleWin by adding to this area https://github.com/WebKit/WebKit/blob/main/Source/cmake/OptionsWin.cmake#L93-L102 and then do the enabling of tests for AppleWin.

[Martin Robinson]

(In reply to Don Olmstead from comment #14)
> (In reply to Martin Robinson from comment #13)
> > Don, sorry for the confusion here. I totally agree about splitting the parts
> > of this patch. I think you are correct that the first version did too much. 
> > My take on your plan is to:
> > 
> > 1. Turn on scroll snap for Windows in this bug (as long as I can get around
> > the testing issue). This would avoid having to rename the bug.
> > 2. Open a new bug ("Enable CSS Scroll Snap by Default") that would be the
> > "Proposal" patch that you have uploaded.
> 
> Sounds great! I would probably do the "Enable CSS Scroll Snap by Default"
> first. You can explicitly turn it off for AppleWin by adding to this area
> https://github.com/WebKit/WebKit/blob/main/Source/cmake/OptionsWin.cmake#L93-
> L102 and then do the enabling of tests for AppleWin.

Oh, that's a good point! I'll get that uploaded tomorrow.

[Martin Robinson]

Created attachment 430339 [details]
Patch

[Martin Robinson]

Created attachment 430554 [details]
Patch

[EWS]

Committed r278452 (238472@main): <https://commits.webkit.org/238472@main>

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