Bug 47391

Summary: Fix scrolling of noncomposited iframes within composited document
Product: WebKit Reporter: Nat Duca <nduca>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: simon.fraser, vangelis
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Attachments:
Description Flags
Test case
none
Patch for the purposes of discussion only.
none
Issue invalidate in the right coordinate system. Oops.
none
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.
none
Patch illustrating the correct approach
none
Simon's patch, but repaint only the iframe on the parent renderer.
none
Patch mitz: review+

Description Nat Duca 2010-10-07 19:02:35 PDT
With tests such as the attached, GraphicsLayer does not receive a setNeedsDisplay for the ScrollView's content area, leading to scrolling artifacts.
Comment 1 Nat Duca 2010-10-07 19:03:25 PDT
Created attachment 70185 [details]
Test case
Comment 2 Nat Duca 2010-10-07 19:12:39 PDT
Created attachment 70187 [details]
Patch for the purposes of discussion only.
Comment 3 Nat Duca 2010-10-07 19:33:34 PDT
Created attachment 70190 [details]
Issue invalidate in the right coordinate system. Oops.
Comment 4 Simon Fraser (smfr) 2010-10-12 14:57:11 PDT
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.
Comment 5 Nat Duca 2010-10-12 15:57:17 PDT
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.
Comment 6 Nat Duca 2010-10-13 14:34:28 PDT
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 7 Simon Fraser (smfr) 2010-10-13 15:55:59 PDT
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.
Comment 8 Nat Duca 2010-10-14 14:27:38 PDT
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 9 Simon Fraser (smfr) 2010-10-14 16:30:54 PDT
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.
Comment 10 Simon Fraser (smfr) 2010-10-14 16:31:38 PDT
<rdar://problem/8554258>
Comment 11 Simon Fraser (smfr) 2010-10-17 19:54:19 PDT
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.
Comment 12 Simon Fraser (smfr) 2010-10-17 21:20:26 PDT
Created attachment 70994 [details]
Patch illustrating the correct approach

This is closer to the correct solution, but see the FIXMEs.
Comment 13 Nat Duca 2010-10-20 09:20:03 PDT
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.
Comment 14 Simon Fraser (smfr) 2010-10-20 09:44:44 PDT
google.com is a bad thing to use in an iframe because it takes over the main frame if you're signed in.
Comment 15 Simon Fraser (smfr) 2010-10-20 09:54:28 PDT
The patch doesn't work because it calls invalidateRect(), which does not invalidate compositing layers.
Comment 16 Nat Duca 2010-10-21 14:28:26 PDT
Created attachment 71494 [details]
Simon's patch, but repaint  only the iframe on the parent renderer.
Comment 17 Nat Duca 2010-10-21 14:33:53 PDT
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 18 Simon Fraser (smfr) 2010-10-24 12:28:42 PDT
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.
Comment 19 Simon Fraser (smfr) 2010-10-25 17:14:52 PDT
Created attachment 71822 [details]
Patch
Comment 20 mitz 2010-10-25 17:20:28 PDT
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?
Comment 21 Simon Fraser (smfr) 2010-10-25 18:07:07 PDT
http://trac.webkit.org/changeset/70509