Bug 38286 – Need to call FrameView::scrollPositionChanged when changing the scroll position when the ScrollView does not have a platformWidget

• Component: New Bugs
• Status: CLOSED FIXED

[James Robinson]

Need to call FrameView::scrollPositionChanged when changing the scroll position when the ScrollView does not have a platformWidget

[James Robinson]

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.

[James Robinson]

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)

[James Robinson]

Created attachment 54628 [details]
Patch

[James Robinson]

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

[Darin Adler]

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.

[James Robinson]

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.

[James Robinson]

Created attachment 54644 [details]
Patch

[James Robinson]

Created attachment 54648 [details]
Patch

[Benjamin Poulain]

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

[Benjamin Poulain]

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

[James Robinson]

Created attachment 54727 [details]
Patch

[James Robinson]

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.

[Benjamin Poulain]

(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 :)

[James Robinson]

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

[WebKit Review Bot]

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

[Eric Seidel (no email)]

Totally lame to leave the tree broken :(

[Benjamin Poulain]

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

[Simon Hausmann]

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

[James Robinson]

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

[Simon Hausmann]

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

[Simon Hausmann]

(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 :)