[Webkit-unassigned] [Bug 49382] Spatial Navigation: issues with the node selection algorithm.

[bugzilla-daemon at webkit.org]

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


Antonio Gomes <tonikitoo at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #73954|review?                     |review-
               Flag|                            |




--- Comment #20 from Antonio Gomes <tonikitoo at webkit.org>  2010-11-15 22:18:45 PST ---
(From update of attachment 73954)
View in context: https://bugs.webkit.org/attachment.cgi?id=73954&action=review

Great! ... but we are still missing some stuff. One more batch needed.

> WebCore/page/FocusController.cpp:783
> +            rect = nodeRectInAbsoluteCoordinates(focusedNode, true);

Add a comment here explaining the bool.

rect = nodeRectInAbsoluteCoordinates(focusedNode, true /* ignore border */);

> WebCore/page/FocusController.cpp:817
> +bool FocusController::navigateIn4WayDirection(FocusDirection direction, KeyboardEvent* event)

I would really like a better name here. Why not keep the previous name?

> WebCore/page/FocusController.cpp:833
> +        container = scrollableOrFrameParentForNodeAndDirection(direction, focusedNode);
> +        startingRect = nodeRectInAbsoluteCoordinates(focusedNode, true);

On IRC we talked about renamed FocusableCandidate to FocusableContent. It would have an additional IntRect, and Node could be 0. The 'container' above would be the FocusableContent::scrollableEnclosingNode. Does it make sense to implement, so it all gets consistent?

ps: Add a comment explaining the bool here too.

> WebCore/page/FocusController.cpp:836
> +    bool consumed = false;

what is consumed?

> WebCore/page/FocusController.cpp:841
> +        startingRect = nodeRectInAbsoluteCoordinates(container, true);

Comment for the bool.

> WebCore/page/FocusController.cpp:844
> +    } while (!consumed && container);
> +    return consumed;

Add a line between these two.

> WebCore/page/SpatialNavigation.cpp:64
> +    , rect(nodeRectInAbsoluteCoordinates(n, true))

Please add a "/* */" comment explaining the bool.

> WebCore/page/SpatialNavigation.cpp:301
> +{

Could you consider making use of isRectInDirection() all over this method, instead of "curRect.x() < targetRect.right()" , "targetRect.x() < curRect.right()"...

It made practical results better for me, back on time.

> WebCore/page/SpatialNavigation.cpp:494
> +    default:
> +        break;
> +    }

Add an assert_not_reached here, and return :)

> WebCore/page/SpatialNavigation.cpp:528
> +            ASSERT_NOT_REACHED();

add a 'return' here as well.

> WebCore/page/SpatialNavigation.cpp:564
> +            ASSERT_NOT_REACHED();

'return' here too.

> WebCore/page/SpatialNavigation.cpp:569
> +
> +        return true;

Remove this empty line.

> WebCore/page/SpatialNavigation.cpp:667
> +        if (parent->isDocumentNode()) // This is true only if node is a document node.

I would remove this comment.

> WebCore/page/SpatialNavigation.cpp:672
> +            parent = parent->parentNode();
> +    } while (parent && !canScrollInDirection(direction, parent) && !parent->isDocumentNode());
> +    return parent;

Empty line between this two.

> WebCore/page/SpatialNavigation.cpp:705
> +    frame->view()->calculateScrollbarModesForLayout(horizontalMode, verticalMode);

Maybe FrameView::calculateScrollbarModesForLayout does not consider the case when Webkit side disabled the scrollbars? See ScrollView::scrollbarModes()...

> WebCore/page/SpatialNavigation.cpp:788
> +        ASSERT_NOT_REACHED();

'return' here too.

> WebCore/page/SpatialNavigation.cpp:848
> +    switch (direction) {
> +    case FocusDirectionLeft:
> +        if (nodeRect.right() > currentRect.x())
> +            return;
> +        break;
> +    case FocusDirectionUp:
> +        if (nodeRect.bottom() > currentRect.y())
> +            return;
> +        break;
> +    case FocusDirectionRight:
> +        if (nodeRect.x() < currentRect.right())
> +            return;
> +        break;
> +    case FocusDirectionDown:
> +        if (nodeRect.y() < currentRect.bottom())
> +            return;

are not you doing these checks in areRectsMoreThanFullScreenApart?

> WebCore/page/SpatialNavigation.cpp:851
> +        ASSERT_NOT_REACHED();

return here.

> WebCore/page/SpatialNavigation.cpp:857
> +    int sameAxisDist = 0;
> +    int otherAxisDist = 0;

Lets spell out 'Dist'.

> WebCore/page/SpatialNavigation.cpp:898
> +    IntRect candidateRect = nodeRectInAbsoluteCoordinates(candidate.node);

do not need to do this. Just use candidate.rect ?

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