Bug 37803

Summary: Spatial Navigation: adapt the logic of {deep}findFocusableNodeInDirection to do traversal starting from Node* not Document*
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: AccessibilityAssignee: Antonio Gomes <tonikitoo>
Status: CLOSED FIXED    
Severity: Normal CC: hausmann, kenneth, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 35784, 36463    
Attachments:
Description Flags
patch v1
tonikitoo: commit-queue-
patch v2
tonikitoo: commit-queue-
(committer in r58882, reviewed by kenneth) patch v3 none

Description Antonio Gomes 2010-04-19 08:43:59 PDT
Instead of having findFocusableNodeInDirection and deepFindFocusableNodeInDirection logic working w/ a Document pointer as incoming parameter from callers, I would like to change them to get a Node pointer. Of course it can node be any node, but "scrollable containers" nodes, e.g. <html> (documentElement()), <iframe>, <frame> or scrollable div's.

It is a preparation for supporting scrollable content in Spatial Navigation. See bug 36463.

patch coming...
Comment 1 Antonio Gomes 2010-04-19 08:47:16 PDT
s/it can node be any node/it can NOT be any node/
Comment 2 Antonio Gomes 2010-04-19 09:00:16 PDT
Created attachment 53682 [details]
patch v1
Comment 3 Antonio Gomes 2010-04-20 10:52:31 PDT
Created attachment 53845 [details]
patch v2

- Better ChangeLog entry
Comment 4 Antonio Gomes 2010-04-22 06:49:33 PDT
Created attachment 54057 [details]
(committer in r58882, reviewed by kenneth) patch v3

Same as patch v2, but fixed a minor bug in an ASSERT code.
Comment 5 Antonio Gomes 2010-04-22 06:50:15 PDT
To help reviewrs, I can summarize patch v3 (attachment 54057 [details]):

* changes in advanceFocusDirectionally and findFocusedNodeInDirection are pretty much straighforward:

- at the former, instead passing Document* to the latter, it passes Document::firstChild() - a node -
- ... and it traverses the tree down from this node.
- Further at findFocusedNodeInDirection method, it changes the use of FOR to WHILE, but logic is exactly the same.

* in deepFindFocusableNodeInDirection, code is essentially the same, but under a IF now:

  // Iframe or Frame.
+ if (container->hasTagName(frameTag) || container->hasTagName(iframeTag)) {
  (...)
Comment 6 Antonio Gomes 2010-04-26 20:08:16 PDT
Ping review?

see comment #5 for an easier reviewing.
Comment 7 Kenneth Rohde Christiansen 2010-05-06 07:20:17 PDT
Comment on attachment 54057 [details]
(committer in r58882, reviewed by kenneth) patch v3


> 
WebCore/page/FocusController.cpp:393
 +  void FocusController::findFocusableNodeInDirection(Node* outter, Node* focusedNode,
outer

WebCore/page/FocusController.cpp:398
 +      ASSERT(outter);
outer

WebCore/page/FocusController.cpp:404
 +      Node* candidate = outter;
same thing

WebCore/page/FocusController.cpp:443
 +      // Track if focusedNode is a descendant or not of the current container node being processed.
no need to add the "or not"
Comment 8 Antonio Gomes 2010-05-06 09:19:05 PDT
Comment on attachment 54057 [details]
(committer in r58882, reviewed by kenneth) patch v3

Clearing flags on attachment: 54057

Committed r58882: <http://trac.webkit.org/changeset/58882>
Comment 9 Simon Hausmann 2010-05-07 00:39:59 PDT
Revision r58882 cherry-picked into qtwebkit-2.0 with commit 712e5d5a8c5ff7d974e19bce03c56bf93ad82314