Bug 237696 – [iOS] Fix overscroll-behavior for main document

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Nikos Mouchtaris]

Make overscroll-behavior on html element work for iOS.

[Nikos Mouchtaris]

rdar://89501965

[Nikos Mouchtaris]

Created attachment 454309 [details]
Patch

[Nikos Mouchtaris]

Created attachment 454432 [details]
Patch

[Nikos Mouchtaris]

Created attachment 454437 [details]
Patch

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 454442 [details]
Patch

[Simon Fraser (smfr)]

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.

[Nikos Mouchtaris]

Created attachment 454501 [details]
Patch

[Simon Fraser (smfr)]

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"

[Nikos Mouchtaris]

Created attachment 454504 [details]
Patch

[Nikos Mouchtaris]

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

[EWS]

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