Bug 94934

Summary: Wheel-events fails temporarily after reload
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: WebKit2Assignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cmarcelo, hausmann, jturcotte, kenneth, menard, tonikitoo, webkit.review.bot, zoltan
Priority: P2    
Version: 420+   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Allan Sandfeld Jensen 2012-08-24 06:50:50 PDT
After reloading some pages, wheel events will temporarily fail to work until the page has been scrolled by some other means.

The problem is restricted to ports using resizesToContents, possibly only Qt.
Comment 1 Allan Sandfeld Jensen 2012-08-24 06:55:59 PDT
Created attachment 160415 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-08-27 13:24:40 PDT
Comment on attachment 160415 [details]
Patch

How do we test this?  We can do reloads and synthesized wheel events in DRT....
Comment 3 Allan Sandfeld Jensen 2012-08-27 15:12:25 PDT
(In reply to comment #2)
> (From update of attachment 160415 [details])
> How do we test this?  We can do reloads and synthesized wheel events in DRT....

If we can set the fixed visible rect and resizes to content, then it should be possible to test it simple by reloading a page that is larger than the visible rect, and checking the size of the fixed visible rect after the reload.

I will take a look to see if it is possible.
Comment 4 Andras Becsi 2012-08-28 02:33:18 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 160415 [details] [details])
> > How do we test this?  We can do reloads and synthesized wheel events in DRT....
> 
> If we can set the fixed visible rect and resizes to content, then it should be possible to test it simple by reloading a page that is larger than the visible rect, and checking the size of the fixed visible rect after the reload.
> 
> I will take a look to see if it is possible.

It should at least be possible to test this with a QML test, see tst_wheelEventHandling.qml.
Comment 5 Allan Sandfeld Jensen 2012-08-28 03:02:43 PDT
Created attachment 160941 [details]
Patch

Now with a QML test. LayoutTests does not support fixedVisibleContentRect
Comment 6 Simon Hausmann 2012-08-29 02:52:21 PDT
Comment on attachment 160941 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        Set fixed visible content rect using, WebPage::viewportSize instead of WebPage::size when available.

There's a stray comma in the sentence :)

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_wheelEventHandling.qml:49
> +            var position = webView.contentY

This seems to be an unused variable.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1243
> +    if (shouldUseFixedLayout) {
> +#if USE(TILED_BACKING_STORE)
> +        m_frame->coreFrame()->view()->setFixedVisibleContentRect(WebCore::IntRect(WebCore::IntPoint(), webPage->viewportSize()));
> +#else
>          m_frame->coreFrame()->view()->setFixedVisibleContentRect(webPage->bounds());
> +#endif

I admit that I don't know this code well enough to feel truly comfortable reviewing this. I would prefer if Kenneth could do that.
Comment 7 Jocelyn Turcotte 2012-08-29 06:44:23 PDT
Comment on attachment 160941 [details]
Patch

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

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1243
>> +    if (shouldUseFixedLayout) {
>> +#if USE(TILED_BACKING_STORE)
>> +        m_frame->coreFrame()->view()->setFixedVisibleContentRect(WebCore::IntRect(WebCore::IntPoint(), webPage->viewportSize()));
>> +#else
>>          m_frame->coreFrame()->view()->setFixedVisibleContentRect(webPage->bounds());
>> +#endif
> 
> I admit that I don't know this code well enough to feel truly comfortable reviewing this. I would prefer if Kenneth could do that.

I don't think shouldUseFixedLayout will ever be true if USE(TILED_BACKING_STORE) is false.

Also I think that logically the visible rect that should be applied is the one last sent by the UI process, anything else is artificial. Would it be possible to do this in Frame::createView and transfer the fixedVisibleContentRect form the previously attached FrameView when creating the new one? Or do it from the outside here rather than guessing it from the page which might still be incomplete at this point?
Comment 8 Allan Sandfeld Jensen 2012-08-29 07:04:57 PDT
(In reply to comment #7)
> I don't think shouldUseFixedLayout will ever be true if USE(TILED_BACKING_STORE) is false.
> 
Maybe not, but API-wise we need to atleast handle it somehow, since shouldUseFixedLayout can be set without TILED_BACKING_STORE being active.

> Also I think that logically the visible rect that should be applied is the one last sent by the UI process, anything else is artificial. Would it be possible to do this in Frame::createView and transfer the fixedVisibleContentRect form the previously attached FrameView when creating the new one? Or do it from the outside here rather than guessing it from the page which might still be incomplete at this point?

The page here is both the old page and the new page, and reading viewPortSize reads the last size that used to set the fixedVisibleContentRect size. At least as far as I know, the fixed visible content rect is always set as a consequence of WebPage::setViewportSize().

The only thing missing from the fixedVisibleRect the position, but the position is restored separately someplace else.
Comment 9 Antonio Gomes 2012-08-29 07:38:10 PDT
> 
> I don't think shouldUseFixedLayout will ever be true if USE(TILED_BACKING_STORE) is false.

That is the case for BlackBerry: fixed layout: true,backing store: false (we use our own).
Comment 10 Allan Sandfeld Jensen 2012-08-29 08:09:50 PDT
Created attachment 161228 [details]
Patch

Changed to use WebFrame::visibleContentBounds which is always available, cleaned up changelog and test-case
Comment 11 Jocelyn Turcotte 2012-08-29 08:22:19 PDT
(In reply to comment #9)
> > 
> > I don't think shouldUseFixedLayout will ever be true if USE(TILED_BACKING_STORE) is false.
> 
> That is the case for BlackBerry: fixed layout: true,backing store: false (we use our own).

Ah, good to know!
Comment 12 Allan Sandfeld Jensen 2012-09-03 02:55:11 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > > 
> > > I don't think shouldUseFixedLayout will ever be true if USE(TILED_BACKING_STORE) is false.
> > 
> > That is the case for BlackBerry: fixed layout: true,backing store: false (we use our own).
> 
> Ah, good to know!

Of course BlackBerry does not use WebKit2 (yet), so you are both right ;)
Comment 13 WebKit Review Bot 2012-09-03 07:14:41 PDT
Comment on attachment 161228 [details]
Patch

Clearing flags on attachment: 161228

Committed r127420: <http://trac.webkit.org/changeset/127420>
Comment 14 WebKit Review Bot 2012-09-03 07:14:45 PDT
All reviewed patches have been landed.  Closing bug.