Bug 39217 - Add an optional "starting node' parameter to scrollRecursively and scrollOverflow of EventHandler
: Add an optional "starting node' parameter to scrollRecursively and scrollOver...
Status: CLOSED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: PC All
: P2 Enhancement
Assigned To:
:
:
: 18662
: 39195
  Show dependency treegraph
 
Reported: 2010-05-17 07:06 PST by
Modified: 2011-04-19 05:15 PST (History)


Attachments
patch v1 (5.18 KB, patch)
2010-05-17 07:18 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff
patch v2 (5.17 KB, patch)
2010-05-17 07:22 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff
patch v2.1 (5.05 KB, patch)
2010-05-17 10:59 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff
(committed with r60159, reviewed by darin adler) patch v2.2 (5.06 KB, patch)
2010-05-17 11:08 PST, Antonio Gomes
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-05-17 07:06:06 PST
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 ...
------- Comment #1 From 2010-05-17 07:18:31 PST -------
Created an attachment (id=56241) [details]
patch v1
------- Comment #2 From 2010-05-17 07:22:28 PST -------
Created an attachment (id=56242) [details]
patch v2

And the right/newer/working version of the patch.
------- Comment #3 From 2010-05-17 10:39:27 PST -------
(In reply to comment #2)
> Created an attachment (id=56242) [details] [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
------- Comment #4 From 2010-05-17 10:50:16 PST -------
(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?
------- Comment #5 From 2010-05-17 10:59:58 PST -------
Created an attachment (id=56250) [details]
patch v2.1

(In reply to comment #4)
> (From update of attachment 56242 [details] [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"
------- Comment #6 From 2010-05-17 11:03:30 PST -------
(From update of attachment 56250 [details])
> +    bool scrollRecursively(ScrollDirection, ScrollGranularity, Node* fromNode = 0);

Forgot to change names here.
------- Comment #7 From 2010-05-17 11:08:19 PST -------
Created an attachment (id=56251) [details]
patch v2.2

Changed missing bits of patch 2.1, as per Darin's request.
------- Comment #8 From 2010-05-25 06:21:13 PST -------
(From update of attachment 56251 [details])
Clearing flags on attachment: 56251

Committed r60159: <http://trac.webkit.org/changeset/60159>
------- Comment #9 From 2010-05-25 06:51:22 PST -------
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
------- Comment #10 From 2010-05-27 14:09:52 PST -------
Revision r60159 cherry-picked into qtwebkit-2.0 with commit 146d466226932322e6f6242b083372a6ab502e80