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

Antonio Gomes
Reported 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...
Attachments
patch v1 (9.79 KB, patch)
2010-04-19 09:00 PDT, Antonio Gomes 		tonikitoo: commit-queue-
DetailsFormatted DiffDiff
patch v2 (11.80 KB, patch)
2010-04-20 10:52 PDT, Antonio Gomes 		tonikitoo: commit-queue-
DetailsFormatted DiffDiff
(committer in r58882, reviewed by kenneth) patch v3 (11.48 KB, patch)
2010-04-22 06:49 PDT, Antonio Gomes 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Antonio Gomes
Comment 1 2010-04-19 08:47:16 PDT
s/it can node be any node/it can NOT be any node/
Antonio Gomes
Comment 2 2010-04-19 09:00:16 PDT
Created attachment 53682 [details] patch v1
Antonio Gomes
Comment 3 2010-04-20 10:52:31 PDT
Created attachment 53845 [details] patch v2 - Better ChangeLog entry
Antonio Gomes
Comment 4 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.
Antonio Gomes
Comment 5 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)) { (...)
Antonio Gomes
Comment 6 2010-04-26 20:08:16 PDT
Ping review? see comment #5 for an easier reviewing.
Kenneth Rohde Christiansen
Comment 7 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"
Antonio Gomes
Comment 8 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>
Simon Hausmann
Comment 9 2010-05-07 00:39:59 PDT
Revision r58882 cherry-picked into qtwebkit-2.0 with commit 712e5d5a8c5ff7d974e19bce03c56bf93ad82314
Note You need to log in before you can comment on or make changes to this bug.