Bug 38286

The (very helpful) tests in http://trac.webkit.org/changeset/57971 exposed a repaint issue in ports that do not give ScrollViews a platformWidget (Safari Win, Chromium, etc). See http://trac.webkit.org/export/58366/trunk/LayoutTests/fast/repaint/fixed-child-move-after-scroll.html. The problem is that if there is not a platformWidget, FrameView::scrollPositionChanged() is not called every time the scroll changes and so repaint invalidations are not generated for all fixed position elements that might need them.

Here's what the callstack looks like from the Mac port, which ends up calling FrameView::scrollPositionChanged through the platform layers: #0 WebCore::FrameView::scrollPositionChanged (this=0x8184800) #1 0x0034c02f in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] #2 0x9153042a in _nsnote_callback () #3 0x905bd47a in __CFXNotificationPost () #4 0x905bd753 in _CFXNotificationPostNotification () #5 0x9152d680 in -[NSNotificationCenter postNotificationName:object:userInfo:] () #6 0x91536ed8 in -[NSNotificationCenter postNotificationName:object:] () #7 0x9660f024 in -[NSView _postBoundsChangeNotification] () #8 0x967451a0 in -[NSView translateOriginToPoint:] () #9 0x96744435 in -[NSClipView _immediateScrollToPoint:] () #10 0x96743efe in -[NSClipView scrollToPoint:] () #11 0x967b10af in -[NSScrollView scrollClipView:toPoint:] () #12 0x966de4de in -[NSClipView _scrollTo:animate:] () #13 0x966de091 in -[NSClipView _scrollTo:] () #14 0x968701de in -[NSClipView _scrollPoint:fromView:] () #15 0x96870154 in -[NSView scrollPoint:] () #16 0x048bff9a in WebCore::ScrollView::platformSetScrollPosition #17 0x048bce4e in WebCore::ScrollView::setScrollPosition #18 0x041c59a9 in WebCore::ScrollView::scrollBy #19 0x041be8aa in WebCore::DOMWindow::scrollBy #20 0x04449e3c in WebCore::jsDOMWindowPrototypeFunctionScrollBy (javascript)

Created attachment 54628 [details] Patch

I've verified that this fixes all of the pixel tests in http://trac.webkit.org/changeset/57971 on the Chromium linux port (which doesn't use a platformWidget for the ScrollView).

Comment on attachment 54628 [details] Patch r=me > + virtual void scrollPositionChanged() {} I'd prefer that this be private. There's no need for code outside the class to ever call this and a derived class can override a function even if it's private.

Comment on attachment 54628 [details] Patch This patch is wrong, it doesn't handle scrolls driven by the keyboard (down arrow or page down) since they go through ScrollView::scroll() and not setScrollPosition(). I'll upload a new patch with a test for this momentarily and also address Darin's feedback.

Created attachment 54644 [details] Patch

Created attachment 54648 [details] Patch

@James Awesome, I was going to work on this today. :) Could you please wait a few hours before landing it? I need to check if it is always called before ScrollView::scrollContents() so I can use it to implement 33150.

Comment on attachment 54648 [details] Patch > scrollContents(scrollDelta); > + scrollPositionChanged(); For my follow up patch, it would be better if those calls could be the other way around: + scrollPositionChanged(); scrollContents(scrollDelta); So all the positions are valid in the code of scrollContents() and I can use the repaint rects to implement 33150. > void ScrollView::scrollRectIntoViewRecursively(const IntRect& r) > @@ -338,6 +339,7 @@ void ScrollView::setScrollPosition(const IntPoint& scrollPoint) > return; > > updateScrollbars(IntSize(newScrollPosition.x(), newScrollPosition.y())); > + scrollPositionChanged(); > } I don't think you need this call if you already call scrollPositionChanged() in ScrollView::valueChanged(). UpdateScrollbar() will end up in ScrollView::valueChanged(), so scrollPositionChanged() will be called twice. Thanks again for making this patch, I go to bed with a problem, I woke up with your patch :)

Created attachment 54727 [details] Patch

Thanks for the feedback, Benjamin. I've moved the scrollPositionChanged() call as you suggested. I plan to grab new expectations for fast/repaint/fixed-move-after-keyboard-scroll.html off of the bots after landing. Simon Fraser suggested that it might be better to move the scrollPositionChanged() call from being public on FrameView to being public on ScrollView. The current only callsite to this is the mac platform layer here: http://trac.webkit.org/browser/trunk/WebKit/mac/WebView/WebHTMLView.mm#L1162. I'm happy to move the function if it makes sense, but I'm less familiar with how this code is supposed to interact with the port layer.

(In reply to comment #12) > Simon Fraser suggested that it might be better to move the > scrollPositionChanged() call from being public on FrameView to being public on > ScrollView. The current only callsite to this is the mac platform layer here: > http://trac.webkit.org/browser/trunk/WebKit/mac/WebView/WebHTMLView.mm#L1162. > I'm happy to move the function if it makes sense, but I'm less familiar with > how this code is supposed to interact with the port layer. I am not sure to understand what you mean. You cannot implement scrollPositionChanged() in ScrollView because you need to access the frame. If you mean have scrollPositionChanged() public in ScrollView instead of Private, the I don't have anything against it :)

Committed r58615: <http://trac.webkit.org/changeset/58615>

http://trac.webkit.org/changeset/58615 might have broken SnowLeopard Intel Release (Tests)

Totally lame to leave the tree broken :(

(In reply to comment #16) > Totally lame to leave the tree broken :( I am surprised this patch breaks Mac, I think this path is never taken on this platform (the scrollbar of frame being native). I can have a look tomorrow morning.

Revision r58615 cherry-picked into qtwebkit-2.0 with commit 57d10d5c05e59bbf7de8189ff47dd18d1be996dc

Committed r59251: <http://trac.webkit.org/changeset/59251>

Revision r59251 cherry-picked into qtwebkit-2.0 with commit 3673bcff281e46da4b1d6829b8ff3c10939c6890

(In reply to comment #20) > Revision r59251 cherry-picked into qtwebkit-2.0 with commit 3673bcff281e46da4b1d6829b8ff3c10939c6890 Oops, I reverted that. Doesn't look like an intentional change :)