Bug 105187 - [WK2] ManualTests/window-geometry.html contents is shaking while scrolling
Summary: [WK2] ManualTests/window-geometry.html contents is shaking while scrolling
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 105978 108820
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-17 09:09 PST by Mikhail Pozdnyakov
Modified: 2014-02-05 11:15 PST (History)
15 users (show)

See Also:


Attachments
preliminary patch. (4.70 KB, patch)
2012-12-19 08:46 PST, Mikhail Pozdnyakov
benjamin: review-
Details | Formatted Diff | Diff
patch (2.72 KB, patch)
2013-02-01 08:45 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v3 (2.62 KB, patch)
2013-02-04 07:24 PST, Mikhail Pozdnyakov
eflews.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-12-17 09:09:47 PST
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
Comment 1 Kenneth Rohde Christiansen 2012-12-17 09:11:55 PST
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.
Comment 2 Kenneth Rohde Christiansen 2012-12-17 09:20:02 PST
Adding some Qt guys. Do you have similar issues on Qt side?
Comment 3 Andras Becsi 2012-12-17 09:51:45 PST
(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".
Comment 4 Andras Becsi 2012-12-17 09:58:17 PST
(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.
Comment 5 Kenneth Rohde Christiansen 2012-12-17 10:33:48 PST
We are using delegated scrolling. Very much same arch as Qt.
Comment 6 Mikhail Pozdnyakov 2012-12-18 05:40:49 PST
        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.
Comment 7 Mikhail Pozdnyakov 2012-12-19 08:46:04 PST
Created attachment 180174 [details]
preliminary patch.
Comment 8 Kenneth Rohde Christiansen 2012-12-19 16:30:54 PST
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
Comment 9 Benjamin Poulain 2013-01-12 00:03:47 PST
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.
Comment 10 Benjamin Poulain 2013-01-12 00:04:41 PST
The EFL tag is misleading here, removing it.
Comment 11 Mikhail Pozdnyakov 2013-01-14 01:18:22 PST
(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!
Comment 12 Mikhail Pozdnyakov 2013-02-01 08:45:39 PST
Created attachment 186057 [details]
patch

Updated since the dependent bug is closed an pageScaleFactor can be used.
Comment 13 Noam Rosenthal 2013-02-01 08:58:04 PST
This looks fine to me.
Comment 14 Kenneth Rohde Christiansen 2013-02-01 09:28:57 PST
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.
Comment 15 Mikhail Pozdnyakov 2013-02-04 06:33:46 PST
(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.
Comment 16 Mikhail Pozdnyakov 2013-02-04 07:24:30 PST
Created attachment 186371 [details]
patch v3

Updated after bug#108820 is fixed and due to Kenneth comments.
Comment 17 EFL EWS Bot 2013-04-23 03:26:38 PDT
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
Comment 18 Andreas Kling 2014-02-05 11:15:40 PST
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.