[webkit-reviews] review granted: [Bug 49382] Spatial Navigation: issues with the node selection algorithm. : [Attachment 74526] Patch - code only

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has granted Yael <yael.aharon at nokia.com>'s
request for review:
Bug 49382: Spatial Navigation: issues with the node selection algorithm.
https://bugs.webkit.org/show_bug.cgi?id=49382

Attachment 74526: Patch - code only
https://bugs.webkit.org/attachment.cgi?id=74526&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74526&action=review

> WebCore/ChangeLog:18
> +	   content of such a container. The only exception is if the container
has style overflow:hidden. 
> +	   We will not scroll in that case.

It would be nice to have a test case for this case. I do not remember if you
added it in the other bug.

> WebCore/ChangeLog:21
> +	   With this patch, we handle z-index and positioning so that if there
are 2 overlapping focusable nodes,
> +	   we do a hit test and only the node on top can get focus.

Same here.

> WebCore/page/FocusController.cpp:679
> +bool FocusController::navigateInContainer(Node* container, const IntRect&
startingRect, FocusDirection direction, KeyboardEvent* event)

advanceFocusDirectionallyInContainer?

> WebCore/page/FocusController.cpp:687
> +	   newStartingRect = virtualStartingRectForDirection(direction,
nodeRectInAbsoluteCoordinates(container));

Lets name it virtualRectForDirection.

> WebCore/page/SpatialNavigation.cpp:543
> +    if (!container->renderBox())

being a renderBox does not necessarily mean it is scrollable. Maybe

toRenderBox(container->renderer()) &&
toRenderBox(container->renderer())->canBeScrolledAndHasScrollableArea()

?