Bug 233788

Summary: Implement CSS overscroll-behavior for iOS
Product: WebKit Reporter: Nikos Mouchtaris <nmouchtaris>
Component: New BugsAssignee: Nikos Mouchtaris <nmouchtaris>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin, j+webkit-bugzilla, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 176454    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
simon.fraser: review+
overscroll-behavior ios
none
Patch none

Description Nikos Mouchtaris 2021-12-02 16:22:08 PST
Implement CSS overscroll-behavior for iOS
Comment 1 Nikos Mouchtaris 2021-12-02 16:47:28 PST
Created attachment 445791 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-12-06 15:29:47 PST
<rdar://problem/86126776>
Comment 3 Nikos Mouchtaris 2021-12-07 14:58:10 PST
Created attachment 446239 [details]
Patch
Comment 4 Nikos Mouchtaris 2021-12-07 16:49:53 PST
Created attachment 446255 [details]
Patch
Comment 5 Simon Fraser (smfr) 2021-12-09 11:16:14 PST
Comment on attachment 446255 [details]
Patch

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

Why is there no layout test with this?

> Source/WebKit/ChangeLog:10
> +        to UIKit by Russell Ladd, to allow controlling of scroll chaining.

I don't think this level of detail needs to be in the webkit changelog.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:261
> +            scrollView.bouncesHorizontally = params.horizontalOverscrollBehavior == OverscrollBehavior::None ? NO : YES;
> +            scrollView.bouncesVertically = scrollingStateNode.scrollableAreaParameters().verticalOverscrollBehavior == OverscrollBehavior::None ? NO : YES;
> +            scrollView._allowsParentToBeginHorizontally = scrollingStateNode.scrollableAreaParameters().horizontalOverscrollBehavior != OverscrollBehavior::Auto ? NO : YES;
> +            scrollView._allowsParentToBeginVertically = scrollingStateNode.scrollableAreaParameters().verticalOverscrollBehavior != OverscrollBehavior::Auto ? NO : YES;

params vs scrollingStateNode.scrollableAreaParameters() ?
Comment 6 Nikos Mouchtaris 2021-12-09 12:37:46 PST
Created attachment 446588 [details]
Patch
Comment 7 Nikos Mouchtaris 2021-12-09 14:57:24 PST
Created attachment 446615 [details]
Patch
Comment 8 Darin Adler 2021-12-09 15:26:33 PST
Comment on attachment 446615 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:261
> +            scrollView.bouncesHorizontally = params.horizontalOverscrollBehavior == OverscrollBehavior::None ? NO : YES;
> +            scrollView.bouncesVertically = params.verticalOverscrollBehavior == OverscrollBehavior::None ? NO : YES;
> +            scrollView._allowsParentToBeginHorizontally = params.horizontalOverscrollBehavior != OverscrollBehavior::Auto ? NO : YES;
> +            scrollView._allowsParentToBeginVertically = params.verticalOverscrollBehavior != OverscrollBehavior::Auto ? NO : YES;

Since this is all Boolean logic, we could rewrite it to not use ? NO : YES so much, like this:

     scrollView.bouncesHorizontally = params.horizontalOverscrollBehavior != OverscrollBehavior::None;

Fine to land what was reviewed, but I would prefer the more direct boolean style rather than one that implements "!" using "? NO : YES" syntax.
Comment 9 Nikos Mouchtaris 2021-12-09 15:48:41 PST
Created attachment 446625 [details]
overscroll-behavior ios
Comment 10 Nikos Mouchtaris 2022-01-28 19:26:35 PST
Created attachment 450302 [details]
Patch
Comment 11 EWS 2022-01-28 20:23:50 PST
Committed r288782 (246559@main): <https://commits.webkit.org/246559@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450302 [details].
Comment 12 Nikos Mouchtaris 2022-02-17 17:32:10 PST
*** Bug 233787 has been marked as a duplicate of this bug. ***