[webkit-reviews] review denied: [Bug 121626] Spatial Navigation : User should be able to navigate html elemets having element.style.cursor="pointer" : [Attachment 213246] Updated patch-1

Fri Oct 4 06:01:44 PDT 2013

Antonio Gomes <tonikitoo at webkit.org> has denied Abhijeet Kandalkar
<kandalkar.abhijeet58 at gmail.com>'s request for review:
Bug 121626: Spatial Navigation : User should be able to navigate html elemets
having element.style.cursor="pointer"
https://bugs.webkit.org/show_bug.cgi?id=121626

Attachment 213246: Updated patch-1
https://bugs.webkit.org/attachment.cgi?id=213246&action=review

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

r-. See comments.

Also, a question: this property (cursor: point) seems inheritable. What happens
if one does <body style="cursor:point"> ?

> Source/WebCore/dom/Element.cpp:251
> +bool Element::isSpatialNavigationFocusable() const
> +{
> +    if (!document().frame() &&
!document().frame()->settings().spatialNavigationEnabled())
> +	   return false;
> +
> +    if (!inDocument())
> +	   return false;
> +
> +    if (!renderer() || renderer()->style()->visibility() != VISIBLE)
> +	   return false;
> +
> +    return (renderer()->style()->cursor() == CURSOR_POINTER);
> +}
> +

this does not belong to Element. See more below.

> Source/WebCore/page/FocusController.cpp:768
> +	   if (!element->isSpatialNavigationFocusable() &&
!element->isKeyboardFocusable(event) && !element->isFrameOwnerElement() &&
!canScrollInDirection(element, direction))

isSpatialNavigationFocusable belong, I believe to SpatialNavigation.h, and
should likely be a static method, taking an Element*

Additionally, it should be named with cursor:point case in mind.
isSpatialNavigationFocusable is too generic in this case. Maybe
hasCursorPointStyle?