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.

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

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

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

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

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.

Created attachment 180174 [details] preliminary patch.

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

The EFL tag is misleading here, removing it.

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

Created attachment 186057 [details] patch Updated since the dependent bug is closed an pageScaleFactor can be used.

This looks fine to me.

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.

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

Created attachment 186371 [details] patch v3 Updated after bug#108820 is fixed and due to Kenneth comments.

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