[Webkit-unassigned] [Bug 117265] Spatial Navigation should avoid unwanted calculation while deciding focus candidate.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117265


Antonio Gomes <tonikitoo at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #206329|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #10 from Antonio Gomes <tonikitoo at webkit.org>  2013-07-28 21:22:31 PST ---
(From update of attachment 206329)
View in context: https://bugs.webkit.org/attachment.cgi?id=206329&action=review

Almost there... r- for now.

> LayoutTests/fast/spatial-navigation/snav-search-optimization.html:3
> +    This test ensures the optimization done in seaching logic to find best candidate focusable node with minimum iterations.

seaRching

> LayoutTests/fast/spatial-navigation/snav-search-optimization.html:52
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "9"); // Focused node is 2 and navigation direction is DOWN. Only 1,3,4,5,6,7,8,9,iframe are considered as valid candidate.
> +        eventSender.keyDown("downArrow");
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "6"); // Focused node is 5 and navigation direction is DOWN. Only 4,6,7,8,9,iframe are considered as valid candidate.
> +        eventSender.keyDown("downArrow");
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "2"); // Focused node is 8 and navigation direction is DOWN. Only 7,9 are considered as valid candidate.
> +        eventSender.keyDown("rightArrow");
> +        eventSender.keyDown("upArrow");
> +        eventSender.keyDown("leftArrow");
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "9"); // Focused node is 6 and navigation direction is LEFT. Only 1,2,3,4,5,7,8,9,iframe are considered as valid candidate.
> +        eventSender.keyDown("leftArrow");
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "6"); // Focused node is 5 and navigation direction is LEFT. Only 1,2,4,7,8,iframe are considered as valid candidate.
> +        eventSender.keyDown("leftArrow");
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "2"); // Focused node is 4 and navigation direction is LEFT. Only 1,7 are considered as valid candidate.
> +        eventSender.keyDown("upArrow");
> +        shouldBe("internals.lastSpatialNavigationCandidateCount()", "5"); // Focused node is 4 and navigation direction is UP. Only 1,2,3,iframe,6 are considered as valid candidate.

I am not sure if these comments are valid. I would omit them.

Additionally, why should the iframe itself be considered a focus candidate?

> Source/WebCore/page/FocusController.cpp:794
> +    if (focusedFrame() && focusedFrame()->document()) {
> +        candidateCount += focusedFrame()->document()->page()->lastSpatialNavigationCandidateCount();
> +        focusedFrame()->document()->page()->setLastSpatialNavigationCandidateCount(candidateCount);
> +    }

could you explain this block? maybe a comment in the code..

> Source/WebCore/page/Page.h:547
> +    unsigned m_lastSpatialNavigationCandidatesCount;

please add a comment like

// NOTE: Only called from Internals for testing.

> Source/WebCore/page/SpatialNavigation.cpp:628
> +        if (candidateRect.x() < currentRect.maxX())

return candidateRect.x() < currentRect.maxX();

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.