Bug 237696

rdar://89501965

Created attachment 454309 [details]
Patch

Created attachment 454432 [details]
Patch

Created attachment 454437 [details]
Patch

Comment on attachment 454437 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        UIScrollView should rubber band or not. Need to investigate adding a test for this,
> +        as this doesn't require the new UIKit SPI.

The comment about testing is no longer relevant.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:232
> +#if HAVE(UIKIT_OVERSCROLL_BEHAVIOR_SUPPORT)
> +    scrollView._allowsParentToBeginHorizontally = horizontalOverscrollBehavior == OverscrollBehavior::Auto;
> +    scrollView._allowsParentToBeginVertically = verticalOverscrollBehavior == OverscrollBehavior::Auto;
> +#endif

I don't think we want to set this on the root scroll view. That would allow web content to prevent scroll gestures in the web view from propagating to some ancestor scroller.

Created attachment 454442 [details]
Patch

Comment on attachment 454442 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.h:86
> +    static void updateScrollViewForOverscrollBehavior(UIScrollView *, const WebCore::OverscrollBehavior, const WebCore::OverscrollBehavior, bool);

We try to avoid bool function parameters where it's hard to know at the call site what it means (this is called "the boolean trap"). You make it harder here by not naming the parameters in the header.

Possible solution are to use an enum, like 
enum class AllowScrollPropagation : bool { No, Yes };

or two functions.

Created attachment 454501 [details]
Patch

Comment on attachment 454501 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:229
> +    if (allowPropogation == AllowScrollPropagation::No) {

Isn't this backwards?

Or maybe it's confusing that the enum is called "AllowScrollPropagation" (bad suggestion on my part). I was thinking more like "AllowOverscrollToPreventScrollPropagation"

Created attachment 454504 [details]
Patch

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 454501 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454501&action=review
> 
> > Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:229
> > +    if (allowPropogation == AllowScrollPropagation::No) {
> 
> Isn't this backwards?
> 
> Or maybe it's confusing that the enum is called "AllowScrollPropagation"
> (bad suggestion on my part). I was thinking more like
> "AllowOverscrollToPreventScrollPropagation"

Ah I had interpreted AllowScrollPropagation::No to mean we were disabling scroll propagation. I updated to AllowOverscrollToPreventScrollPropagation as its a bit more clear.

Committed r291198 (248353@main): <https://commits.webkit.org/248353@main>

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