Bug 105187 – [WK2] ManualTests/window-geometry.html contents is shaking while scrolling

• Component: WebKit EFL
• Status: NEW

[Mikhail Pozdnyakov]

1) Launch ManualTests/window-geometry.html test with EFL WK2 mini browser
2) Scroll main window -> both main window contents and little windows are shaking

[Kenneth Rohde Christiansen]

It seems to not be scrolling correctly at all to me.

Ie. the scrollBy scrolls but asks to scroll again before it gets its uptodate position. That could cause the shaking... moving to new position, reading the old position and moving back.

[Kenneth Rohde Christiansen]

Adding some Qt guys. Do you have similar issues on Qt side?

[Andras Becsi]

(In reply to comment #2)
> Adding some Qt guys. Do you have similar issues on Qt side?

AFAIK opening additional windows / dialogs from JS would need client-side QML on Qt, which is not implemented right now in MiniBrowser.

But the main window seems to scroll correctly in MiniBrowser, without any "shaking".

[Andras Becsi]

(In reply to comment #1)
> It seems to not be scrolling correctly at all to me.
> 
> Ie. the scrollBy scrolls but asks to scroll again before it gets its uptodate position. That could cause the shaking... moving to new position, reading the old position and moving back.

I'm not familiar with EFL, but this almost sounds like event delivery issues (eg. both the UI and WebCore try to apply the scroll delta and this conflicts).
Does EFL use delegated scrolling or does WebCore handle scroll events?
Or an incorrectly updated visible rect could also be a source of the issue.

[Kenneth Rohde Christiansen]

We are using delegated scrolling. Very much same arch as Qt.

[Mikhail Pozdnyakov]

        var before = document.body.scrollTop;
        window.scrollBy(0, 10 * sign);
        if (before == document.body.scrollTop) {
            sign = sign * -1;
        }

the problem is that document.body.scrollTop is not updated immediately after window.scrollBy(0, 10 * sign); so it changes sign every time.

[Mikhail Pozdnyakov]

Created attachment 180174 [details]
preliminary patch.

[Kenneth Rohde Christiansen]

Comment on attachment 180174 [details]
preliminary patch.

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1428
>  void WebPage::pageDidRequestScroll(const IntPoint& point)
>  {
>      send(Messages::WebPageProxy::PageDidRequestScroll(point));

I think we need to turn this into a non-request. A request can be ignored, and that is not what we are doing here.

It can only be called from javascript (which is supposed to be suspended when we are interaction with the page) or by the session history, which already checks whether someone interacted (but that doesn't combine 100% with PageViewportHandler::hadUserInteraction - but there is a bug about that)

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1438
> +    // it might not be possible to scroll the last pixel and that affects fixed position elements.
> +    FloatRect bounds;
> +    bounds.setWidth(std::max(float(0), view->contentsSize().width() - floorf(m_viewportSize.width() / m_scale)));
> +    bounds.setHeight(std::max(float(0), view->contentsSize().height() - floorf(m_viewportSize.height() / m_scale)));

Why can't you use the current one instead of introducing m_scale? We really need to move to use pageScaleFactor

[Benjamin Poulain]

Comment on attachment 180174 [details]
preliminary patch.

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

This is an interesting idea, but it needs good test coverage.

This raises interesting questions like how to drive this properly when you have JavaScript and User initiated scrolling going on simultaneously.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:249
> +#if USE(TILED_BACKING_STORE)
> +    , m_scale(1)
> +#endif

WebPage already has a concept of scaleFactor. Why introduce a new scale here?
The name is also too generic given the existing support for scaling.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:888
>  #if USE(TILED_BACKING_STORE)
>      WebCore::IntSize m_viewportSize;
> +    float m_scale;
>  #endif

This has nothing to do with TILED_BACKING_STORE.

[Benjamin Poulain]

The EFL tag is misleading here, removing it.

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 180174 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180174&action=review
> 
> This is an interesting idea, but it needs good test coverage.
> 
> This raises interesting questions like how to drive this properly when you have JavaScript and User initiated scrolling going on simultaneously.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:249
> > +#if USE(TILED_BACKING_STORE)
> > +    , m_scale(1)
> > +#endif
> 
> WebPage already has a concept of scaleFactor. Why introduce a new scale here?
> The name is also too generic given the existing support for scaling.
> 
Fully agree, but the problem is that we currently do not consider scale factors from WebCore. That is why this bug depends on bug#105978.

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:888
> >  #if USE(TILED_BACKING_STORE)
> >      WebCore::IntSize m_viewportSize;
> > +    float m_scale;
> >  #endif
> 
> This has nothing to do with TILED_BACKING_STORE.
Indeed :) it is rather delegated scrolling code, however all the related code is inside TILED_BACKING_STORE guards for some reason.

Thank you for review!

[Mikhail Pozdnyakov]

Created attachment 186057 [details]
patch

Updated since the dependent bug is closed an pageScaleFactor can be used.

[Noam Rosenthal]

This looks fine to me.

[Kenneth Rohde Christiansen]

Comment on attachment 186057 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1454
> +    // We need to update 'FixedVisibleContentRect' right away, so that
> +    // JS window properties have updated values even before actual delegated scrolling happens.
> +    FrameView* view = m_page->mainFrame()->view();

Need to update the visible content rect immediately so that DOM Window properties have
correct values before the actual delegates scroll happens.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1465
> +    // Unfortunately it doesn't seem to be enough, so just always allow one pixel more.
> +    position.setX(clampTo(point.x(), bounds.x(), bounds.width()) + 1);
> +    position.setY(clampTo(point.y(), bounds.y(), bounds.height()) + 1);

We should try to fix this instead of adding workarounds, or at least understand what is going on.

[Mikhail Pozdnyakov]

(In reply to comment #14)
> (From update of attachment 186057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186057&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1454
> > +    // We need to update 'FixedVisibleContentRect' right away, so that
> > +    // JS window properties have updated values even before actual delegated scrolling happens.
> > +    FrameView* view = m_page->mainFrame()->view();
> 
> Need to update the visible content rect immediately so that DOM Window properties have
> correct values before the actual delegates scroll happens.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1465
> > +    // Unfortunately it doesn't seem to be enough, so just always allow one pixel more.
> > +    position.setX(clampTo(point.x(), bounds.x(), bounds.width()) + 1);
> > +    position.setY(clampTo(point.y(), bounds.y(), bounds.height()) + 1);
> 
> We should try to fix this instead of adding workarounds, or at least understand what is going on.

Ok, it should be fixed in viewport controller first, so put this bug as a dependency for the dedicated one.

[Mikhail Pozdnyakov]

Created attachment 186371 [details]
patch v3

Updated after bug#108820 is fixed and due to Kenneth comments.

[EFL EWS Bot]

Comment on attachment 186371 [details]
patch v3

Attachment 186371 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/157741

[Andreas Kling]

Comment on attachment 186371 [details]
patch v3

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.