Bug 142503

<rdar://problem/20093603>

Created attachment 424183 [details] Patch

Comment on attachment 424183 [details] Patch Removing review flag. This needs a bit of work.

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!

Created attachment 424264 [details] Patch

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.

Created attachment 424323 [details] Patch

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.

Created attachment 426554 [details] Patch

Created attachment 426566 [details] Proposal

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.

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.

(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.

(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.

Created attachment 430339 [details] Patch

Created attachment 430554 [details] Patch

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].