Bug 39217 – Add an optional "starting node' parameter to scrollRecursively and scrollOverflow of EventHandler

• Status: CLOSED
• Resolution: FIXED
• Component: WebCore Misc.

[Antonio Gomes]

It would be useful if scrollOverflow and scrollRecursively methods of EventHandler had an optional parameter to be the starting point of a scroll action. For example, currently, EventHandler::scrollOverflow starts a scrolling off of either a 'focusedNode' of a m_'mousePressNode' (see below):

bool EventHandler::scrollOverflow(...) {
    Node* node = m_frame->document()->focusedNode();

     if (!node)
        node = m_mousePressNode.get();
(...)
     
but it would be useful, for Spatial Navigation purposes, that we could pass a Node for the scrolling to start from.

scenario (see illustration below):

1) a focused node called "focused", an overflow div (with a clipped overflow focusable node called "offscreen");
2) user presses key down.
3) Focus should move to "offscreen", but since it is clipped overflow, spatial navigation will scroll-in-direction until it gets visible.


--------------------------------

   *********
   *focused*
   *********

 vertically overflowed div
 ********************
 *                * *
 *                * *
 *                * *
 *                * *
 ********************
 .                  .  
 .   *offscreen*    .
 --------------------   



--------------------------------

In order to scroll the overflowed div, it is needed to pass to EventHandler::scrollOverflow a starting Node, so it will scroll what I send it to scroll. Otherwise its assumptions to get the node from where to start scrolling from wont help Spatial Navigation.

patch coming ...

[Antonio Gomes]

Created attachment 56241 [details]
patch v1

[Antonio Gomes]

Created attachment 56242 [details]
patch v2

And the right/newer/working version of the patch.

[Antonio Gomes]

(In reply to comment #2)
> Created an attachment (id=56242) [details]
> patch v2

Darin, could you please help me with this patch?

Call site and layout test is in attachment https://bug-39195-attachments.webkit.org/attachment.cgi?id=56249

[Darin Adler]

Comment on attachment 56242 [details]
patch v2

I don't think the name "fromNode" is clear enough. Argument names should be noun phrases; there's no such thing as a "from node". Maybe "starting node" is the right name?

[Antonio Gomes]

Created attachment 56250 [details]
patch v2.1

(In reply to comment #4)
> (From update of attachment 56242 [details])
> I don't think the name "fromNode" is clear enough. Argument names should be noun phrases; there's no such thing as a "from node". Maybe "starting node" is the right name?

Same as patch v2, but changed "fromNode" to 'startingNode"

[Darin Adler]

Comment on attachment 56250 [details]
patch v2.1

> +    bool scrollRecursively(ScrollDirection, ScrollGranularity, Node* fromNode = 0);

Forgot to change names here.

[Antonio Gomes]

Created attachment 56251 [details]
(committed with r60159, reviewed by darin adler) patch v2.2

Changed missing bits of patch 2.1, as per Darin's request.

[Antonio Gomes]

Comment on attachment 56251 [details]
(committed with r60159, reviewed by darin adler) patch v2.2

Clearing flags on attachment: 56251

Committed r60159: <http://trac.webkit.org/changeset/60159>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60159 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60158
http://trac.webkit.org/changeset/60159

[Simon Hausmann]

Revision r60159 cherry-picked into qtwebkit-2.0 with commit 146d466226932322e6f6242b083372a6ab502e80