Bug 120380 – [iOS] Don't opt into accelerated composited scrolling for elements with -webkit-overflow-scrolling: touch

• Status: NEW
• Component: Layout and Rendering

[Daniel Bates]

On iOS we don't want to opt into using the accelerated composited scrolling code path for elements with touch overflow scrolling because we use UIKit to handle overflow scrolling.

[Daniel Bates]

Created attachment 209816 [details]
Patch

[Daniel Bates]

Created attachment 209818 [details]
Patch with layout tests

[Darin Adler]

Comment on attachment 209818 [details]
Patch with layout tests

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

> Source/WebCore/rendering/RenderLayer.cpp:2060
> +        // On iOS we don't want to opt into accelerated composited scrolling, which creates scroll bar
> +        // layers in WebCore, because we use UIKit to composite our scroll bars.

This comment is in the wrong place. It should go before the #if, not in the #else side.

I also think that && !PLATFORM(IOS) should go *after* the ENABLE check.

What the comment does not explain is why ENABLE(ACCELERATED_OVERFLOW_SCROLLING) needs to be true for iOS even though we don’t want to do this. Is there other code wrapped by ACCELERATED_OVERFLOW_SCROLLING we do want?

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 209818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209818&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:2060
> > +        // On iOS we don't want to opt into accelerated composited scrolling, which creates scroll bar
> > +        // layers in WebCore, because we use UIKit to composite our scroll bars.
> 
> This comment is in the wrong place. It should go before the #if, not in the #else side.

Will move.

> 
> I also think that && !PLATFORM(IOS) should go *after* the ENABLE check.
> 
> What the comment does not explain is why ENABLE(ACCELERATED_OVERFLOW_SCROLLING) needs to be true for iOS even though we don’t want to do this. 

Maybe using !USE(UIKIT) or !USE(UIKIT_SCROLLING)  or !USE(IOS_SCROLLING) would be more descriptive than !PLATFORM(IOS) in the proposed patch? That is, on iOS we use UIKit for hardware accelerated scrolling of overflow elements instead of the non-accelerated/accelerated compositing code path in WebCore; => We do not want to opt into the ENABLE(ACCELERATED_OVERFLOW_SCROLLING) guarded code in RenderLayer::updateNeedsCompositedScrolling() (since it is specific to using -webkit-overflow-scroll with the non-UIKit scrolling code path in WebCore).

Alternatively, maybe a better description for this comment would be:

// On iOS we use UIKit for hardware accelerated scrolling of overflow elements instead of the accelerated
// compositing code path in WebCore. So, we never want to force such compositing in WebCore.

On another note, maybe there is a better way to structure this code such that we differentiate the concept of ACCELERATED_OVERFLOW_SCROLLING ("support for CSS property -webkit-overflow-scrolling") from the implementation used for scrolling (e.g. UIKit)?

Is there other code wrapped by ACCELERATED_OVERFLOW_SCROLLING we do want?

Yes, there is more code wrapped by ACCELERATED_OVERFLOW_SCROLLING that we do want.

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 209818 [details])
> [...]
> I also think that && !PLATFORM(IOS) should go *after* the ENABLE check.

Will move the conjunct !PLATFORM(IOS) after the conjunct ENABLE(ACCELERATED_OVERFLOW_SCROLLING).

[Darin Adler]

(In reply to comment #4)
> Maybe using !USE(UIKIT) or !USE(UIKIT_SCROLLING)  or !USE(IOS_SCROLLING) would be more descriptive than !PLATFORM(IOS) in the proposed patch?

Yes, might be a way to improve this later, but not if this is the only place we have such an #if.

> On another note, maybe there is a better way to structure this code such that we differentiate the concept of ACCELERATED_OVERFLOW_SCROLLING ("support for CSS property -webkit-overflow-scrolling") from the implementation used for scrolling (e.g. UIKit)?

I don’t think that support for the CSS property -webkit-overflow-scrolling should be considered *accelerated* overflow scrolling, so I think you are describing it wrong.

>> Is there other code wrapped by ACCELERATED_OVERFLOW_SCROLLING we do want?
> 
> Yes, there is more code wrapped by ACCELERATED_OVERFLOW_SCROLLING that we do want.

OK, too bad.

[Simon Fraser (smfr)]

We should ask other webkit clients if they care about accelerated scrolling. Maybe our UIKit version is the only one we need to maintain.

[Antonio Gomes]

(In reply to comment #7)
> We should ask other webkit clients if they care about accelerated scrolling. Maybe our UIKit version is the only one we need to maintain.

BlackBerry used to care. Arvid, Jacky?

[Arvid Nilsson]

(In reply to comment #8)
> (In reply to comment #7)
> > We should ask other webkit clients if they care about accelerated scrolling. Maybe our UIKit version is the only one we need to maintain.
> 
> BlackBerry used to care. Arvid, Jacky?

I believe our accelerated scrolling code isn't fully upstreamed, i.e. there are some #if BLACKBERRY's that we haven't upstreamed. And since we haven't upstreamed all of it it wouldn't be fair to ask the upstream community to maintain this bit of code for us... What do you think Jacky?

[Jacky Jiang]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > We should ask other webkit clients if they care about accelerated scrolling. Maybe our UIKit version is the only one we need to maintain.
> > 
> > BlackBerry used to care. Arvid, Jacky?
> 
> I believe our accelerated scrolling code isn't fully upstreamed, i.e. there are some #if BLACKBERRY's that we haven't upstreamed. And since we haven't upstreamed all of it it wouldn't be fair to ask the upstream community to maintain this bit of code for us... What do you think Jacky?

I agree with Arvid.

[Daniel Bates]

(In reply to comment #6)
> (In reply to comment #4)
> [...]
> I don’t think that support for the CSS property -webkit-overflow-scrolling should be considered *accelerated* overflow scrolling, so I think you are describing it wrong.

Currently we guard the CSS property -webkit-overflow-scrolling behind the ENABLE(ACCELERATED_OVERFLOW_SCROLLING) guard.

From talking with Simon Fraser today on IRC, it only makes sense to support the CSS property -webkit-overflow-scrolling together with accelerated composited scrolling as the purpose of this property is for developers to explicitly opt into hardware accelerated, native-style, scrolling. That is, it doesn't make sense to support this CSS property with repaint-based scrolling.