Bug 78664 – Add -webkit-overflow-scrolling CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sami Kyostila]

\Add -webkit-overflow-scrolling CSS property

[Sami Kyostila]

Created attachment 127106 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127106 [details]
Patch

Attachment 127106 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11518659

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Eric Seidel (no email)]

Comment on attachment 127106 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Add a CSS property indicating that an element with overflow scrolling
> +        should follow the platform's behavior for touch scrollable user
> +        interface objects. For instance, this property could enable momentum
> +        scrolling for the element if that is the platform convention.

Could you provide a bit more context here?  Is this a CSS property that iOS WebKit supports?  Is this a new property?  Is there a standard in the works?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2124
> +        case CSSPropertyWebkitOverflowScrolling:
> +            return style->useTouchOverflowScrolling() ? CSSPrimitiveValue::createIdentifier(CSSValueTouch) : CSSPrimitiveValue::createIdentifier(CSSValueAuto);

It seems like we're supposed to use the cssValuePool these days?

> Source/WebCore/css/CSSStyleSelector.cpp:1891
> +    bool hasTouchOverflowScrolling = (style->overflowX() != OHIDDEN || style->overflowY() != OHIDDEN) && style->useTouchOverflowScrolling();

Should this just be a helper method on style?  sytle->hasTouchOverflowScrolling()?  That might be confusing with "useTouchOverflowScrolling".  I'm not sure exactly what this code is tryign to do.

[Adam Barth]

> svg/css/getComputedStyle-basic.xhtml
> fast/css/getComputedStyle/computed-style.html
> fast/css/getComputedStyle/computed-style-without-renderer.html

By the way, these tests commonly fail whenever you introduce a new CSS property.  The correct resolution is to update the expected results to reflect the new property.

[WebKit Review Bot]

Comment on attachment 127106 [details]
Patch

Attachment 127106 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11515631

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Simon Fraser (smfr)]

Given the recent backlash around -webkit properties, we should think about how to standardize this property.

[Simon Fraser (smfr)]

Comment on attachment 127106 [details]
Patch

Two points:
1. I think you should mail webkit-dev saying that you intend to add this property.
2. I wonder if this stuff should be behind an #ifdef since it only makes sense on touch-enabled platforms.

[Antonio Gomes]

This property came as iOS specific solution, since they did not kinetic scroll in-region scrollable areas (e.g. scrollable divs) by default. Other webkit-based browsers only support it, because web authors use it for iOS.

In the blackberry port, we support it as a matter of feature detection, but we kinetic scroll "divs" (ok, not only divs) by default.

So do we really want that, or we can just in fact drop it, if iOS implements its behavior by default for all scrollable divs?

[Adam Barth]

> Given the recent backlash around -webkit properties, we should think about how to standardize this property.

Indeed.  However, given that this feature has been shipping with this name in iOS WebKit for some time, I don't believe that should block this patch.

> 1. I think you should mail webkit-dev saying that you intend to add this property.

Will do.

> 2. I wonder if this stuff should be behind an #ifdef since it only makes sense on touch-enabled platforms.

That seems quite sensible.

@Sami: Would you be willing to wrap the feature in an ENABLE(OVERFLOW_SCROLLING) ifdef?

[Adam Barth]

Comment on attachment 127106 [details]
Patch

I'm marking this R- because the patch doesn't have an ENABLE flag.

[Sami Kyostila]

(In reply to comment #3)
> Could you provide a bit more context here?  Is this a CSS property that iOS WebKit supports?  Is this a new property?  Is there a standard in the works?

This property is indeed the one introduced by iOS 5 WebKit. Another implementation is in Chrome for Android.

The reason why this needs to be opt-in rather than something that is done for all overflow divs is that an efficient implementation wants to split the scrolling content into its own compositing layer so that it can be translated without having to be repainted. This also gives the div a stacking context, which is not what a simple "overflow: scroll" should do.

I agree this should be standardized to make sure we all agree on the semantics. I'm not aware of any formal specification other than the mention here:

https://developer.apple.com/library/safari/#documentation/appleapplications/reference/SafariCSSRef/Articles/StandardCSSProperties.html

I'm new to drafting web standards so I'm open to suggestions here.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2124
> > +        case CSSPropertyWebkitOverflowScrolling:
> > +            return style->useTouchOverflowScrolling() ? CSSPrimitiveValue::createIdentifier(CSSValueTouch) : CSSPrimitiveValue::createIdentifier(CSSValueAuto);
> 
> It seems like we're supposed to use the cssValuePool these days?

I'll look into it.

> > Source/WebCore/css/CSSStyleSelector.cpp:1891
> > +    bool hasTouchOverflowScrolling = (style->overflowX() != OHIDDEN || style->overflowY() != OHIDDEN) && style->useTouchOverflowScrolling();
> 
> Should this just be a helper method on style?  sytle->hasTouchOverflowScrolling()?  That might be confusing with "useTouchOverflowScrolling".  I'm not sure exactly what this code is tryign to do.

It's only giving the element a stacking context if it 1) has non-hidden overflow and 2) specifies this new property. On second thought, it might be clearer if the stacking context is always created regardless of whether overflow is hidden or not. I'll rework this part.

[Sami Kyostila]

(In reply to comment #7)
> 1. I think you should mail webkit-dev saying that you intend to add this property.

Good idea.

> 2. I wonder if this stuff should be behind an #ifdef since it only makes sense on touch-enabled platforms.

Sure, I can do that. That said, I can also see the utility of supporting just the property (without any special scrolling interaction) on all platforms because of the stacking context creation. This way sites using this property would always be laid out correctly.

[Sami Kyostila]

(In reply to comment #8)
> This property came as iOS specific solution, since they did not kinetic scroll in-region scrollable areas (e.g. scrollable divs) by default. Other webkit-based browsers only support it, because web authors use it for iOS.
> 
> In the blackberry port, we support it as a matter of feature detection, but we kinetic scroll "divs" (ok, not only divs) by default.
> 
> So do we really want that, or we can just in fact drop it, if iOS implements its behavior by default for all scrollable divs?

There are two issues here: supporting kinetic/momentum scrolls for overflow elements, and being able to scroll overflow elements more efficiently using the compositor. I agree that for the first part does not require any new markup and browsers can just implement it for all scrollable things (that's what we also do in Chrome for Android).

The second part is trickier since promoting the overflow content into a separate compositing layer also gives it a stacking context (similarly to 3D transforms and opacity). Doing this to all overflow elements would break pages, which is why the mechanism needs to be opt-in.

[Antonio Gomes]

(In reply to comment #13)
> (In reply to comment #8)
> > This property came as iOS specific solution, since they did not kinetic scroll in-region scrollable areas (e.g. scrollable divs) by default. Other webkit-based browsers only support it, because web authors use it for iOS.
> > 
> > In the blackberry port, we support it as a matter of feature detection, but we kinetic scroll "divs" (ok, not only divs) by default.
> > 
> > So do we really want that, or we can just in fact drop it, if iOS implements its behavior by default for all scrollable divs?
> 
> There are two issues here: supporting kinetic/momentum scrolls for overflow elements, and being able to scroll overflow elements more efficiently using the compositor. I agree that for the first part does not require any new markup and browsers can just implement it for all scrollable things (that's what we also do in Chrome for Android).

so you are saying you want to ask web author to specify in their HTML: "hey scroll this div fast"? When would not he want it?

It is up to the implementation to actually optimize it, in a lower level, even for huge divs, it is possible.

[Sami Kyostila]

(In reply to comment #14)
> so you are saying you want to ask web author to specify in their HTML: "hey scroll this div fast"? When would not he want it?
> 
> It is up to the implementation to actually optimize it, in a lower level, even for huge divs, it is possible.

In an ideal world there would be no need for this property, but in practice I don't believe we can start pulling out arbitrary page elements into layers without complicated logic and more importantly complicated failure modes (e.g., why isn't this element scrolling fast) -- but I invite you to prove me wrong :)

I think grouping things by stacking contexts is already well established for transforms and opacity and developers will know what to expect.

[Noam Rosenthal]

> so you are saying you want to ask web author to specify in their HTML: "hey scroll this div fast"? When would not he want it?

In a layer that scrolls automatically and not based on user input; like an automatically rolling news-feed.

The hint is "The user is going to scroll this div" rather than "something is going to scroll this div"; And that is a web-developer concern rather than an implementation concern.

[Antonio Gomes]

(In reply to comment #16)
> > so you are saying you want to ask web author to specify in their HTML: "hey scroll this div fast"? When would not he want it?
> 
> In a layer that scrolls automatically and not based on user input; like an automatically rolling news-feed.
> 
> The hint is "The user is going to scroll this div" rather than "something is going to scroll this div"; And that is a web-developer concern rather than an implementation concern.

Valid case and I agree.

I still do think introducing another -webkit-xxx prefix without a broad accordance about it is the way to go. Is it planned to be standardized. What does mozilla, ie and opera think? Etc..

I need this functionally, but I am not sure this css properly is the long term way to go. Please correct if I am wrong.

[Antonio Gomes]

> Valid case and I agree.
> 
> I still do think introducing another -webkit-xxx prefix without a broad accordance about it is the way to go. Is it planned to be standardized? What does mozilla, ie and opera think? Etc..

... I still do *not* think...

[Adam Barth]

In any case, having this behind a compile-time flag is a prudent way to proceed regardless of whether folks with non-touch browsers decide to enable it.

> I still do think introducing another -webkit-xxx prefix without a broad accordance about it is the way to go. Is it planned to be standardized. What does mozilla, ie and opera think? Etc..

While I agree that we should be more reticent to introduce -webkit- prefixed CSS features in light of Mozilla's recent blog posts, I'm not sure that concern applies in this case because this property has been shipping in iOS WebKit for a while and therefore isn't really "new".

[Adam Barth]

Antonio, if you have further thoughts on this matter, it might be more productive to reply to the webkit-dev thread on this topic.  If you have specific technical insights about this patch, those would be appropriate to share on this bug.

[Sami Kyostila]

Created attachment 127261 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 127261 [details]
Patch

Looks reasonable.  What else uses this property, since clearly this is only parsing code?

[Sami Kyostila]

(In reply to comment #22)
> Looks reasonable.  What else uses this property, since clearly this is only parsing code?

Thanks.

Right, these are just the parser bits. The next step is to bring in the actual functionality, which is roughly:

1) Extracting touch scrollable elements into separate GraphicsLayers.

2) Registering scrollable layers with the scroll coordinator to allow the platform to implement the scrolling interaction.

3) Various optimizations to avoid unnecessarily invalidating scrollable layer contents.

4) Computing an input event region for scrollable layers so that threaded scrolling won't interfere with JS callbacks.

I'll link the respective patches to this bug so folks at home can keep score.

[WebKit Review Bot]

Comment on attachment 127261 [details]
Patch

Clearing flags on attachment: 127261

Committed r107881: <http://trac.webkit.org/changeset/107881>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

I think we need to revert the ENABLE(OVERFLOW_SCROLLING) part of the patch in Chromium, since that causes overflow to create new stacking context which changes the z-ordering on desktop pages: http://code.google.com/p/chromium/issues/detail?id=128325.  We have to figure out how to get this code landed and keep WebKit's behavior fairly consistent without changing breaking a ton of pages.

[James Robinson]

It also looks like the logic this patch added to CSSStyleSelector.cpp doesn't quite match the iOS5 code drop :(