Bug 94934 – Wheel-events fails temporarily after reload

• Component: WebKit2
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 160415 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 160415 [details]
Patch

How do we test this?  We can do reloads and synthesized wheel events in DRT....

[Allan Sandfeld Jensen]

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

[Andras Becsi]

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

[Allan Sandfeld Jensen]

Created attachment 160941 [details]
Patch

Now with a QML test. LayoutTests does not support fixedVisibleContentRect

[Simon Hausmann]

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.

[Jocelyn Turcotte]

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?

[Allan Sandfeld Jensen]

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

[Antonio Gomes]

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

[Allan Sandfeld Jensen]

Created attachment 161228 [details]
Patch

Changed to use WebFrame::visibleContentBounds which is always available, cleaned up changelog and test-case

[Jocelyn Turcotte]

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

[Allan Sandfeld Jensen]

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

[WebKit Review Bot]

Comment on attachment 161228 [details]
Patch

Clearing flags on attachment: 161228

Committed r127420: <http://trac.webkit.org/changeset/127420>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.