[webkit-reviews] review denied: [Bug 18662] Extend keyboard navigation to allow directional navigation : [Attachment 26499] Add directional navigation to WebCore (v4)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Antonio Gomes (tonikitoo)
<tonikitoo at gmail.com>'s request for review:
Bug 18662: Extend keyboard navigation to allow directional navigation
https://bugs.webkit.org/show_bug.cgi?id=18662

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

------- Additional Comments from Darin Adler <darin at apple.com>
I like the general direction this is going in.

I do see many problems, though.

It's critical that the patch include test cases, not just code changes. And a
change log entry.

> +	   else if (event->keyIdentifier() == "Down"
> +		    || event->keyIdentifier() == "Up"
> +		    || event->keyIdentifier() == "Right"
> +		    || event->keyIdentifier() == "Left") {

This list is long enough that I think we should make a helper function
isArrowKey that takes either a key identifier or a KeyboardEvent*. Maybe it
could also return the focus direction so you don't have to repeat the list of
key identifiers.

> +	       if(m_frame->page()) {
> +		   if (!m_frame->settings())
> +		       return;
> +		   if(m_frame->settings()->enableKeyboardNavigation()) {
> +		       defaultArrowEventHandler(event);
> +		   }
> +	       }

The page and settings checks checks should be moved inside
defaultArrowEventHandler; in fact, the check for a page of 0 is already in
there.

Also, need a space after the if statement.

> +    if (!page->tabKeyCyclesThroughElements())
> +	   return;

What's this tab check check for? That seems like a copy and paste error. And if
not, needs a comment to explain it.

> +    // FIXME: Are CTRL-arrow keys needed in design mode?
> +    if (m_frame->document()->inDesignMode())
> +	   return;

It seems wrong to check design mode specifically here. I think the check you
want is about whether the document has editable text, or whether the insertion
point is inside editable text or if not, if the entire document is editable.
Despite its name that seems to imply otherwise, design mode is just one of many
ways to make a document or part of a document editable.

> +inline static IntRect absoluteRenderRect(RenderObject* render)

We almost always order it as "static inline" rather than "inline static".

> +{
> +    ASSERT(render);
> +    // FIXME: consider 'transforms' ?

Not sure why you put the word transforms in quotes. You should use more normal
formatting here and write something less uncertain:

    // FIXME: This function is flawed because it doesn't consider the effects
of CSS or SVG transforms.

> +    // handle nested frames.
> +    if (Frame* frame = render->document()->frame()) {
> +	   while (frame) {
> +	       HTMLFrameOwnerElement* ownerElement = frame->ownerElement();
> +	       if(Node* node = static_cast<Node*>(ownerElement))
> +		   rect.move(node->renderer()->offsetLeft(),
node->renderer()->offsetTop());
> +
> +	       frame = frame->tree()->parent();
> +	   }
> +    }

This checks frame for null twice. Since you have the while you don't also need
the if. Also, that while should be written as a for statement. And the inner if
statement should be checking ownerElement for 0, not a separate node variable.
There's no reason to copy the ownerElement into a Node*; you can call the
functions directly on the HTMLFrameOwnerElement.

It's not correct to use the offsetLeft and offsetTop of the frame owner
element's renderer because that's just the distance from the offsetParent, not
the absolute position. Instead you should use one of the localToAbsolute family
of functions for this.

> +inline static int rectangleDistance(IntRect rect1, IntRect rect2)

These should be const IntRect& -- we don't want to copy them.

I think the title should say "distance squared" rather than just "distance".

I think there may be excessive use of "inline" here. It might be making the
code bigger and slower than it needs to be.

The code should to be written so that overflow doesn't make extremely distant
elements seems to be very close. That might have to start with a
rectangleDistance function that can handle overflow.

We want test cases that try to put elements at very far-away positions to see
if overflow gives us the wrong result.

> +inline static bool isRectAboveMainDiagonal(const IntPoint& rectPoint, const
IntRect& rect)

I don't understand the name "rectPoint" for this point. What's a "rectangle
point"?

> +    // Check if some points belonging to rect are above the y = x + c line
passing for rectPoint.

Passing "for"? Do you mean "passing through"?

> +    return (isPointAboveMainDiagonal(rectPoint, rect.location())
> +	       || isPointAboveMainDiagonal(rectPoint, IntPoint(rect.right(),
rect.y()))
> +	       || isPointAboveMainDiagonal(rectPoint, IntPoint(rect.x(),
rect.bottom()))
> +	       || isPointAboveMainDiagonal(rectPoint, IntPoint(rect.right(),
rect.bottom())));

No need for the parentheses here in this expression.

If it wasn't for the brokenness of those functions, I would have suggested you
use the named functions like bottomLeft() and bottomRight(). But they're broken
in design, so I won't suggest you use them here unless someone fixes them and
converts the old callers.

> +inline static bool isRectAboveAntiDiagonal(const IntPoint& rectPoint, const
IntRect& rect)

I believe antidiagonal is one word, so the "D" should not be capitalized.

> +static int distanceInDirection(Node* start, Node* dest, FocusDirection
direction)

> +    // The bounding rectangle of two consecutive links could overlap, so we
decrease the size of
> +    // both rectangles.
> +    // FIXME: Is there a better way to do this?
> +    startRect.inflate(-4);
> +    destRect.inflate(-4);

The comment needs to indicate where the constant 4 comes from. Why is 4 good
enough? Why not 3 or 5?

And we need a test case in the regression tests directory that demonstrates
that 4 is a good choice.

> +	   , distance(std::numeric_limits<int>::max())

Should do "using namespace std" at the top of the file and not say "std::"
here.

Also, this code uses both INT_MAX and numeric_limits<int>::max() in the same
algorithm. Lets chose one or the other.

> +	   } else if (candidate != start &&
candidate->isKeyboardFocusable(event) && candidate->tabIndex() >= 0) {

Why the explicit check for tabIndex() here? That seems like it should be the
responsibility of isKeyboardFocusable? Is there any special reason you're
ruling out items that have a negative tab index? If so, I think you need a
comment and if the rule really needs to be different than isKeyboardFocusable
then it should be encapsulated in a function like isDirectionallyFocusable (or
some better name).

We need a test demonstrating and double-checking which elements are and are not
focusable with directional navigation.

> +bool FocusController::advanceFocusDirectional(FocusDirection direction,
KeyboardEvent* event, bool initialFocus)

The name isn't great here because "advance focus directional" is not good
grammar. Even advanceFocusDirectionally would be slightly better, but it's not
a good name either because "forwards" and "backwards" seem like directions to
me. Maybe advanceFocusGeometrically? You should think about what you'd call
this in conversation and see if you can convert that into a name.

The meaning of the initialFocus argument is quite unclear.

It would be better if the algorithm was expressed in terms of Element* rather
than Node* from top to bottom. I'd rather see that handled right when we go
from the renderer to its node, rather than higher up.

> +bool FocusController::advanceFocusLinear(FocusDirection direction,
KeyboardEvent* event, bool initialFocus)

This is not a good name. Moving forward and backward through the document is
not "linear" -- it doesn't go along a line. I'd suggest
advanceFocusInDocumentOrder perhaps.

> +    , m_enableKeyboardNavigation(false)

Unfortunately, "keyboard navigation" already has a meaning, especially to the
visually impaired, and I don't believe it has anything to do with using arrow
keys to move between elements based on their geometric position. We need a name
that's more specific to what this feature is.

I'm not sure I like the idea of coding in the key bindings to the arrow keys.
But since, for good or bad, that's what this turns on and off, the name could
be shouldUseArrowKeysForDirectionalNavigation or something along those lines.

I think we should consider exposing the directional navigation to JavaScript
somehow, so we can test it even in a browser that has the end-user binding
turned off.

Something like commands for execCommand, but maybe not that because there could
be compatibility consequences of adding something there.