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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 15 07:29:36 PST 2010


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





--- Comment #17 from Yael <yael.aharon at nokia.com>  2010-11-15 07:29:36 PST ---
(In reply to comment #16)
Thank you for the review, my comments are below. 

> > 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.

I will rename the method to reflect the fact that we do not want to select elements that are more than a full screen away. 
There is no other place that this is checked. If I remove this, and there is an element in full alignment, we will scroll all the way down to it, and will refuse any other element that is closer, but not in full alignment.

> > 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.
> 
Good idea, I will change FocusCandidate to include all the necessary information.

> > WebCore/page/SpatialNavigation.cpp:653
> > +Node* scrollableOrFrameParentForNodeAndDirection(FocusDirection direction, Node* node)
> 
> FocusCandidate POD has a enclosingScrollableBox. Why not using this?

This is used during the initialization, before we have a FocusCandidate.


> > 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.
> 
Cannot do that:-) The check you are looking at is for the _current_ node, if we start from a document node, we want to ignore it and keep going up the parent tree. I will add a comment to explain that better.

> 
> > 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.
In my Symbian build of WRT there are scrollbars, I need to check what is going on :-) 

> 
> > 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.
ok

I found a problem with the way I am detecting divs with overflow:hidden, so I will fix that before submitting a new patch.

-- 
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