Bug 176454

Summary: Implement CSS overscroll-behavior
Product: WebKit Reporter: Majid Valipour <majidvp>
Component: CSSAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: NEW ---    
Severity: Normal CC: 709922234, andrea, b56girard, bdakin, bugs.webkit.org, cathiechen, changseok, clopez, cmarcelo, contact, diiiimaaaa, doochik, dvoytenko, dvpdiner2, esprehn+autocc, ews-watchlist, fred.wang, glenn, gyuyoung.kim, henrik, hi, jamesr, jdopazo, joepeck, jonlee, kai.hollberg, kngan, koivisto, kondapallykalyan, kris, kyle.bavender, luiz, macpherson, mail, me, menard, michaelcpuckett, mihai, paulo.sousa, pdr, richardyrh928, rwlbuis, sean, simon.fraser, tannerhodges, the.bull, tonikitoo, vepomoc, vincentriemer, webkit-bug-importer, wenson_hsieh, yonseca, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.csswg.org/css-overscroll-behavior/
Bug Depends on: 192358, 192397    
Bug Blocks:    
Attachments:
Description Flags
Testcase
none
WIP Patch (CSS parsing only)
none
WIP Patch (CSS parsing only)
none
Testcase of annoying scroll chaining
none
WIP Patch (CSS parsing and preference option)
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Add overscroll behavior property to scrolling tree
none
WIP Patch
none
Patch
none
Patch (applies on top of bug 192358)
none
tentative rebase (not tested)
none
tentative rebase (not tested)
none
tentative rebase (not tested)
none
Patch
none
Patch
none
Patch
none
rebase
none
rebase
none
rebase
simon.fraser: review-
WIP-patch-based-on-Fred's-patch
none
WIP-patch-based-on-Fred's-patch
none
WIP-patch-based-on-Fred's-patch
none
WIP-patch-based-on-Fred's-patch none

Description Majid Valipour 2017-09-06 07:13:08 PDT
Consider implementing the new CSS scroll-boundary-behavior property.

## Specification
This feature specification is being incubated in WICG: 
https://wicg.github.io/scroll-boundary-behavior/


Note that this is first CSSWG spec which is going through this new process). 

Additional background on this feature may be found here: 
https://github.com/w3c/csswg-drafts/issues/769

## Summary
CSS scroll-boundary-behavior standardizes a supercharged version of "-ms-scroll-chaining". It
allows developers to determine the browser's behavior once a scroller has reached its full extent.
The unused delta can be propagated to the parent causing scroll chaining, create a glow/bounce
effect without chaining, or just get consumed silently. In particular when used on viewport
defining element, it controls if overscroll can be used for navigation actions such as pull-to-
refresh or swipe navigation.


scroll-boundary-behavior: auto | contain | none

  * `auto`: propagate scroll to the parent scroller. If there is no parent scroller (e.g.,
    viewport) user-agent may perform a default action (e.g. navigation) or show any appropriate
    overscroll UI affordance. This is the default value.
  * `contain`: do not propagate. The user agent may show an appropriate overscroll UI affordance
    such as bounce or glow.
  * `none`: same as contain but also prevents any overscroll UI affordance e.g. bounce or glow.

There will be scroll-boundary-behavior-{x,y} long-hands to control each axis individually.


## Current State
The specification and API has received positive feedback and support from EdgeHTML [1], Mozilla
[2], Chromium[3], and web developers [4,5]. We at Chromium have an implementation landed in
Canary (behind a feature flag**). If we don't receive any strong negative signal we plan to ship
in M63 per intent-to-ship. [3]
 

[1] https://github.com/w3c/csswg-drafts/issues/769#issuecomment-270228948
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=951793#c11
[3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/OqBNF2efmFA/3ByBUKyaCgAJ
[4] https://github.com/w3c/csswg-drafts/issues/769#issuecomment-279832555
[5] https://github.com/w3c/csswg-drafts/issues/769#issuecomment-288878908

** --enable-web-platform-experimental-features or --enable-blink-features=CSSScrollBoundaryBehavior
Comment 1 Radar WebKit Bug Importer 2017-09-07 13:56:45 PDT
<rdar://problem/34315997>
Comment 2 Benoit Girard 2017-11-22 10:29:26 PST
The property has been renamed to 'overscroll-behavior': https://github.com/WICG/overscroll-behavior

There's been no major changes otherwise.
Comment 3 Simon Fraser (smfr) 2017-12-07 10:45:20 PST
*** Bug 179266 has been marked as a duplicate of this bug. ***
Comment 4 Frédéric Wang (:fredw) 2018-09-10 08:07:08 PDT
Created attachment 349309 [details]
Testcase
Comment 5 Frédéric Wang (:fredw) 2018-09-10 08:07:48 PDT
Created attachment 349310 [details]
WIP Patch (CSS parsing only)
Comment 6 Frédéric Wang (:fredw) 2018-09-26 07:55:08 PDT
Created attachment 350862 [details]
WIP Patch (CSS parsing only)

Use runtime instead of compile time flag
Comment 7 Frédéric Wang (:fredw) 2018-10-31 04:32:04 PDT
Created attachment 353488 [details]
Testcase of annoying scroll chaining

This is a simple use case showing when scroll chaining can be annoying. If you scroll the overflow node to its maximum limit, release it and try to scroll it any further then the scrolling is propagated to the main frame (expected scroll chaining behavior). Then if you don't wait a bit but scroll immediately the overflow node again in the other direction, then the scrolling still applies to the main frame. The "position: fixed" is not essential here, but it can make things even more confusing since one does not see the overflow node moving when the main frame is scrolled.
Comment 8 Frédéric Wang (:fredw) 2018-11-15 05:23:57 PST
Created attachment 354920 [details]
WIP Patch (CSS parsing and preference option)
Comment 9 Frédéric Wang (:fredw) 2018-11-27 07:43:54 PST
Created attachment 355733 [details]
WIP Patch
Comment 10 Frédéric Wang (:fredw) 2018-11-28 10:21:11 PST
Created attachment 355887 [details]
WIP Patch

Just some minor change with respect to the 355733, so that "non-local boundary default actions" (e.g. scroll chaining or navigation) are now blocked for overscroll-behavior: none or contain. For now this works on macOS and GTK on attachment 349309 [details] but iOS uses a different code path.
Comment 11 Frédéric Wang (:fredw) 2018-11-28 10:32:19 PST
Created attachment 355888 [details]
WIP Patch

Sorry, I uploaded the wrong patch. Here is the correct one. Also I forgot to say that this only blocks scrolling via the mouse, not via the keyboard (my chromium release on Linux seems to have the same behavior).
Comment 12 Frédéric Wang (:fredw) 2018-11-29 07:59:53 PST
Created attachment 356006 [details]
Add overscroll behavior property to scrolling tree
Comment 13 Frédéric Wang (:fredw) 2018-12-03 14:50:11 PST
Created attachment 356410 [details]
WIP Patch

@smfr: I tried to experiment with UIScrollView's bounces/alwaysBounceHorizontal/alwaysBounceVertical but unfortunately it does not seem possible to use them to disable/enable bouncing in independent directions. I don't see any straightforward UIScrollView API to disable scroll chaining. I guess UIScrollView should probably have an overscroll property none/contain/auto so that it is possible to enable/disable things like bouncing or scrolling chaining (more generally local and non-local boundary default actions) via the UIKitSPI. I understand this is proprietary code and hence has to be handled by Apple, right? If so, can you please comment on rdar://problem/34315997 ?

This patch passes the horizontal/vertical overscroll behavior to the UI process, but probably it would be cleaner to move the OverscrollBehavior enum in ScrollTypes.h
Comment 14 Frédéric Wang (:fredw) 2018-12-04 03:18:25 PST
Created attachment 356479 [details]
Patch
Comment 15 Frédéric Wang (:fredw) 2018-12-04 09:16:44 PST
Created attachment 356505 [details]
Patch (applies on top of bug 192358)
Comment 16 Frédéric Wang (:fredw) 2018-12-06 01:23:56 PST
There is a WPT manual test at https://w3c-test.org/css/css-overscroll-behavior/overscrollBehavior-manual.html
Comment 17 jonjohnjohnson 2018-12-10 13:57:50 PST
Definitely know I'm being a sort of busy body, but sincerely wondering if the webkit implementation of `overscroll-behavior` using the `contain` (or even `none`) value will allow for the same scroll containment in iOS that is showcased in in the following quite complex and hacked link:

https://bugs.webkit.org/attachment.cgi?id=356972&action=edit
https://bugs.webkit.org/attachment.cgi?id=356972

Meaning, we won't have to resort to such a mess to get this user experience?
Comment 18 Ben Frain 2019-02-12 08:27:50 PST
Really hope this is implemented soon. Any modal on iOS that has scrolling inside suffers terribly currently. Having this would mean an end to toggling overflow hidden every time we want a modal with scrollable content.
Comment 19 Henrik Joreteg 2019-02-21 06:16:02 PST
This is big deal for anybody who's implementing any sort of slide-in "hamburger" menu, etc.

For example, I'm currently working with one of the largest retailers in the world on a UI that involves a shopping cart that slides in from the right so you can quickly see cart contents without leaving your shopping experience.

As you add things to your cart that list will grow to the point where it needs to be scrolled. It's obviously not desirable to scroll the underlying page. 

What amounts to being a simple CSS rule in most browsers currently requires frustrating hacks to get it to working correctly on iOS.

Closely related... the additional requirement for adding '-webkit-overflow-scrolling: touch' is another weird one. I can't imagine a scenario where I'm wanting something to on the page to be scrollable but where I *don't* want momentum scrolling. Frankly, scrolling something without this behavior feels super broken. Seems to me it should just be the default.

Anyway, these scrolling-related issues happen increasingly as developers build more complex web pages and currently, it's one of those things that make developers curse Apple.

I think getting this resolved would buy a lot of goodwill.

Please consider implementing this. Thanks!
Comment 20 Frédéric Wang (:fredw) 2019-02-27 09:20:01 PST
Created attachment 363094 [details]
tentative rebase (not tested)
Comment 21 Dmitry Semenov 2019-04-15 10:58:16 PDT
Would be great if this also allowed to prevent edge swipe to go back gesture.

This would help with horizontal carousels. Users often want to swipe carousel, but swipe to "the previous page" instead. The majority of e-commerce websites use a carousel on the product page.

At least for those that use "native" horizontal scroll with `-webkit-overflow-scrolling: touch`.
Comment 22 jonjohnjohnson 2019-04-15 11:51:03 PDT
(In reply to Dmitry Semenov from comment #21)

As far as...
> This would help with horizontal carousels.
I believe spec has you covered, in that internal horizontal scrolling can be made to never chain out to the native viewport behavior with values `none`/`contain`. 

But for this general case...
> Would be great if this also allowed to prevent edge swipe to go back gesture.
Sadly, I think the current spec covers this general case only if your website already has the viewport horizontally scrolling. See https://github.com/w3c/csswg-drafts/issues/3349

- https://developers.google.com/web/updates/2017/11/overscroll-behavior
- https://drafts.csswg.org/css-overscroll-behavior-1/
Comment 23 jonjohnjohnson 2019-04-19 07:57:42 PDT
(In reply to Frédéric Wang (:fredw))

Beware of https://bugs.chromium.org/p/chromium/issues/detail?id=954423 when implementing this on the viewport scroller. Considering css compat with regards to html/body propagation of this property.
Comment 24 Frédéric Wang (:fredw) 2019-06-28 04:06:44 PDT
It seems there are discussions elsewhere to introduce flow-relative properties:

https://drafts.csswg.org/css-overscroll-1/#overscroll-behavior-longhands-logical

I personally think it makes sense.
Comment 25 Frédéric Wang (:fredw) 2019-08-30 10:08:18 PDT
Created attachment 377722 [details]
tentative rebase (not tested)
Comment 26 Frédéric Wang (:fredw) 2019-08-30 11:13:45 PDT
Created attachment 377735 [details]
tentative rebase (not tested)
Comment 27 Frédéric Wang (:fredw) 2019-08-31 00:10:49 PDT
Created attachment 377789 [details]
Patch

This patch now compiles but I can't seem to make it work anymore with the testcase. This would need more debugging...
Comment 28 Frédéric Wang (:fredw) 2019-09-05 11:17:49 PDT
Created attachment 378100 [details]
Patch

OK, I took a closer look into this today and this version should now again have the same behavior as my initial attempt:

* "overscroll-behavior: contain" blocks scroll chaining and go back gesture on macOS.
* "overscroll-behavior: none" blocks scroll bounces on iOS. AFAIK, we still need progress on rdar://problem/34315997 for complete support (see FIXME in ScrollingTreeScrollingNodeDelegateIOS.mm).
* no support for iframe scrolling yet (I don't remember what was the status and need to check the code, but I suspect we can have the same support as overflow: auto nodes).
Comment 29 Frédéric Wang (:fredw) 2019-09-06 09:15:37 PDT
Created attachment 378196 [details]
Patch
Comment 30 Paulo Sousa 2019-11-21 10:09:45 PST
I need this css property to implement a full screen modal on my site.

In safari : when I'm on the top of my full screen modal, if i firmly scroll down, it will trigger the body scroll rubber effect not my modal scroll effect.

Here's an example: https://pzgcy.csb.app/
Comment 31 Jon Lee 2020-03-13 09:22:23 PDT
rdar://problem/9925689
Comment 32 cathiechen 2020-05-14 04:29:39 PDT
Created attachment 399351 [details]
rebase

Rebase the patch.
Comment 33 cathiechen 2020-05-14 04:57:11 PDT
Created attachment 399352 [details]
rebase
Comment 34 cathiechen 2020-05-14 06:47:11 PDT
Created attachment 399355 [details]
rebase

Rebase Fred's patch.
Comment 35 Dima Voytenko 2020-05-28 14:24:13 PDT
A friendly ping here. It looks like the patches have been available for some time. Could they be reviewed?
Comment 36 Simon Fraser (smfr) 2020-05-28 15:38:46 PDT
Comment on attachment 399355 [details]
rebase

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

I don't see any changes here related to scroll latching, which I'd expect are necessary to stop scroll propagation. For example, I would expect some code in ScrollingTree::handleWheelEvent/ScrollingTreeLatchingController.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3044
> +        case CSSPropertyOverscrollBehavior:
> +            return cssValuePool.createValue(std::max(style.overscrollBehaviorX(), style.overscrollBehaviorY()));
> +        case CSSPropertyOverscrollBehaviorX:
> +            return cssValuePool.createValue(style.overscrollBehaviorX());
> +        case CSSPropertyOverscrollBehaviorY:
> +            return cssValuePool.createValue(style.overscrollBehaviorY());

This code should check if the feature is enabled, right?

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2209
> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(OverscrollBehavior e)

e -> behavior

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:595
> +        return valueID == CSSValueAuto || valueID == CSSValueContain || valueID == CSSValueNone;

Does this need to check overscrollBehaviorEnabled ?

> Source/WebCore/page/EventHandler.cpp:305
> +static bool didScrollInScrollableArea(ScrollableArea& scrollableArea, double deltaX, double deltaY, unsigned deltaMode)

Can we make deltaMode be an enum first, please? It's hard to tell what it means in this context.

> Source/WebCore/page/FrameView.cpp:428
> +    if (RenderWidget* frameRenderer = frame().ownerRenderer())
> +        return frameRenderer->style().overscrollBehaviorX();

This is pretty weird; style in the parent document can affect overscroll in a subframe (any subframe, possibly cross-origin?). That doesn't seem right. Overscroll's impact should be limited to the current frame. I filed https://github.com/w3c/csswg-drafts/issues/5129

Certainly this code is wrong because the parent frame's style may not have been updated. The parent frame needs to push some state down onto this FrameView, if this is something we need to support.
Comment 37 cathiechen 2020-07-10 10:21:55 PDT
Comment on attachment 399355 [details]
rebase

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

Hi Simon,
Sorry for the slow reply.
I'll upload a WIP patch later. This patch fixes the code review and supports overscroll-behavior for Mac async scroll.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3044
>> +            return cssValuePool.createValue(style.overscrollBehaviorY());
> 
> This code should check if the feature is enabled, right?

Yes, we should check settings for this.
Added `"settings-flag": "overscrollBehavior"` to CSSProperties.json, so the propertyID of "overscrollBehavior" is invalid if the feature is disabled.
Added "overscrollBehavior" valid check test for this.

>> Source/WebCore/css/CSSPrimitiveValueMappings.h:2209
>> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(OverscrollBehavior e)
> 
> e -> behavior

Done

>> Source/WebCore/css/parser/CSSParserFastPaths.cpp:595
>> +        return valueID == CSSValueAuto || valueID == CSSValueContain || valueID == CSSValueNone;
> 
> Does this need to check overscrollBehaviorEnabled ?

Yes, done.

>> Source/WebCore/page/EventHandler.cpp:305
>> +static bool didScrollInScrollableArea(ScrollableArea& scrollableArea, double deltaX, double deltaY, unsigned deltaMode)
> 
> Can we make deltaMode be an enum first, please? It's hard to tell what it means in this context.

OK, restored WheelEvent& and added a struct to carry ignore info here.

>> Source/WebCore/page/FrameView.cpp:428
>> +        return frameRenderer->style().overscrollBehaviorX();
> 
> This is pretty weird; style in the parent document can affect overscroll in a subframe (any subframe, possibly cross-origin?). That doesn't seem right. Overscroll's impact should be limited to the current frame. I filed https://github.com/w3c/csswg-drafts/issues/5129
> 
> Certainly this code is wrong because the parent frame's style may not have been updated. The parent frame needs to push some state down onto this FrameView, if this is something we need to support.

Yes, make sense. I think it seems reasonable to limit the impact in the current frame unless the specification clarify this.
Added FIXME in here.
Comment 38 cathiechen 2020-07-10 10:23:43 PDT
Created attachment 403978 [details]
WIP-patch-based-on-Fred's-patch
Comment 39 EWS Watchlist 2020-07-10 10:24:24 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 40 cathiechen 2020-07-10 20:53:19 PDT
Created attachment 404040 [details]
WIP-patch-based-on-Fred's-patch
Comment 41 Frédéric Wang (:fredw) 2020-07-10 21:55:01 PDT
Comment on attachment 404040 [details]
WIP-patch-based-on-Fred's-patch

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

I think it would be good to have more tests. Do we have more WPT tests we can use?

> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable-expected.txt:1
> +Pass test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.

invalidated

> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable.html:9
> +            document.write("Fail test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.");

invalidated

> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable.html:11
> +            document.write("Pass test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.");

I think you can simplify this: 

var result = "overscrollBehavior" in document.documentElement.style ? "PASS" : "FAIL";
document.write(`${result} test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.`);

> LayoutTests/fast/scrolling/overscroll-behavior-validate-if-enable-expected.txt:1
> +Pass test overscrollBehavior should be validate if overscrollBehaviorEnabled is enabled.

validated

> LayoutTests/fast/scrolling/overscroll-behavior-validate-if-enable.html:11
> +            document.write("Fail test overscrollBehavior should be validate if overscrollBehaviorEnabled is enabled.");

Same here.

> Source/WebCore/page/FrameView.cpp:436
> +    auto* document = frame().document();

I guess the FIXME applies here too.
Comment 42 cathiechen 2020-07-12 07:34:36 PDT
Comment on attachment 404040 [details]
WIP-patch-based-on-Fred's-patch

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

Hi Fred,
Thanks for the review:)

>> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable-expected.txt:1
>> +Pass test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.
> 
> invalidated

Done

>> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable.html:9
>> +            document.write("Fail test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.");
> 
> invalidated

Done

>> LayoutTests/fast/scrolling/overscroll-behavior-invalidate-if-disable.html:11
>> +            document.write("Pass test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.");
> 
> I think you can simplify this: 
> 
> var result = "overscrollBehavior" in document.documentElement.style ? "PASS" : "FAIL";
> document.write(`${result} test overscrollBehavior should be invalidate if overscrollBehaviorEnabled is disabled.`);

Done, thanks:)

>> LayoutTests/fast/scrolling/overscroll-behavior-validate-if-enable-expected.txt:1
>> +Pass test overscrollBehavior should be validate if overscrollBehaviorEnabled is enabled.
> 
> validated

Done

>> LayoutTests/fast/scrolling/overscroll-behavior-validate-if-enable.html:11
>> +            document.write("Fail test overscrollBehavior should be validate if overscrollBehaviorEnabled is enabled.");
> 
> Same here.

Done

>> Source/WebCore/page/FrameView.cpp:436
>> +    auto* document = frame().document();
> 
> I guess the FIXME applies here too.

Done
Comment 43 cathiechen 2020-07-12 07:35:20 PDT
Created attachment 404095 [details]
WIP-patch-based-on-Fred's-patch
Comment 44 cathiechen 2020-07-12 08:54:49 PDT
Created attachment 404100 [details]
WIP-patch-based-on-Fred's-patch