Bug 39217 - Add an optional "starting node' parameter to scrollRecursively and scrollOverflow of EventHandler
Summary: Add an optional "starting node' parameter to scrollRecursively and scrollOver...
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 18662
Blocks: 39195
  Show dependency treegraph
 
Reported: 2010-05-17 07:06 PDT by Antonio Gomes
Modified: 2011-04-19 05:15 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-05-17 07:06:06 PDT
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 Antonio Gomes 2010-05-17 07:18:31 PDT
Created attachment 56241 [details]
patch v1
Comment 2 Antonio Gomes 2010-05-17 07:22:28 PDT
Created attachment 56242 [details]
patch v2

And the right/newer/working version of the patch.
Comment 3 Antonio Gomes 2010-05-17 10:39:27 PDT
(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
Comment 4 Darin Adler 2010-05-17 10:50:16 PDT
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?
Comment 5 Antonio Gomes 2010-05-17 10:59:58 PDT
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"
Comment 6 Darin Adler 2010-05-17 11:03:30 PDT
Comment on attachment 56250 [details]
patch v2.1

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

Forgot to change names here.
Comment 7 Antonio Gomes 2010-05-17 11:08:19 PDT
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.
Comment 8 Antonio Gomes 2010-05-25 06:21:13 PDT
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>
Comment 9 WebKit Review Bot 2010-05-25 06:51:22 PDT
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 Simon Hausmann 2010-05-27 14:09:52 PDT
Revision r60159 cherry-picked into qtwebkit-2.0 with commit 146d466226932322e6f6242b083372a6ab502e80