Bug 100524 – Allow painting outside overflow clip in accelerated scrolling layers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sami Kyöstilä]

Allow painting outside overflow clip in accelerated scrolling layers

[Sami Kyöstilä]

Created attachment 170937 [details]
Patch

[Sami Kyöstilä]

This was split off bug 96087. Requires test support from bug 97801.

[Build Bot]

Comment on attachment 170937 [details]
Patch

Attachment 170937 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14612095

New failing tests:
compositing/overflow/updating-scrolling-content.html

[WebKit Review Bot]

Comment on attachment 170937 [details]
Patch

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

New failing tests:
platform/chromium/virtual/softwarecompositing/overflow/updating-scrolling-content.html

[Sami Kyöstilä]

Created attachment 172536 [details]
Patch

Rebased now that 97801 landed.

[Antonio Gomes]

Comment on attachment 172536 [details]
Patch

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

Looks good. One question:

> Source/WebCore/rendering/RenderBox.cpp:656
> +    if (hasOverflowClip() && hasLayer() && layer()->usesCompositedScrolling())

does hasOverflowClip imply a having a layer? If so, should we extract hasOverflowClip and usesCompositedScrolling into a helper method?

[Sami Kyöstilä]

Comment on attachment 172536 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:656
>> +    if (hasOverflowClip() && hasLayer() && layer()->usesCompositedScrolling())
> 
> does hasOverflowClip imply a having a layer? If so, should we extract hasOverflowClip and usesCompositedScrolling into a helper method?

Yes, I believe it does. Good idea, this makes things a little neater.

[Sami Kyöstilä]

Created attachment 172578 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 172578 [details]
Patch

I'm confused about why we need this patch. Doesn't the ShouldRespectOverflowClip stuff in RenderLayer take care of this?

[Sami Kyöstilä]

The reason why we also need this is to make sure we don't lose any updates to the contents of the scrolling GraphicsLayer. Consider this case where A and B form the scrolling content. The A part is currently inside the overflow clip (i.e., visible) and B is hidden:

    +---+
    | A |
    +---+
    : B :
    +...+

Since A is visible, it will be painted to the scrolling GraphicsLayer. Now if we scroll down, the situation becomes this:

    +...+
    : A :
    +---+
    | B |
    +---+

Now both A and B have been painted to the GraphicsLayer. Suppose we now change the A content to C. Without the patch, nothing happens because the paint operation is outside the overflow clip. With the patch, the new content is properly updated:

    w/patch      w/o patch
    =======      =========

    +...+        +...+
    : C :        : A :
    +---+        +---+
    | B |        | B |
    +---+        +---+

Finally let's scroll back up. Since scrolling no longer invalidates the content, without the patch we see the old A instead the new C:

    +---+        +---+
    | C |        | A |
    +---+        +---+
    : B :        : B :
    +...+        +...+

I'm open to suggestions if you have a better way of solving this problem.

[Simon Fraser (smfr)]

Do your tests show that this problem exists on iOS 6?

[Sami Kyöstilä]

(In reply to comment #11)
> Do your tests show that this problem exists on iOS 6?

iOS 6 has a similar fix for this bug (see RenderBox::computeRectForRepaint), but it doesn't cover all the cases. For example, a text selection isn't painted outside the overflow clip, but I think iOS uses a different way to highlight selected text so the problem doesn't show up.

[Sami Kyöstilä]

Simon, do you think there's something that needs to be done for this patch?

[Simon Fraser (smfr)]

Comment on attachment 172578 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:2757
> +    if (!isRoot() && !usesCompositedScrolling()) {

This seems a bit heavy-handed. Maybe it would be better to provide a variant of visualOverflowRect() that checks for composited scrolling? This is also hot code so beware of perf regressions.

> Source/WebCore/rendering/RenderBox.cpp:622
> +    return hasOverflowClip() && layer()->usesCompositedScrolling();

There are some cases where hasOverflowClip() does not guarantee that you have a layer(), so you should check hasLayer() too.

> Source/WebCore/rendering/RenderBox.cpp:662
>      paintRect.move(-scrolledContentOffset()); // For overflow:auto/scroll/hidden.
>  
> +    // Do not clip scroll layer contents to reduce the number of repaints while scrolling.
> +    if (usesCompositedScrolling())
> +        return;

Why bother to move the paintRect if you're going to return?

[Sami Kyöstilä]

(In reply to comment #14)
> (From update of attachment 172578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172578&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:2757
> > +    if (!isRoot() && !usesCompositedScrolling()) {
> 
> This seems a bit heavy-handed. Maybe it would be better to provide a variant of visualOverflowRect() that checks for composited scrolling? This is also hot code so beware of perf regressions.

I took your suggestion and now check against the union rect of visual and layout overflow for composited scrolling. While doing this I noticed RenderTable also does similar paint rejection, but since I wasn't able to make composited scrolling trigger that path I opted not to change it.

> > Source/WebCore/rendering/RenderBox.cpp:622
> > +    return hasOverflowClip() && layer()->usesCompositedScrolling();
> 
> There are some cases where hasOverflowClip() does not guarantee that you have a layer(), so you should check hasLayer() too.

Thanks, I wasn't aware of this.

> > Source/WebCore/rendering/RenderBox.cpp:662
> >      paintRect.move(-scrolledContentOffset()); // For overflow:auto/scroll/hidden.
> >  
> > +    // Do not clip scroll layer contents to reduce the number of repaints while scrolling.
> > +    if (usesCompositedScrolling())
> > +        return;
> 
> Why bother to move the paintRect if you're going to return?

paintRect is a reference and we always need to apply the scroll offset to it.

[Sami Kyöstilä]

Created attachment 173916 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 173916 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:622
> +    return hasOverflowClip() && layer() && layer()->usesCompositedScrolling();

use hasLayer() (it's a bit-check).

[Sami Kyöstilä]

Created attachment 173927 [details]
Patch

Patch for landing.

[WebKit Review Bot]

Comment on attachment 173927 [details]
Patch

Clearing flags on attachment: 173927

Committed r134456: <http://trac.webkit.org/changeset/134456>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.