Bug 204936 - Add support for scroll behavior relies on native scroll interfaces for iOS platform
Summary: Add support for scroll behavior relies on native scroll interfaces for iOS pl...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: cathiechen
URL:
Keywords:
Depends on: 204882
Blocks: 188043
  Show dependency treegraph
 
Reported: 2019-12-05 23:00 PST by cathiechen
Modified: 2020-03-10 03:15 PDT (History)
28 users (show)

See Also:


Attachments
Patch (243.92 KB, patch)
2019-12-06 00:33 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (248.44 KB, patch)
2019-12-09 06:02 PST, cathiechen
no flags Details | Formatted Diff | Diff
UI-scroll-animation-for-review (189.47 KB, patch)
2019-12-09 08:51 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (297.32 KB, patch)
2019-12-11 00:45 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (149.71 KB, patch)
2019-12-11 00:46 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.72 KB, patch)
2019-12-11 03:35 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (148.84 KB, patch)
2019-12-11 03:35 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.66 KB, patch)
2019-12-11 07:24 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (148.78 KB, patch)
2019-12-11 07:25 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.32 KB, patch)
2019-12-11 09:06 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (148.45 KB, patch)
2019-12-11 09:06 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.22 KB, patch)
2019-12-11 09:25 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (148.34 KB, patch)
2019-12-11 09:25 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (294.92 KB, patch)
2019-12-13 04:32 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (148.04 KB, patch)
2019-12-13 04:33 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (294.92 KB, patch)
2019-12-13 05:53 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.57 KB, patch)
2019-12-13 06:18 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.76 KB, patch)
2019-12-13 07:07 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (295.74 KB, patch)
2019-12-13 07:53 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (294.95 KB, patch)
2019-12-15 07:08 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (148.03 KB, patch)
2019-12-15 07:08 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-ews(including parsing + non_native_animation) (294.88 KB, patch)
2019-12-15 09:13 PST, cathiechen
no flags Details | Formatted Diff | Diff
native-animation-for-review (147.97 KB, patch)
2019-12-15 09:14 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (152.05 KB, patch)
2020-01-19 22:22 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (152.51 KB, patch)
2020-02-12 01:43 PST, cathiechen
no flags Details | Formatted Diff | Diff
Patch (152.17 KB, patch)
2020-02-17 21:40 PST, cathiechen
cathiechen: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description cathiechen 2019-12-05 23:00:23 PST
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.
Comment 1 cathiechen 2019-12-06 00:33:58 PST
Created attachment 384994 [details]
Patch
Comment 2 cathiechen 2019-12-09 06:02:46 PST
Created attachment 385146 [details]
Patch
Comment 3 cathiechen 2019-12-09 08:51:55 PST
Created attachment 385155 [details]
UI-scroll-animation-for-review

This patch only contains change for UI scroll.
Comment 4 cathiechen 2019-12-11 00:45:55 PST
Created attachment 385361 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 5 cathiechen 2019-12-11 00:46:38 PST
Created attachment 385362 [details]
native-animation-for-review
Comment 6 Frédéric Wang (:fredw) 2019-12-11 01:45:33 PST
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 7 cathiechen 2019-12-11 03:33:37 PST
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
Comment 8 cathiechen 2019-12-11 03:35:07 PST
Created attachment 385374 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 9 cathiechen 2019-12-11 03:35:25 PST
Created attachment 385375 [details]
native-animation-for-review
Comment 10 cathiechen 2019-12-11 07:24:45 PST
Created attachment 385388 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 11 cathiechen 2019-12-11 07:25:06 PST
Created attachment 385389 [details]
native-animation-for-review
Comment 12 cathiechen 2019-12-11 09:06:23 PST
Created attachment 385399 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 13 cathiechen 2019-12-11 09:06:42 PST
Created attachment 385400 [details]
native-animation-for-review
Comment 14 cathiechen 2019-12-11 09:25:36 PST
Created attachment 385402 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 15 cathiechen 2019-12-11 09:25:57 PST
Created attachment 385403 [details]
native-animation-for-review
Comment 16 Frédéric Wang (:fredw) 2019-12-13 01:23:59 PST
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 17 cathiechen 2019-12-13 02:15:31 PST
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
Comment 18 cathiechen 2019-12-13 04:32:57 PST
Created attachment 385591 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 19 cathiechen 2019-12-13 04:33:18 PST
Created attachment 385592 [details]
native-animation-for-review
Comment 20 cathiechen 2019-12-13 05:53:30 PST
Created attachment 385596 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 21 cathiechen 2019-12-13 06:18:59 PST
Created attachment 385597 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 22 cathiechen 2019-12-13 07:07:10 PST
Created attachment 385600 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 23 cathiechen 2019-12-13 07:53:20 PST
Created attachment 385602 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 24 cathiechen 2019-12-15 07:08:04 PST
Created attachment 385716 [details]
native-animation-for-ews(including parsing + non_native_animation)

Rebase
Comment 25 cathiechen 2019-12-15 07:08:31 PST
Created attachment 385717 [details]
native-animation-for-review
Comment 26 cathiechen 2019-12-15 09:13:55 PST
Created attachment 385722 [details]
native-animation-for-ews(including parsing + non_native_animation)
Comment 27 cathiechen 2019-12-15 09:14:16 PST
Created attachment 385723 [details]
native-animation-for-review
Comment 28 cathiechen 2020-01-19 22:22:57 PST
Created attachment 388201 [details]
Patch
Comment 29 cathiechen 2020-02-12 01:43:46 PST
Created attachment 390497 [details]
Patch
Comment 30 Frédéric Wang (:fredw) 2020-02-13 05:17:17 PST
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 31 cathiechen 2020-02-17 21:37:44 PST
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?
Comment 32 cathiechen 2020-02-17 21:40:34 PST
Created attachment 391029 [details]
Patch