Bug 32668 – [Qt] Add new scrollRecursively API to QWebFrame

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Joseph Ligman]

Adding scrollRecursively is useful for any QtWebKit enabled application that supports touch panning across nested iframes.

[Joseph Ligman]

Created attachment 45080 [details]
Patch to add new QWebFrame API scrollRecursively

I took the concept from the EventHandler::scrollRecursively

[WebKit Review Bot]

Attachment 45080 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2822:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2832:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2845:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2855:  Missing space after ,  [whitespace/comma] [3]
WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2858:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 5

[Joseph Ligman]

Created attachment 45081 [details]
 Patch to add new QWebFrame API scrollRecursively

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45081 [details] without any errors.

[Joseph Ligman]

(In reply to comment #2)
> Attachment 45080 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2822:  Missing space after , 
> [whitespace/comma] [3]
> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2832:  Missing space after , 
> [whitespace/comma] [3]
> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2845:  Missing space after , 
> [whitespace/comma] [3]
> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2855:  Missing space after , 
> [whitespace/comma] [3]
> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2858:  Missing space after , 
> [whitespace/comma] [3]
> Total errors found: 5

at least I'm consistent :)

[Kenneth Rohde Christiansen]

Comment on attachment 45081 [details]
 Patch to add new QWebFrame API scrollRecursively 

> Index: WebKit/qt/ChangeLog
> ===================================================================
> --- WebKit/qt/ChangeLog	(revision 52259)
> +++ WebKit/qt/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> [phone]  Joe Ligman  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Add new API to QWebFrame to scrollRecursively starting with any css overflow 
> +        then checking current frame and then ancestors
> +        https://bugs.webkit.org/show_bug.cgi?id=32668
> +
> +        * Api/qwebframe.cpp:
> +        (QWebFramePrivate::scrollOverflow):
> +        (QWebFrame::scrollRecursively):
> +        * Api/qwebframe.h:
> +        * Api/qwebframe_p.h:
> +        * tests/qwebframe/qwebframe.qrc:
> +        * tests/qwebframe/testiframe.html: Added.
> +        * tests/qwebframe/testiframe2.html: Added.
> +        * tests/qwebframe/tst_qwebframe.cpp:
> +
>  2009-12-17  Benjamin Poulain  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.
> Index: WebKit/qt/Api/qwebframe.cpp
> ===================================================================
> --- WebKit/qt/Api/qwebframe.cpp	(revision 52259)
> +++ WebKit/qt/Api/qwebframe.cpp	(working copy)
> @@ -361,6 +361,45 @@ void QWebFramePrivate::renderRelativeCoo
>      }
>  }
>  
> +bool QWebFramePrivate::scrollOverflow(int dx, int dy)
> +{
> +    if (!frame || !frame->document() || !frame->eventHandler())
> +        return false;
> +
> +    Node* node = frame->document()->focusedNode();
> +    if (!node)
> +        node = frame->document()->elementFromPoint(frame->eventHandler()->currentMousePosition().x(),
> +                                                    frame->eventHandler()->currentMousePosition().y());

Remove the space before frame->

> +    if (!node)
> +        return false;
> +
> +    RenderObject* renderer = node->renderer();
> +    if (!renderer)
> +        return false;
> +
> +    if (renderer->isListBox())
> +        return false;
> +
> +    RenderLayer* renderLayer = renderer->enclosingLayer();
> +    if (!renderLayer)
> +        return false;
> +
> +    bool scrolledHorizontal = false;
> +    bool scrolledVertical = false;
> +
> +    if (dx > 0)
> +        scrolledHorizontal = renderLayer->scroll(ScrollRight, ScrollByPixel, dx);
> +    else if (dx < 0)
> +        scrolledHorizontal = renderLayer->scroll(ScrollLeft, ScrollByPixel, qAbs(dx));
> +
> +    if (dy > 0)
> +        scrolledVertical = renderLayer->scroll(ScrollDown, ScrollByPixel, dy);
> +    else if (dy < 0)
> +        scrolledVertical = renderLayer->scroll(ScrollUp, ScrollByPixel, qAbs(dy));
> +
> +    return (scrolledHorizontal || scrolledVertical);
> +}
> +
>  /*!
>      \class QWebFrame
>      \since 4.4
> @@ -1000,6 +1039,50 @@ void QWebFrame::scroll(int dx, int dy)
>  }
>  
>  /*!
> +  \since 4.7
> +  Scrolls nested frames starting at this frame, \a dx pixels to the right 
> +  and \a dy pixels downward. Both \a dx and \a dy may be negative. First attempts
> +  to scroll elements with css overflow followed by this frame. If this 

It is called CSS

> +  frame doesn't scroll, attempts to scroll the parent
> +
> +  \sa QWebFrame::scroll, QWebFramePrivate::scrollOverflow

Dont add references to the private!

> +*/
> +bool QWebFrame::scrollRecursively(int dx, int dy)
> +{
> +    bool scrolledHorizontal = false;
> +    bool scrolledVertical = false;
> +    bool scrolledOverflow = d->scrollOverflow(dx, dy);
> +
> +    if (!scrolledOverflow) {
> +        Frame* frame = d->frame;
> +        if (!frame || !frame->view())
> +            return false;
> +
> +        do {
> +            IntSize scrollOffset = frame->view()->scrollOffset();
> +            IntPoint maxScrollOffset = frame->view()->maximumScrollPosition();
> +
> +            if (dx > 0) // scroll right
> +                scrolledHorizontal = scrollOffset.width() < maxScrollOffset.x();
> +            else if (dx < 0) // scroll left
> +                scrolledHorizontal = scrollOffset.width() > 0;
> +
> +            if (dy > 0) // scroll down
> +                scrolledVertical = scrollOffset.height() < maxScrollOffset.y();
> +            else if (dy < 0) //scroll up
> +                scrolledVertical = scrollOffset.height() > 0;
> +
> +            if (scrolledHorizontal || scrolledVertical) {
> +                frame->view()->scrollBy(IntSize(dx, dy));
> +                return true;
> +            }
> +            frame = frame->tree()->parent(); 
> +        } while (frame && frame->view());
> +    }
> +    return (scrolledHorizontal || scrolledVertical || scrolledOverflow);
> +}
> +
> +/*!
>    \property QWebFrame::scrollPosition
>    \since 4.5
>    \brief the position the frame is currently scrolled to.
> Index: WebKit/qt/Api/qwebframe.h
> ===================================================================
> --- WebKit/qt/Api/qwebframe.h	(revision 52259)
> +++ WebKit/qt/Api/qwebframe.h	(working copy)
> @@ -156,6 +156,7 @@ public:
>      QRect scrollBarGeometry(Qt::Orientation orientation) const;
>  
>      void scroll(int, int);
> +    bool scrollRecursively(int, int);
>      QPoint scrollPosition() const;
>      void setScrollPosition(const QPoint &pos);
>  
> Index: WebKit/qt/Api/qwebframe_p.h
> ===================================================================
> --- WebKit/qt/Api/qwebframe_p.h	(revision 52259)
> +++ WebKit/qt/Api/qwebframe_p.h	(working copy)
> @@ -85,6 +85,8 @@ public:
>      void renderRelativeCoords(WebCore::GraphicsContext*, QWebFrame::RenderLayer, const QRegion& clip);
>      void renderContentsLayerAbsoluteCoords(WebCore::GraphicsContext*, const QRegion& clip);
>  
> +    bool scrollOverflow(int dx, int dy);
> +
>      QWebFrame *q;
>      Qt::ScrollBarPolicy horizontalScrollBarPolicy;
>      Qt::ScrollBarPolicy verticalScrollBarPolicy;
> Index: WebKit/qt/tests/qwebframe/qwebframe.qrc
> ===================================================================
> --- WebKit/qt/tests/qwebframe/qwebframe.qrc	(revision 52259)
> +++ WebKit/qt/tests/qwebframe/qwebframe.qrc	(working copy)
> @@ -4,5 +4,7 @@
>  <file>style.css</file>
>  <file>test1.html</file>
>  <file>test2.html</file>
> +<file>testiframe.html</file>
> +<file>testiframe2.html</file>
>  </qresource>
>  </RCC>
> Index: WebKit/qt/tests/qwebframe/testiframe2.html
> ===================================================================
> --- WebKit/qt/tests/qwebframe/testiframe2.html	(revision 0)
> +++ WebKit/qt/tests/qwebframe/testiframe2.html	(revision 0)
> @@ -0,0 +1,21 @@
> +</html>
> +<html>
> +<head>
> +<title></title>
> +<style type="text/css">
> +<!--
> +#content {
> +  background: #fff;
> +  position: absolute;
> +  top: 0px;
> +  left: 0px;
> +  width: 800px;
> +  height: 800px;
> +}
> +-->
> +</style>
> +</head>
> +<body>
> +<div id="content"> </div>
> +</body>
> +</html>
> \ No newline at end of file
> Index: WebKit/qt/tests/qwebframe/testiframe.html
> ===================================================================
> --- WebKit/qt/tests/qwebframe/testiframe.html	(revision 0)
> +++ WebKit/qt/tests/qwebframe/testiframe.html	(revision 0)
> @@ -0,0 +1,54 @@
> +</html>
> +<html>
> +<head>
> +<title></title>
> +<style type="text/css">
> +<!--
> +#header {
> +  background: #0f0;
> +  position: absolute;
> +  top: 0px;
> +  left: 0px;
> +  width: 800px;
> +  height: 100px;
> +}
> +#content1 {
> +  background: #ff0;
> +  position: absolute;
> +  top: 101px;
> +  left: 0px;
> +  width: 400px;
> +  height: 400px;
> +  overflow: scroll;
> +}
> +#content2 {
> +  background: #ff7;
> +  position: absolute;
> +  top: 101px;
> +  left: 401px;
> +  width: 400px;
> +  height: 400px;
> +}
> +#footer {
> +  background: #0f0;
> +  position: absolute;
> +  top: 502px;
> +  left: 0px;
> +  width: 800px;
> +  height: 200px;
> +}
> +-->
> +</style>
> +</head>
> +<body>
> +<div id="header"></div>
> +<div id="content1">You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.
> +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.
> +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.
> +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.
> +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.
> +You can use the overflow property when you want to have better control of the layout. Try to change the overflow property to: visible, hidden, auto, or inherit and see what happens. The default value is visible.</div>
> +<iframe id="content2" name="control" src="testiframe2.html"> </iframe>
> +<div id="footer"></div>
> +</body>
> +</html>
> \ No newline at end of file
> Index: WebKit/qt/tests/qwebframe/tst_qwebframe.cpp
> ===================================================================
> --- WebKit/qt/tests/qwebframe/tst_qwebframe.cpp	(revision 52259)
> +++ WebKit/qt/tests/qwebframe/tst_qwebframe.cpp	(working copy)
> @@ -576,6 +576,7 @@ private slots:
>      void scrollPosition();
>      void evaluateWillCauseRepaint();
>      void qObjectWrapperWithSameIdentity();
> +    void scrollRecursively();
>  
>  private:
>      QString  evalJS(const QString&s) {
> @@ -2795,5 +2796,69 @@ void tst_QWebFrame::qObjectWrapperWithSa
>      QCOMPARE(mainFrame->toPlainText(), QString("test2"));
>  }
>  
> +void tst_QWebFrame::scrollRecursively()
> +{
> +    // The test content is 
> +    // a nested frame set
> +    // The main frame scrolls
> +    // and has two children
> +    // an iframe and a div overflow
> +    // both scroll
> +    QWebView webView;
> +    QWebPage* webPage = webView.page();
> +    QSignalSpy loadSpy(webPage, SIGNAL(loadFinished(bool)));
> +    QUrl url = QUrl("qrc:///testiframe.html");
> +    webPage->mainFrame()->load(url);
> +    QTRY_COMPARE(loadSpy.count(), 1);
> +
> +    QList<QWebFrame*> children =  webPage->mainFrame()->childFrames();
> +    QVERIFY(children.count() == 1);
> +
> +    // 1st test
> +    // call scrollRecursively over mainframe
> +    // verify scrolled
> +    // verify scroll postion changed
> +    QPoint scrollPosition(webPage->mainFrame()->scrollPosition());
> +    QVERIFY(webPage->mainFrame()->scrollRecursively(10, 10));
> +    QVERIFY(scrollPosition != webPage->mainFrame()->scrollPosition());
> +
> +    // 2nd test
> +    // call scrollRecursively over child iframe
> +    // verify scrolled
> +    // verify child scroll position changed
> +    // verify parent's scroll position did not change
> +    scrollPosition = webPage->mainFrame()->scrollPosition();
> +    QPoint childScrollPosition = children.at(0)->scrollPosition();
> +    QVERIFY(children.at(0)->scrollRecursively(10, 10));
> +    QVERIFY(scrollPosition == webPage->mainFrame()->scrollPosition());
> +    QVERIFY(childScrollPosition != children.at(0)->scrollPosition());
> +
> +    // 3rd test
> +    // call scrollRecursively over div overflow
> +    // verify scrolled == true
> +    // verify parent and child frame's scroll postion did not change
> +    QWebElement div = webPage->mainFrame()->documentElement().findFirst("#content1");
> +    QMouseEvent evpres(QEvent::MouseMove, div.geometry().center(), Qt::NoButton, Qt::NoButton, Qt::NoModifier);
> +    webPage->event(&evpres);
> +    scrollPosition = webPage->mainFrame()->scrollPosition();
> +    childScrollPosition = children.at(0)->scrollPosition();
> +    QVERIFY(webPage->mainFrame()->scrollRecursively(5, 5));
> +    QVERIFY(childScrollPosition == children.at(0)->scrollPosition());
> +    QVERIFY(scrollPosition == webPage->mainFrame()->scrollPosition());
> +
> +    // 4th test
> +    // call scrollRecursively twice over childs iframe
> +    // verify scrolled == true first time
> +    // verify parent's scroll == true second time
> +    // verify parent and childs scroll position changed
> +    childScrollPosition = children.at(0)->scrollPosition();
> +    QVERIFY(children.at(0)->scrollRecursively(-10, -10));
> +    QVERIFY(childScrollPosition != children.at(0)->scrollPosition());
> +    scrollPosition = webPage->mainFrame()->scrollPosition();
> +    QVERIFY(children.at(0)->scrollRecursively(-10, -10));
> +    QVERIFY(scrollPosition != webPage->mainFrame()->scrollPosition());
> +
> +}
> +
>  QTEST_MAIN(tst_QWebFrame)
>  #include "tst_qwebframe.moc"

[Joseph Ligman]

Created attachment 45086 [details]
Patch to add new QWebFrame API scrollRecursively

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45086 [details] without any errors.

[Adam Barth]

> at least I'm consistent :)

Thanks for fixing the style nits.  :)

[Joseph Ligman]

*** Bug 26934 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 45086 [details]
Patch to add new QWebFrame API scrollRecursively

Clearing flags on attachment: 45086

Committed r52311: <http://trac.webkit.org/changeset/52311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

i am late here ( vacations) 

nits: joe, i would had added a break point (".") at the end of 

"+  \since 4.7
+  Scrolls nested frames starting at this frame, \a dx pixels to the right 
+  and \a dy pixels downward. Both \a dx and \a dy may be negative. First attempts 
+  to scroll elements with CSS overflow followed by this frame. If this 
+  frame doesn't scroll, attempts to scroll the parent" 

and documented the what true/false means in terms of returned value.

[Laszlo Gombos]

Joe, would you mind prepare a version of this patch for 4.6 branch with private APIs (qt_scrollRecursively, qt_scrollOverflow) ? Thanks.

[Joseph Ligman]

Created attachment 46140 [details]
private API prepared to merge to 4.6

[Simon Hausmann]

(In reply to comment #15)
> Created an attachment (id=46140) [details]
> private API prepared to merge to 4.6

The auto test included in this patch fails for me:

FAIL!  : tst_QWebFrame::qtscrollRecursively() 'children.count() == 1' returned FALSE. ()

Also I don't think we should use qt_ as prefix for the private API as that might conflict with private API that qt exports. I would refer qtwebkit_webframe_scrollRecursively(). Also the qt_privateScrollOverflow function should be marked as static and it does not need a qt_ prefix.

[Simon Hausmann]

Created attachment 46274 [details]
Patch for private API

[Simon Hausmann]

Added commit 236bb65bb77eac6f668fd9fbf53d85e58b7d291a that cherry-picks 52311 and then commit 39b1a3d826dce489d2f5c99282ef0c30b5dbb9f6 on top that fixes the private API.