This is the second implement part of scroll behavior in bug 188043. This patch contains the implement of scroll behavior that relies on native scroll interfaces on iOS platform. On other platforms, the behavior isn't changed. I.g. The smooth scroll will be performed by an instant scroll instead.
Created attachment 384994 [details] Patch
Created attachment 385146 [details] Patch
Created attachment 385155 [details] UI-scroll-animation-for-review This patch only contains change for UI scroll.
Created attachment 385361 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385362 [details] native-animation-for-review
Comment on attachment 385362 [details] native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385362&action=review > LayoutTests/ChangeLog:12 > + copy scroll-behavior test to fast/scrolling/ios/ why not move them into fast/scrolling/ios/cssom-view? > LayoutTests/ChangeLog:14 > + * fast/scrolling/ios/resources/scroll-behavior.js: Added. I think this is copied from LayoutTests/imported/w3c/web-platform-tests/css/cssom-view/support/scroll-behavior.js ; any reason why you decided to move it to the shared ios/resources/ ? > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:255 > + // The scroll position of animated scrolling will be set from the the UI process. So it shouldn't be set now. the the => by the? > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:-266 > - unneeded whitespace change > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:226 > + } This is a bit weird, why not #if PLATFORM(IOS_FAMILY) if (requestScrollWithAnimation()) return; #endif ? > Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:215 > +#endif Do we really need to add IOS conditionals for this and other APIs? I think we can add the instantScrollDidStopAnimation parameter on all platforms with a default value and ignore it on non-iOS ones.
Comment on attachment 385362 [details] native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385362&action=review Hi Fred, Thanks for the review:) >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:255 >> + // The scroll position of animated scrolling will be set from the the UI process. So it shouldn't be set now. > > the the => by the? Done >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:-266 >> - > > unneeded whitespace change done >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:226 >> + } > > This is a bit weird, why not > > #if PLATFORM(IOS_FAMILY) > if (requestScrollWithAnimation()) > return; > #endif > > ? Done >> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:215 >> +#endif > > Do we really need to add IOS conditionals for this and other APIs? > > I think we can add the instantScrollDidStopAnimation parameter on all platforms with a default value and ignore it on non-iOS ones. done
Created attachment 385374 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385375 [details] native-animation-for-review
Created attachment 385388 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385389 [details] native-animation-for-review
Created attachment 385399 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385400 [details] native-animation-for-review
Created attachment 385402 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385403 [details] native-animation-for-review
Comment on attachment 385403 [details] native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385403&action=review > LayoutTests/ChangeLog:12 > + copy scroll-behavior test to fast/scrolling/ios/ I guess this is only needed because AsyncOverflowScrollingEnabled is not fully supported on non-iOS platforms? Maybe explain this? > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:277 > +#endif Maybe you can do only one call stateNode->setRequestedScrollPosition (see below). bool animated = false #if PLATFORM(IOS_FAMILY) ... #endif stateNode->setRequestedScrollPosition(scrollPosition, inProgrammaticScroll, animated); > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:240 > +#endif I think this #if #else can be merged in one function definition using the UNUSED_PARAM(requestedWithAnimation) on non-iOS platform? > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:119 > +#endif setRequestedScrollPosition can be have a single declaration outside the #if ; maybe you can remove the default value for requestedWithAnimation you end up always being explicit. > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:168 > +#endif You don't need PLATFORM(IOS_FAMILY) here, just use UNUSED_PARAM(needSyncScrollPosition) on non-iOS platforms. > Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:273 > +#endif Again, you can probably do only one node.setRequestedScrollPosition call ouside the #if > Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:231 > +#endif Ditto? > Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp:111 > +#endif Can probably merge this in one m_scrollingCoordinatorProxy.scrollingTreeNodeDidScroll call and do the #if to calculate the last parameter.
Comment on attachment 385403 [details] native-animation-for-review View in context: https://bugs.webkit.org/attachment.cgi?id=385403&action=review Hi Fred, Thanks for the review:) >> LayoutTests/ChangeLog:12 >> + copy scroll-behavior test to fast/scrolling/ios/ > > I guess this is only needed because AsyncOverflowScrollingEnabled is not fully supported on non-iOS platforms? Maybe explain this? done >> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.cpp:240 >> +#endif > > I think this #if #else can be merged in one function definition using the UNUSED_PARAM(requestedWithAnimation) on non-iOS platform? Done, thanks:) >> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:119 >> +#endif > > setRequestedScrollPosition can be have a single declaration outside the #if ; maybe you can remove the default value for requestedWithAnimation you end up always being explicit. Done >> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:168 >> +#endif > > You don't need PLATFORM(IOS_FAMILY) here, just use UNUSED_PARAM(needSyncScrollPosition) on non-iOS platforms. Done >> Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:273 >> +#endif > > Again, you can probably do only one node.setRequestedScrollPosition call ouside the #if Done >> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:231 >> +#endif > > Ditto? done >> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp:111 >> +#endif > > Can probably merge this in one m_scrollingCoordinatorProxy.scrollingTreeNodeDidScroll call and do the #if to calculate the last parameter. done
Created attachment 385591 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385592 [details] native-animation-for-review
Created attachment 385596 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385597 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385600 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385602 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385716 [details] native-animation-for-ews(including parsing + non_native_animation) Rebase
Created attachment 385717 [details] native-animation-for-review
Created attachment 385722 [details] native-animation-for-ews(including parsing + non_native_animation)
Created attachment 385723 [details] native-animation-for-review
Created attachment 388201 [details] Patch
Created attachment 390497 [details] Patch
Comment on attachment 390497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390497&action=review > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:307 > +void AsyncScrollingCoordinator::setScrollAnimationInProgress(ScrollingNodeID nodeID, bool isScrollAnimationInProgress) This function does nothing if isScrollAnimationInProgress is true. Should it be called interruptScrollAnimation()? > Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:110 > + setScrollAnimationInProgress(nodeID, isScrollAnimationInProgress); This could be if (!isScrollAnimationInProgress) interruptScrollAnimation(nodeID);
Comment on attachment 390497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390497&action=review >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:307 >> +void AsyncScrollingCoordinator::setScrollAnimationInProgress(ScrollingNodeID nodeID, bool isScrollAnimationInProgress) > > This function does nothing if isScrollAnimationInProgress is true. Should it be called interruptScrollAnimation()? isScrollAnimationInProgress is always false currently. This function is used to indicate that native animated scroll is end. I changed the name to nativeScrollAnimationStopped, WDYT?
Created attachment 391029 [details] Patch
<rdar://problem/71331742>
I'll take this over, if that's OK.
(In reply to Simon Fraser (smfr) from comment #34) > I'll take this over, if that's OK. Sure, no problem, thanks!
Created attachment 440729 [details] Patch
Created attachment 440730 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 440731 [details] Patch
Committed r283911 (242782@main): <https://commits.webkit.org/242782@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440731 [details].