Bug 100524

Created attachment 170937 [details] Patch

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

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

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

Created attachment 172536 [details] Patch Rebased now that 97801 landed.

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?

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.

Created attachment 172578 [details] Patch

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?

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.

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

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

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

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?

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

Created attachment 173916 [details] Patch

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

Created attachment 173927 [details] Patch Patch for landing.

Comment on attachment 173927 [details] Patch Clearing flags on attachment: 173927 Committed r134456: <http://trac.webkit.org/changeset/134456>

All reviewed patches have been landed. Closing bug.