Bug 22740

Created attachment 25852 [details] patch to QWebFrame to get the next focus point given a point and a direction Added a method to QWebFrame to return the next focus point given a direction and a point. This function enables directional tab navigation. The details of the algorithm can be found at http://www.w3.org/TR/WICD/#nav-distance-fkt

Comment on attachment 25852 [details] patch to QWebFrame to get the next focus point given a point and a direction Good stuff Joe, but I have a few questions about the concept, let's discuss on IRC. Some style comments, see http://webkit.org/coding/coding-style.html and http://trac.webkit.org/wiki/Coding%20Style%20Guidelines > + //The dotDist is the euclidian distance Spaces beteeen // and comment > + int dotDist = 0; A better name for this? > + int Overlap = 0; > + int xdisplacement = 0; > + int ydisplacement = 0; Variables should be lowerCaseTitleCase > + dy = focus.y()-rect.bottom(); Spaces between operators > + } > + else if (direction == QWebFrame::South) { else if on the same line as closing brace > + else if (focus.y() > rect.bottom()) > + dy = focus.y() - rect.bottom(); > + > + } Don't need the blank line > + //overlapping rectangles are rewarded > + if ((rect.y()>=rbound.y() && rect.y()<=rbound.bottom()) || > + (rect.bottom()>=rbound.y() && rect.bottom()<=rbound.bottom())) { Boolean operators aligned on the left side, not on the right > + QPoint next = m_view->page()->mainFrame()->nextFocusPoint(QWebFrame::South,current); Can probably store mainFrame here and use later instead of going through the view and page.

I think this functionality should exist in WebCore, rather than in any particular port's API layer. That way all ports can benefit from the functionality. I think it may be possible to share some of the existing code in FocusController::advanceFocus with this new functionality. I'd imagine some changes like the following: FocusDirection gains new members Up, Down, Left, Right or similar The calls to Document::{next,previous}FocusableNode are replaced with calls to a static helper function that looks something like this: static Node* nextFocusableNodeInDirection(Node* start, FocusDirection direction) { switch (direction) { ... } } The rest of advanceFocus could probably remain largely the same.

Created attachment 25965 [details] focus node navigation using arrow keys Moved functionality up so it is usable by all platforms. Form control navigation needs to be handled by the client, similar to tab navigation.

Comment on attachment 25965 [details] focus node navigation using arrow keys 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@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.

(In reply to comment #5) Thanks for taking the time to review, and adding your comments. This is really useful as it helps me get closer to being able to participate. > 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. The <meta> tag is non-sense (i.e. it is just the way that we want to control the feature and I will figure out a way to generalize it for each platform.) As far as why it should be built in to the browser, the best reason I have is that it's a requirement for an implementation that we are building on the Qt port, and it seems like I have the following options: 1.) I can convince WebKit.org (you) to accept this. 2.) I can move the feature to the port and try and convince Qt to accept 3.) I could try and convince Qt to expose whatever functionality I need to implement the feature through their API. 4.) Fork my own branch This must be a common problem as 3rd parties want to implement different features, and I would guess there is no general rule so I will try all options before going to #4. The remaining comments are appreciated and will be addressed in the next patch.

Created attachment 26126 [details] directional focus node navigation patch This patch address the comments from the last review. It also adds a layout test, however it is disabled for all platforms. Platforms need to handle navigation of the input boxes and text areas if they want to enable this feature. The meta tag check is gone and a setting is added to let platforms choose when this should be used, and if the platforms want to use this they can enable the layout test. Please review and give comments.

I have a couple of changes that will improve the search. I want to submit another patch, will any reviewer provide comments on what I have here?

(In reply to comment #6) > The <meta> tag is non-sense (i.e. it is just the way that we want to control > the feature and I will figure out a way to generalize it for each platform.) Who is "we" here? Should this be something that web sites turn on explicitly or not? Should it be controlled by the site or by the browser. > As far as why it should be built in to the browser, the best reason I have is > that it's a requirement for an implementation that we are building on the Qt > port, and it seems like I have the following options: > > 1.) I can convince WebKit.org (you) to accept this. > 2.) I can move the feature to the port and try and convince Qt to accept > 3.) I could try and convince Qt to expose whatever functionality I need to > implement the feature through their API. > 4.) Fork my own branch > > This must be a common problem as 3rd parties want to implement different > features, and I would guess there is no general rule so I will try all options > before going to #4. Sorry, I'm still mixed up about this. Your answer seems to be about the mechanics of getting the code checked in, but I'm asking about the rationale for who would want this feature and under what circumstance. You say "it's a requirement for an implementation". I presume you mean a particular browser for a particular platform. Do you think that web browsers in general should all have this feature? Or do you think it's something they'd need to turn on explicitly as WebKit clients? Would users turn this on, or websites, or web browser developers?

(In reply to comment #7) > This patch address the comments from the last review. It also adds a layout > test, however it is disabled for all platforms. Platforms need to handle > navigation of the input boxes and text areas if they want to enable this > feature. The meta tag check is gone and a setting is added to let platforms > choose when this should be used, and if the platforms want to use this they can > enable the layout test. I think that for testing purposes we should include this code in all platforms and offer a way to turn it on for layout tests on Mac OS X at least, and on other platforms too.

(In reply to comment #8) > I have a couple of changes that will improve the search. I want to submit > another patch, will any reviewer provide comments on what I have here? I'll just review the new patch. Sorry I hadn't gotten to this yet. Because the title of the bug says QWebFrame, it's easy to overlook the fact that this is actually a cross-platform patch. You may want to retitle the bug.

(In reply to comment #9) Thanks again for your comments. Sorry for the confusion. > You say "it's a requirement for an implementation". I presume you mean a > particular browser for a particular platform. My particular browser is a client of the Qt port, and the Qt port does not provide an Api for this type of navigation. This is also why the title says QWebFrame, as that's where I started. > Do you think that web browsers in general should all have this feature? Or do > you think it's something they'd need to turn on explicitly as WebKit clients? > Would users turn this on, or websites, or web browser developers? > I think this feature could be controlled by the websites, which was the idea behind the meta tag, this is how my particular browser would want turn the feature on and off, however users could enable it just as well. The confusing part is that web browser developers would also need to enable the feature. For these browsers, like Safari, there would be some work since they need to handle the 4 way navigation in the editor client (i.e. not to get stuck in select elements, text areas, and input boxes) if they wanted the feature. I think this is how it works for navigating using the TAB key, and this is also why I disabled the layout tests for now, the idea being I would enable the test when the feature was implemented/completed for the particular browser platform. Maybe I can find a cross platform way of handling input boxes?

(In reply to comment #12) > > Do you think that web browsers in general should all have this feature? Or do > > you think it's something they'd need to turn on explicitly as WebKit clients? > > Would users turn this on, or websites, or web browser developers? > > I think this feature could be controlled by the websites, which was the idea > behind the meta tag Sorry, I still don't understand this. Why would a website author turn this on? This doesn't seem like something a website would need to customize. Maybe you can give a specific example of when a website would need to turn it on or off and how that's helpful. > For these browsers, like Safari, there would be some work since they > need to handle the 4 way navigation in the editor client (i.e. not to get stuck > in select elements, text areas, and input boxes) if they wanted the feature. You lost me again. Lets say this feature was on. Presumably you'd have some user interface to trigger the directional navigation. What would it be? Normal keys on the keyboard? Some special key combination? A special keyboard? I don't know what you mean about "getting stuck" exactly. To me it makes sense to have a command that lets you move focus from one place to another, directionally. Even if there's no keyboard binding on a particular platform for that operation. I don't see why that would be a compile time switch. You could make it accessible from JavaScript and also bound to particular input keys on particular platforms. So it could be tested anywhere.

There seems to be some overlap between this and bug 18662.

Note that Node::getRect() does not take CSS transforms into account.

(In reply to comment #13) > Sorry, I still don't understand this. Why would a website author turn this on? > This doesn't seem like something a website would need to customize. Maybe you > can give a specific example of when a website would need to turn it on or off > and how that's helpful. Having a website enable this feature is only hypothetical for a specific implementation. The patch controls the feature through a new flag in the settings, so it can be enabled in whatever way makes sense for a particular browser. However, I think I understand what you mean, in that having a common way of turning this on validates it as a feature that is useful for all platforms. I suppose it's sort of critical to define this and what is the benefit of adding this code here and not implementing it somewhere else, like in the client, which is what the first patch here does. > > > For these browsers, like Safari, there would be some work since they > > need to handle the 4 way navigation in the editor client (i.e. not to get stuck > > in select elements, text areas, and input boxes) if they wanted the feature. > > You lost me again. Lets say this feature was on. Presumably you'd have some > user interface to trigger the directional navigation. What would it be? Normal > keys on the keyboard? Some special key combination? A special keyboard? > > I don't know what you mean about "getting stuck" exactly. Sorry, I wasn't clear. You need to modify the editor client when navigating a text box using the right left up down keyboard keys, defining some sort of precedence on when to set the key event as handled. What I do is check the cursor position in relation to the text. If you can navigate to another character or line feed do so, otherwise return without handling the key event and let the next closest node be chosen. This is a problem with other types of form controls as well, and usually comes down to leaving some functionality out if you are navigating using this feature. Maybe this is a reason for websites to enable the feature (since navigating this way generally works better with certain content). Thanks for spending the time, I'll keep working on defining this and take a look at bug 18662.

(In reply to comment #13) > You lost me again. Lets say this feature was on. Presumably you'd have some > user interface to trigger the directional navigation. What would it be? Normal > keys on the keyboard? Some special key combination? A special keyboard? > > I don't know what you mean about "getting stuck" exactly. > > To me it makes sense to have a command that lets you move focus from one place > to another, directionally. Even if there's no keyboard binding on a particular > platform for that operation. I don't see why that would be a compile time > switch. You could make it accessible from JavaScript and also bound to > particular input keys on particular platforms. > > So it could be tested anywhere. Sorry. I missed your point in my last comment. I was thinking of navigating using a joystick, and wanted it to be seamless across text areas. I think a special key combination could work with a joystick, I would just need a way to enter and exit the control (maybe with an extra button push). Any ideas?

(In reply to comment #17) > I was thinking of navigating > using a joystick, and wanted it to be seamless across text areas. I think a > special key combination could work with a joystick, I would just need a way to > enter and exit the control (maybe with an extra button push). Any ideas? You'd need to make sure these joystick events were distinguishable from other keyboard events. The new behavior would not be based on compile-time switches, but rather on the event objects themselves. So there would not be any #if in the new code.

If you do add new events types for joystick, please generalize them so that they could apply to any directional navigation input device.

(In reply to comment #19) > If you do add new events types for joystick, please generalize them so that > they could apply to any directional navigation input device. I agree, of course. We want to use a sensible abstraction of some type. And I should clarify that these don't need to be entirely new event types. We could decide that the joystick directional events are treated as keyboard events, but somehow distinguishable by some additional setting or property on the event object.

(In reply to comment #20) > (In reply to comment #19) > > If you do add new events types for joystick, please generalize them so that > > they could apply to any directional navigation input device. > > I agree, of course. We want to use a sensible abstraction of some type. > > And I should clarify that these don't need to be entirely new event types. We > could decide that the joystick directional events are treated as keyboard > events, but somehow distinguishable by some additional setting or property on > the event object. > Ok thanks, I will see what I can figure out, and will put this under a new "joystick event types" bug when I have something.

Comment on attachment 26126 [details] directional focus node navigation patch This is really the same thing as bug 18662. This patch looks pretty good, but you guys need to pool your efforts. I'm going to mark this bug as a duplicate of that other one and set the review flag on this to review- for now. Please coordinate so we don't try to do the same thing twice. There are some things stronger in your patch and some stronger in that other patch, so I'm sure we can get a great result by working together.

*** This bug has been marked as a duplicate of 18662 ***