Bug 47391 – Fix scrolling of noncomposited iframes within composited document

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nat Duca]

With tests such as the attached, GraphicsLayer does not receive a setNeedsDisplay for the ScrollView's content area, leading to scrolling artifacts.

[Nat Duca]

Created attachment 70185 [details]
Test case

[Nat Duca]

Created attachment 70187 [details]
Patch for the purposes of discussion only.

[Nat Duca]

Created attachment 70190 [details]
Issue invalidate in the right coordinate system. Oops.

[Simon Fraser (smfr)]

Comment on attachment 70190 [details]
Issue invalidate in the right coordinate system. Oops.

On Mac, redrawing of composited iframes on scrolling happens via the swizzled setNeedsDisplayInRect: method in WebTMLView.mm. It is broken on Windows and in WebKit2, so something like this patch is needed.

However, this patch adds an invalidate for every scroll, not just scrolls of iframes in compositing layers. We certainly don't want that.

[Nat Duca]

Haha! Good point. Hmm....

(In reply to comment #4)
> (From update of attachment 70190 [details])
> On Mac, redrawing of composited iframes on scrolling happens via the swizzled setNeedsDisplayInRect: method in WebTMLView.mm. It is broken on Windows and in WebKit2, so something like this patch is needed.
> 
> However, this patch adds an invalidate for every scroll, not just scrolls of iframes in compositing layers. We certainly don't want that.

[Nat Duca]

Created attachment 70663 [details]
Only issue invalidates for layers that have owner renderer's who have a layer that itself is composited. The check for the layer is important for iframes that render directly onto the window, I think.

[Simon Fraser (smfr)]

Comment on attachment 70663 [details]
Only issue invalidates for layers that have owner renderer's who have a layer that itself is composited. The check for the layer is important for iframes that render directly onto the window, I think.

I'd like to try this in composited iframes in webkit2 on mac (or old webkit on windows), but don't have time at present.

[Nat Duca]

I hear you... I'd test myself but I'm new enough to the team to not have either VS2005 or a mac yet.

Given that, do you want to wait on trying the patch yourself before we land this? Or, is it low-enough risk in terms of breakage that it can land anyway?

I'm fine either way, just trying to decide on next steps given I can't do much myself.

(In reply to comment #7)
> (From update of attachment 70663 [details])
> I'd like to try this in composited iframes in webkit2 on mac (or old webkit on windows), but don't have time at present.

[Simon Fraser (smfr)]

Comment on attachment 70663 [details]
Only issue invalidates for layers that have owner renderer's who have a layer that itself is composited. The check for the layer is important for iframes that render directly onto the window, I think.

This doesn't fix webkit2 iframe scrolling on Mac, so is likely not correct.

[Simon Fraser (smfr)]

<rdar://problem/8554258>

[Simon Fraser (smfr)]

Here's the stack where the problem occurs:

#1  0x0000000100203302 in WebKit::ChunkedUpdateDrawingArea::scroll
#2  0x0000000100248037 in WebKit::WebChromeClient::scroll
#3  0x0000000100fa1129 in WebCore::Chrome::scroll
#4  0x00000001012bd06b in WebCore::FrameView::scrollContentsFastPath
#5  0x00000001019e6808 in WebCore::ScrollView::scrollContents
#6  0x00000001019e6a1e in WebCore::ScrollView::valueChanged
#7  0x00000001012bcccc in WebCore::FrameView::valueChanged
#8  0x00000001019dd6b4 in WebCore::Scrollbar::setCurrentPos
#9  0x00000001019dd867 in WebCore::Scrollbar::setValue

The bug is that ScrollView::scrollContents() always call through to hostWindow to invalidate on scrolling. If the iframe is composited, it needs to repaint via a RenderObject.

[Simon Fraser (smfr)]

Created attachment 70994 [details]
Patch illustrating the correct approach

This is closer to the correct solution, but see the FIXMEs.

[Nat Duca]

I like this approach better. Thanks. :) It definitely didn't feel clean to wedge this code into the scrollChanged path... pushing the slowScroll to FrameView makes a lot of sense to me.

However, I do have a basic question for my own education -- you said that attachment 70663 [details] did not fix iframe scrolling on webkit2. However, when I apply that patch to webkit2 on my mac, it actually does fix things. Is there a use case that I'm not trying out or something?

Not trying to be cantankerous, just trying to understand what the algorithmic change is, versus stylistic.

[Simon Fraser (smfr)]

google.com is a bad thing to use in an iframe because it takes over the main frame if you're signed in.

[Simon Fraser (smfr)]

The patch doesn't work because it calls invalidateRect(), which does not invalidate compositing layers.

[Nat Duca]

Created attachment 71494 [details]
Simon's patch, but repaint  only the iframe on the parent renderer.

[Nat Duca]

Regarding two of the fixme's you added...

The hostWindow()->invalidateWindow() calls in ScrollViewContents:
- Not sure I follow your point here; if I could get to understanding the distinction you're making, I'm ok doing a refactor.

The pan-scroll issue:
Great point. I'm happy to file and take ownership of a separate bug for this problem.

[Simon Fraser (smfr)]

Comment on attachment 71494 [details]
Simon's patch, but repaint  only the iframe on the parent renderer.

I'm not sure updateRect is correct for transformed iframes in compositing layers.

[Simon Fraser (smfr)]

Created attachment 71822 [details]
Patch

Comment on attachment 71822 [details]
Patch

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

> WebCore/page/FrameView.cpp:1011
> +            IntRect rect(0, 0, visibleWidth(), visibleHeight());
> +            // Add borders and padding.
> +            rect.move(frameRenderer->borderLeft() + frameRenderer->paddingLeft(),
> +                      frameRenderer->borderTop() + frameRenderer->paddingTop());

How about initializing the rect with the correct origin?

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/70509