With tests such as the attached, GraphicsLayer does not receive a setNeedsDisplay for the ScrollView's content area, leading to scrolling artifacts.
Created attachment 70185 [details] Test case
Created attachment 70187 [details] Patch for the purposes of discussion only.
Created attachment 70190 [details] Issue invalidate in the right coordinate system. Oops.
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.
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.
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.
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.
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.
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.
<rdar://problem/8554258>
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.
Created attachment 70994 [details] Patch illustrating the correct approach This is closer to the correct solution, but see the FIXMEs.
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.
google.com is a bad thing to use in an iframe because it takes over the main frame if you're signed in.
The patch doesn't work because it calls invalidateRect(), which does not invalidate compositing layers.
Created attachment 71494 [details] Simon's patch, but repaint only the iframe on the parent renderer.
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.
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.
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?
http://trac.webkit.org/changeset/70509