[webkit-reviews] review requested: [Bug 18662] Extend keyboard navigation to allow directional navigation : [Attachment 49463] Add directional navigation to WebCore (v15)

[bugzilla-daemon at webkit.org]

Antonio Gomes (tonikitoo) <tonikitoo at webkit.org> has asked  for review:
Bug 18662: Extend keyboard navigation to allow directional navigation
https://bugs.webkit.org/show_bug.cgi?id=18662

Attachment 49463: Add directional navigation to WebCore (v15)
https://bugs.webkit.org/attachment.cgi?id=49463&action=review

------- Additional Comments from Antonio Gomes (tonikitoo)
<tonikitoo at webkit.org>
Addressed most of Simon's comments.

(In reply to comment #63)
> (From update of attachment 49074 [details])
> > +	 FocusDirection focusDirectionForKey(const AtomicString&);
> 
> This method should be |const|

Fixed.
 
> > +	     FocusDirection tabDirection = (FocusDirectionUp || direction ==
FocusDirectionLeft) ?
> > +					    FocusDirectionForward :
FocusDirectionBackward;
> 
> Shouldn't this be:
> 
>   FocusDirection tabDirection = ( direction == FocusDirectionUp || direction
==
> FocusDirectionLeft)
> ?

Fixed. Not sure how I miss that for so long.

> > +	 // Move up in the chain of nested frames.
> > +	 frame = frame->tree()->top();
> 
> I don't undersand this. Why jump to the topmost frame?

Simon, the logic used here relies on a top-down traversal algorithm
to walk through all keyboard focusable elements in the dom tree,
looking the "closest" node. That is why it goes to the top one
in before all.

> > +	     if (!(isMovingToRootDocument(focusedNode, candidate)
> > +		 && (closestFocusCandidate.node
> > +		 && (focusedNode->document() ==
closestFocusCandidate.node->document())))) {
> 
> You don't need so many parens here.

Fixed.

> >  namespace WebCore {
> >	 enum FocusDirection {
> >	     FocusDirectionForward = 0,
> > -	     FocusDirectionBackward
> > +	     FocusDirectionBackward,
> > +	     FocusDirectionNone,
> > +	     FocusDirectionUp,
> > +	     FocusDirectionDown,
> > +	     FocusDirectionLeft,
> > +	     FocusDirectionRight
> >	 };
> 
> Would making FocusDirectionNone have value 0 break anything? It would be
> preferable.

Fixed. It does not break anything, and indeed looks better.

> > +++ b/WebCore/page/SpatialNavigation.cpp
> 
> > +namespace WebCore {
> > +
> > +long long spatialDistance(FocusDirection, const IntRect&, const IntRect&);

> > +IntRect renderRectRelativeToRootDocument(RenderObject*);
> > +RectsAlignment alignmentForRects(FocusDirection, const IntRect&, const
IntRect&);
> > +bool areRectsFullyAligned(FocusDirection, const IntRect&, const IntRect&);

> > +bool areRectsPartiallyAligned(FocusDirection, const IntRect&, const
IntRect&);
> > +bool isRectInDirection(FocusDirection, const IntRect&, const IntRect&);
> 
> I'm not sure if we normally declare file-local functions as 'static'. Take a
> look at some other files to see.

Fixed. Made all file-local functions static.

> 
> > +// FIXME_tonikitoo: FocusCandidate distanceDataForNode(start, dest,
direction)
> It's not clear what the action item is.

Fixed. Comment removed.

> > +long long distanceInDirection(Node* start, Node* dest, FocusDirection
direction, FocusCandidate& candidate)
> > +{
> > +	 long long& distance = candidate.distance;
> 
> You set distance to be a reference to candidate.distance here...
> 
> > +	     // Reset distance if we are now in higher precedence case.
> > +	     if (candidate.alignment < alignment && candidate.parentAlignment <
alignment)
> > +		 distance = cMaxDistance;
> 
> and then assign to it lower down. Why use the reference at all? This is hard
to
> follow.

Right the logic here is: in case we find a node with a higher focus precedence
than the current best focus candidate (e.g. totally or partially aligned), 
we want to take that node regardless its distance. So I set current best
candidate
node's current distance to the maximum possible value, which makes me sure that
any
distance we get from |spatialDistance| later on is shorter.

I modified to comment in the code to better reflect that.

> > +	     // FIXME_tonikitoo: make this assignment in the caller method ?
> 
> Either do it, or remove the FIXME.

Removed.

> We would normally put braces at if (useScrollOffset) {

Fixed.

> > +#define MIDDLE(direction, rect)	   \
> > +	 isHorizontalMove(direction) ?	   \
> > +	     rect.y() + rect.height() / 2: \
> > +	     rect.x() + rect.width()  / 2;
> > +
> > +#define END(direction, rect)	   \
> > +	 isHorizontalMove(direction) ?	   \
> > +	     rect.y() + rect.height() :    \
> > +	     rect.x() + rect.width();
> > +
> > +#define START(direction, rect)	   \
> > +	 isHorizontalMove(direction) ? rect.y() : rect.x();
> 
> Please convert these into inline functions.

Fixed. static inline functions added.

> > +// This method checks if rects |a| and |b| are fully aligned either
vertical or
> 
> vertically

Fixed.

> > +// In essence, targets whose middle falls between the top or bottom of the
current rect.
> 
> This isn't a complete sentence.

Fixed.

> > +	 // About the logic below:
> 
> What about it?

Fixed. Comment added in the code.

> > +// * a = Current focused node.
> > +// * b = Focus candidate node.
> 
> In all these methods, rather than commenting what a and b are, you should
just
> name them appropriately: curRect, targetRect, or something.

I agree w/ you, but it could make code harder to read, imho.
And comment is used three times, not much, again imho.
So in this round, I opted to leave it was it was, but if you
really prefer I can fix it as you wish. 
 
> > +	     if (a.y() > b.y() + b.height()) {
> 
> use IntRect:bottom(), IntRect:right() here and in all these methods.

Fixed.

> You could also define some inline methods like:
> 
> bool below(const IntRect& r, int pos)
> {
>   return pos > r.bottom();
> }

Added:
   static inline bool belowOf(const intrect& rect a , const intrect& rect b)
and a correspondent |rightOf| function. is that ok ?

> to make this code more readable. You could also add IntPoint
IntRect::center()
> const if you like (FloatRect has one).

Done in bug 35346.

> > +bool isMovingToRootDocument(Node* from, Node* to)
> > +{
> > +	 if (!from || !to)
> > +	     return false;
> > +
> > +	 Document* rootDocument =
to->document()->page()->mainFrame()->document();
> > +	 return (to->document() == rootDocument && from->document() !=
to->document());
> > +}
> 
> The 'moving to' terminology is confusing. I think this would be clearer with:

> 
> bool isInRootDocument(Node*)
> and then put the other logic at the call site.

Fixed. Done.

> > +void deflateIfNeeded(IntRect& a, IntRect& b, int fudgeFactor)
> If needed for what?

I think comments in the caller site says it. See:

    // The bounding rectangle of two consecutive nodes can overlap. In such
cases,
    // deflate both.
    deflateIfOverlapped(curRect, targetRect);

btw, i renamed function to |deflateIfOverlapped|.

> I think this needs one more round of changes.

very helpful review. thx