[webkit-reviews] review denied: [Bug 22740] It's not possible for clients of QWebFrame to implement directional focus node navigation : [Attachment 25965] focus node navigation using arrow keys
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 12 10:36:12 PST 2008
Darin Adler <darin at apple.com> has denied Joseph Ligman
<joseph.ligman at nokia.com>'s request for review:
Bug 22740: It's not possible for clients of QWebFrame to implement directional
focus node navigation
https://bugs.webkit.org/show_bug.cgi?id=22740
Attachment 25965: focus node navigation using arrow keys
https://bugs.webkit.org/attachment.cgi?id=25965&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
This looks like a good start. Thanks for working on this!
I'm a little confused about the feature you're implementing. I'm not sure I
understand why this should be controlled by web pages and not by the web
browser. But I think it's a great step in the right direction to have the
concept of navigating geometrically, especially if we can get a good
implementation. And I'm sure you can educate me about why this should be built
into web browsers by default and controlled by a <meta> tag in the web page.
New features like this need some tests. You need to learn to write tests using
our "layout test" framework and include a test of the new code in your patch.
> +2008-12-11 Joe Ligman <jligman at mindspring.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Added focuspoint navigation based on
http://www.w3.org/TR/WICD/#nav-distance-fkt.
Should mention the URL to the bugs.webkit.org bug in the change log entry.
> + * ../WebCore/dom/Document.cpp:
> + * ../WebCore/dom/Document.h:
> + * ../WebCore/page/EventHandler.cpp:
> + * ../WebCore/page/EventHandler.h:
> + * ../WebCore/page/FocusController.cpp:
> + * ../WebCore/page/FocusDirection.h:
The path names here should not have '../WebCore' in front of them. The files
should have the function names in them, and good change log entries also have
per-function comments with some more detail of what's being done.
> + if (direction == FocusDirectionNorth) {
We'd normally use a switch statement rather than cascading if statements for
cases like these.
> +static int distanceFunction(FocusDirection direction, const IntRect&
possibleRect, const IntPoint& focusPoint, const IntRect& focusedRect)
This shouldn't be named "distanceFunction" because we name functions after
their results; we don't include the word "function" in their names. So it could
be called "distance" or something more specific.
> +{
> + // The absolute distance (dx or dy) on the navigation axis between the
opposing edges of the currently focused
> + // element and each of the canidates.
Typo here: "canidates".
> + int distanceX = 0;
> + int distanceY = 0;
Why set these to 0 -- they're already set in all cases.
> +
> + // The euclidian distance
> + int euclidianDist = 0;
I believe Euclidean has an "e", not an "i", in it.
> + IntRect focusedRect = (startNode)?startNode->getRect():IntRect(0,0,0,0);
This is not the formatting we'd normally use. This should be written like this:
IntRect focusRect = startNode ? startNode->getRect() : IntRect();
No need for the "0, 0, 0, 0".
> + int distance = -1;
I suggest setting distance to std::numeric_limits<int>::max() instead of -1,
then you won't need a special case in the if statement inside the loop.
> + IntRect focusRect;
This is a "write-only" variable. It is defined and set to various values, but
never used. I suggest removing it.
> + Node* nextNode = 0;
I suggest calking this something like bestNode instead of nextNode. There are a
lot of "next" nodes in this code.
> + for(Node* n = this; n != 0; n = n->traverseNextNode()) {
We use a space after the "for", and "n" rather than "n != 0". See the coding
style guidelines for details.
> + if(n->isFocusable()) {
We use a space after the "if".
> + if (direction == FocusDirectionNorth && r.bottom() >=
focusPoint.y()) continue;
> + if (direction == FocusDirectionSouth && r.y() <= focusPoint.y())
continue;
> + if (direction == FocusDirectionEast && r.x() <= focusPoint.x())
continue;
> + if (direction == FocusDirectionWest && r.right() >=
focusPoint.x()) continue;
I think this logic would read better if the check was a function, rather than
four different if/continue statements. The name of the function would help
explain what's going on.
> + int dis = distanceFunction(direction, r, focusPoint,
focusedRect);
I think dis is an unclear name for this. I suggest calling the other one
bestDistance and this one distance, instead of distance and dis.
> + if (distance == -1 || distance > dis) {
> + distance = dis;
> + focusRect = r;
> + nextNode = n;
> + }
> + }
> + }
> +
> + return nextNode;
> +}
> #include <wtf/ListHashSet.h>
> -
> +#include "FocusDirection.h"
> // FIXME: We should move Mac off of the old Frame-based user stylesheet
loading
You should not remove the space between the list of includes and the
> // code and onto the new code in Page. We can't do that until the code in
Page
> // supports non-file: URLs, however.
> @@ -606,6 +606,18 @@ public:
> */
> Node* previousFocusableNode(Node* start, KeyboardEvent*);
>
> + /**
> + * Searches through the document, starting from the start node. chooses
the next node based on a focus point
> + * algorithm.
> + *
> + * @param FocusDirection
> + *
> + * @return The focus node to start the search
> + *
> + * See http://www.w3.org/TR/WIDC/#nav-distance-fkt
> + */
> + Node* nextFocusableNodeInDirection(FocusDirection direction, Node*
startNode);
Comments are good, but you need to match the style in the rest of the source
file. You'll note that the other functions in this file don't have this style
of comment, and we don't want to move in that direction for the file. Please
make the comment closer in style to the rest of this file. Things like the "see
URL" can go in the .cpp file rather than the header.
We omit argument names like "direction" when the type of the argument already
makes it clear without an argument name.
I don't understand why this function only looks in nodes that are subsequent to
this one in the document. What if the node that's the best candidate is earlier
in document order rather than later?
> + bool tabbed = false;
> + Document* doc = m_frame->document();
> + if (doc) {
> + RefPtr<NodeList> list = doc->getElementsByTagName("meta");
> + unsigned len = list->length();
> + for (unsigned i = 0; i < len; i++) {
> + HTMLMetaElement* meta =
static_cast<HTMLMetaElement*>(list->item(i));
> + if (meta->name() == "navigation" && meta->content() ==
"tabbed")
> + tabbed = true;
> + }
> + }
I think this loop should be in a separate helper function rather than right
here in the event handler code. It makes the logic easier to read.
Also, is this <meta> tag rule really the way we want to trigger this feature?
Maybe there are some web browsers that would want to trigger it based on the
browser rather than on the web page content. I'm not entirely
> + if (tabbed)
> + directionalTabEventHandler(event);
I think this naming is inconsistent with other similar functions in this file.
Functions like handleKeyboardSelectionMovement have verbs for their names,
saying what they do. But maybe there are others named like your new one.
> // provides KB navigation and selection for enhanced accessibility
users
> if (AXObjectCache::accessibilityEnhancedUserInterfaceEnabled())
> - handleKeyboardSelectionMovement(event);
> + handleKeyboardSelectionMovement(event);
It seems that the same event can be handled both by directionalTabEventHandler
and handleKeyboardSelectionMovement -- do we need code to prevent that?
> + if (!event || event->ctrlKey() || event->metaKey() ||
event->altGraphKey())
> + return;
Why the check for !event here? Is it legal to call this with 0?
We really need a comment in this function explaining the high level logic about
when this code should run. I'm surprised about the explicit "design mode" check
-- that won't cover the many other ways HTML editing can be turned on, and I
suspect we'd want this feature off in all those cases too. A comment explaining
when we want this to work would help me determine if the code is correct or
not.
> + if (m_frame->document()->inDesignMode())
> + return;
> +
> + Page* page = m_frame->page();
> + if (!page)
> + return;
> +
> + if (!page->tabKeyCyclesThroughElements())
> + return;
> +
> + String key = event->keyIdentifier();
> +
> + if (key == "Up" &&
page->focusController()->advanceFocus(FocusDirectionNorth, event))
> + event->setDefaultHandled();
> +
> + else if (key == "Down" &&
page->focusController()->advanceFocus(FocusDirectionSouth, event))
> + event->setDefaultHandled();
> +
> + else if (key == "Left" &&
page->focusController()->advanceFocus(FocusDirectionWest, event))
> + event->setDefaultHandled();
> +
> + else if (key == "Right" &&
page->focusController()->advanceFocus(FocusDirectionEast, event))
> + event->setDefaultHandled();
I'd like to see a separate function to map the key name or key event to a
direction, rather than 4 separate if statements here.
> void defaultTabEventHandler(KeyboardEvent*);
> + void directionalTabEventHandler(KeyboardEvent* event);
The word "Tab" in the code above refers to the tab key. But this second
function has nothing to do with the tab key so it should not have tab in its
name. Also, the argument name event should be left out.
>
> void allowDHTMLDrag(bool& flagDHTML, bool& flagUA) const;
>
> - node = (direction == FocusDirectionForward)
> - ? document->nextFocusableNode(0, event)
> - : document->previousFocusableNode(0, event);
> + if (direction == FocusDirectionForward)
> + node = document->nextFocusableNode(0, event);
> + else if (direction == FocusDirectionBackward)
> + node = document->previousFocusableNode(0, event);
> + else
> + node = document->nextFocusableNodeInDirection(direction, 0);
This seems wrong. Instead of changing the code here,
nextFocusableNodeInDirection should handle the forward and backward directions.
> - Node* node = (direction == FocusDirectionForward)
> - ? document->nextFocusableNode(document->focusedNode(), event)
> - : document->previousFocusableNode(document->focusedNode(), event);
> -
> + Node* node = 0;
> + if (direction == FocusDirectionForward)
> + node = document->nextFocusableNode(document->focusedNode(), event);
> + else if (direction == FocusDirectionBackward)
> + node = document->previousFocusableNode(document->focusedNode(),
event);
> + else
> + node = document->nextFocusableNodeInDirection(direction,
document->focusedNode());
Same comment.
> - node = (direction == FocusDirectionForward)
> - ? parentDocument->nextFocusableNode(owner, event)
> - : parentDocument->previousFocusableNode(owner, event);
> + if (direction == FocusDirectionForward)
> + node = parentDocument->nextFocusableNode(owner, event);
> + else if (direction == FocusDirectionBackward)
> + node = parentDocument->previousFocusableNode(owner, event);
> + else
> + node = document->nextFocusableNodeInDirection(direction, owner);
And again.
> + if (direction == FocusDirectionForward)
> + node = d->nextFocusableNode(0, event);
> + else if (direction == FocusDirectionBackward)
> + node = d->previousFocusableNode(0, event);
> + else
> + node = document->nextFocusableNodeInDirection(direction, 0);
> + }
And again.
> + FocusDirectionNorth,
> + FocusDirectionSouth,
> + FocusDirectionEast,
> + FocusDirectionWest
We should call these directions "up", "down", "right", and "left". It doesn't
make sense to use compass directions here.
More information about the webkit-reviews
mailing list