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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 14 07:30:16 PST 2010


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


Antonio Gomes <tonikitoo at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #73729|                            |review-
               Flag|                            |




--- Comment #16 from Antonio Gomes <tonikitoo at webkit.org>  2010-11-14 07:30:15 PST ---
(From update of attachment 73729)
View in context: https://bugs.webkit.org/attachment.cgi?id=73729&action=review

Nice work! 

It needs one more round, though. I would like to get some of my comments addressed and questions answered.

> WebCore/page/SpatialNavigation.cpp:291
> +        return curRect.x() < targetRect.right() || curRect.x() - targetRect.right() > viewSize.width();

"curRect.x() < targetRect.right()". This should not be tested here OR the method has to be renamed. iirc there were other methods that would catch these cases.

areRectsTooFarApart essentially test the second conditions e.g. "curRect.x() - targetRect.right() > viewSize.width();"

> WebCore/page/SpatialNavigation.cpp:293
> +        return targetRect.x() < curRect.right() || targetRect.x() - curRect.right() > viewSize.width();

ditto

> WebCore/page/SpatialNavigation.cpp:295
> +        return curRect.y() < targetRect.bottom() || curRect.y() - targetRect.bottom() > viewSize.height();

ditto

> WebCore/page/SpatialNavigation.cpp:297
> +        return targetRect.y() < curRect.bottom() || targetRect.y() - curRect.bottom() > viewSize.height();

ditto

> WebCore/page/SpatialNavigation.cpp:524
> +bool scrollInDirection(Node* container, FocusDirection direction, const FocusCandidate&)

I wish, if possible you keep using the enclosingScrollableBox logic in FocusCandidate.

That way you should just do

scrollInDirection(direction, focusCandidate);

focuscandidate would have a reference for the 'container' node.

> WebCore/page/SpatialNavigation.cpp:653
> +Node* scrollableOrFrameParentForNodeAndDirection(FocusDirection direction, Node* node)

FocusCandidate POD has a enclosingScrollableBox. Why not using this?

> WebCore/page/SpatialNavigation.cpp:659
> +        if (parent->isDocumentNode())
> +            parent = static_cast<Document*>(parent)->document()->frame()->ownerElement();

put a 'break' right below here, and remove the isDocumentNode check from the loop condition.

> WebCore/page/SpatialNavigation.cpp:673
> +    if (!container->renderBox() || !container->renderBox()->canBeScrolledAndHasScrollableArea())
> +        return false;

why not use isScrollableContainerNode() ?

> WebCore/page/SpatialNavigation.cpp:695
> +    if ((direction == FocusDirectionLeft || direction == FocusDirectionRight) && !frame->view()->horizontalScrollbar())
> +        return false;

Will it work with scrollbar policy thing of the Qt API? It is toggled OFF for WRT.

> WebCore/page/SpatialNavigation.cpp:717
> +IntRect nodeRectInAbsCoords(Node* node, bool ignoreBorder)

Abs => Absolute

> WebCore/page/SpatialNavigation.cpp:732
> +    for (Frame* frame = node->document()->frame(); frame; frame = frame->tree()->parent()) {
> +        if (Element* element = static_cast<Element*>(frame->ownerElement())) {
> +            do {
> +                rect.move(element->offsetLeft(), element->offsetTop());
> +            } while ((element = element->offsetParent()));
> +            rect.move((-frame->view()->scrollOffset()));
> +        }
> +    }

This steps are duplicated here and in frameRectInAbsCoords. Lets make it a helper.

> WebCore/page/SpatialNavigation.cpp:734
> +    // For authors that use border indtead of outline in their CSS, we compensate by ignoring the border when calculating

typo: inStead

> WebCore/page/SpatialNavigation.cpp:744
> +IntRect frameRectInAbsCoords(Frame* frame)

Abs -> Absolute.

> WebCore/page/SpatialNavigation.cpp:755
> +    for (; frame; frame = frame->tree()->parent()) {
> +        if (Element* element = static_cast<Element*>(frame->ownerElement())) {
> +            do {
> +                rect.move(element->offsetLeft(), element->offsetTop());
> +            } while ((element = element->offsetParent()));
> +            rect.move((-frame->view()->scrollOffset()));
> +        }
> +    }

Duplicated code.

> WebCore/page/SpatialNavigation.cpp:816
> +void distanceDataForNode(FocusDirection direction, const IntRect& startingRect, FocusCandidate& candidate)

In my opinion,the signature of this method is inconsistent: you should be either pass the direction and both nodes OR the direction and both rects.

Maybe we need this first, where "startingNode" would be "FocusCandidate focusedNode". If there is not current focused not, focusedNode.isNull() would catch it and then you get your wanted starting rect from within this method.

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



More information about the webkit-unassigned mailing list