[webkit-reviews] review denied: [Bug 18662] Extend keyboard navigation to allow directional navigation : [Attachment 47640] Add directional navigation to WebCore (v11)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 28 12:59:58 PST 2010
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antonio Gomes
(tonikitoo) <tonikitoo at webkit.org>'s request for review:
Bug 18662: Extend keyboard navigation to allow directional navigation
https://bugs.webkit.org/show_bug.cgi?id=18662
Attachment 47640: Add directional navigation to WebCore (v11)
https://bugs.webkit.org/attachment.cgi?id=47640&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> diff --git a/WebCore/page/EventHandler.cpp b/WebCore/page/EventHandler.cpp
> +FocusDirection EventHandler::isArrowKey(const String& keyIdentifier)
The name of this method is confusing. An "isFoo" method should return a bool.
It should be something like
"focusDirectionForKey()" and at call sites you'd do
if (focusDirectionForKey(s) != FocusDirectionNone) {
...
> +{
> + FocusDirection retVal = (FocusDirection) 0;
I think there should be a FocusDirectionNone enum value.
> + if (keyIdentifier == "Down")
> + retVal = FocusDirectionDown;
> + else if (keyIdentifier == "Up")
> + retVal = FocusDirectionUp;
> + else if (keyIdentifier == "Left")
> + retVal = FocusDirectionLeft;
> + else if (keyIdentifier == "Right")
> + retVal = FocusDirectionRight;
Maybe use AtomicStrings for these?
> diff --git a/WebCore/page/FocusController.cpp
b/WebCore/page/FocusController.cpp
> +enum PrecedenceMode {
> + None = -1,
> + Low,
> + Precedent,
> + SuperPrecedent
> +};
Precedence of what? Maybe rename to FocusCandidatePrecedence?
Also needs a comment to explain how "SuperPrecedent" differs from "Precedent".
Why not just use a numeric value, or "Low, Medium, High"?
> +struct BestCandidate {
> + BestCandidate()
> + : node(0)
> + , distance(numeric_limits<long long>::max())
> + , parentDistance(numeric_limits<long long>::max())
> + , mode(None)
> + , parentMode(None)
> + {
> + }
> + Node* node;
> + long long distance;
> + long long parentDistance;
> + PrecedenceMode mode;
> + PrecedenceMode parentMode;
> +};
BestCandidate for what? Rename to FocusCandidate?
> +static long long precedenceDistance(FocusDirection, IntRect&, IntRect&);
> +static long long distanceInDirection(Node*, Node*, FocusDirection,
BestCandidate&);
Do you really need long long for distance? All other WebCore positions are
floats or ints.
> +static void nextInDirection(Document*, Node*, FocusDirection,
KeyboardEvent*, BestCandidate&);
> +static bool isRectInDirection(FocusDirection, IntRect, IntRect);
> +static inline IntRect renderRect(RenderObject*);
> +static long long spatialDistance(FocusDirection, IntRect&, IntRect&);
> +static bool scrollInDirection(Frame*, FocusDirection);
> +static bool isOffscreenRect(Node*);
Should any of these be instance or class methods?
> +///////////////////////////////////////
> +// HELPER SPATIAL NAVIGATION METHODS //
> +///////////////////////////////////////
We don't normally add comments like this.
> +static void nextInDirection(Document* document, Node* start, FocusDirection
direction, KeyboardEvent* event, BestCandidate& bestCandidate)
I think this would be better named getNextFocusCandidate() or
fetchNextFocusCandidate().
> + if (innerDocument == start->document())
> + nextInDirection(innerDocument, start, direction, event,
bestCandidate);
> + else {
> + // Check if the current iframe (or frame) element itself is
a good candidate to move focus to.
> + // If it is, then we traverse its inner nodes.
> + // ps: lets pass a copy of the best candidate, to not get
fooled by a frame w/out
> + // focusable elements.
The "ps:" is weird. Just make it a normal sentence.
> + BestCandidate bestCandidateCopy = bestCandidate;
> + long long distance = distanceInDirection(start, candidate,
direction, bestCandidateCopy);
> + if (distance < bestCandidateCopy.distance) {
> + bestCandidateCopy.parentMode = bestCandidateCopy.mode;
> + bestCandidateCopy.parentDistance = distance;
> +
> + nextInDirection(innerDocument, start, direction, event,
bestCandidateCopy);
> +
> + // If we really have a inner best node candidate, take
it.
> + if (bestCandidate.node != bestCandidateCopy.node)
> + bestCandidate = bestCandidateCopy;
It seems like this "compare and set" logic might be better as a method on
BestCandidate.
> +static long long distanceInDirection(Node* start, Node* dest, FocusDirection
direction, BestCandidate& candidate)
> +{
> + long long distance = candidate.distance;
> +
> + IntRect startRect = IntRect();
> + IntRect destRect = IntRect();
> + RenderObject* startRender = 0;
> + RenderObject* destRender = 0;
> +
> + startRender = start->renderer();
> + if (!startRender)
> + goto exitPoint;
> +
> + destRender = dest->renderer();
> + if (!destRender)
> + goto exitPoint;
No gotos please.
> + startRect = renderRect(startRender);
> + destRect = renderRect(destRender);
Declare the IntRects here.
> + // The bounding rectangle of two consecutive links could overlap, so we
decrease the size
> + // of both rectangles.
> + startRect.inflate(-4);
> + destRect.inflate(-4);
These can cause the rect widths to go negative, which you don't want. Why the
special case for links?
> + if (takesSuperPrecedence(direction, startRect, destRect) ||
candidate.parentMode == SuperPrecedent) {
> + // reset distance as we are now in super precedence mode
Comments should use Sentence case.
Seems like more logic to push into BestCandidate, perhaps.
> +exitPoint:
> + return numeric_limits<long long>::max();
I think it would be clearer if you had a constant for numeric_limits<long
long>::max(), like cMaxDistance.
> +// FIXME: This function is flawed because it doesn't consider the effects of
CSS or SVG transforms.
It does take transforms into account; absoluteClippedOverflowRect() does the
right thing.
However, it does not behave correctly with transformed frames.
> +static inline IntRect renderRect(RenderObject* render)
The method should be renamed to indicate that it is returning a rect relative
to the root document.
> +{
> + ASSERT(render);
> +
> + Node* node = render->node();
> + Document* mainDocument =
node->document()->page()->mainFrame()->document();
> + bool useScrollOffset = !isOffscreenRect(node) && node->document() !=
mainDocument;
> +
> + IntRect rect(render->absoluteClippedOverflowRect());
> +
> + // There are cases, mostly when |render|'s associated node is offscreen,
> + // it is safier to get its position regardless the container scroll
offset.
Typo: "safier".
> + if (useScrollOffset)
> + if (FrameView* frameView = render->node()->document()->view())
> + rect.move(-frameView->scrollOffset());
> +
> + // handle nested frames.
> + for (Frame* frame = render->document()->frame(); frame; frame =
frame->tree()->parent())
> + if (HTMLFrameOwnerElement* ownerElement = frame->ownerElement())
> + rect.move(ownerElement->offsetLeft(),
ownerElement->offsetTop());
> +
> + return rect;
> +}
> +// -- grabbed from MICROB excepted by UP|DOWN logic.
What is MICROB? What is the license for this code?
> +static bool isRectInDirection(FocusDirection direction, IntRect startRect,
IntRect destRect)
Pass rects by const reference.
> + int middle = 0;
Declare this only where used.
> + switch (direction) {
> + case FocusDirectionLeft:
> + middle = destRect.x() + abs(destRect.width()) / 2;
> + return (middle < startRect.x());
> + case FocusDirectionRight:
> + middle = destRect.x() + abs(destRect.width()) / 2;
> + return (middle > startRect.x() + abs(startRect.width()));
> + case FocusDirectionUp :
> + return (destRect.y() < startRect.y());
> + case FocusDirectionDown:
> + return (destRect.y() + abs(destRect.height()) > startRect.y() +
abs(startRect.height()));
> + default:
> + ASSERT_NOT_REACHED();
This assert is not required, and prevents the compiler from warning about
unhandled values.
> +// Checks if |node| is in the visible area (viewport) of its container
document.
But that's backwards from the method behavior.
> +static bool isOffscreenRect(Node* node)
> +{
> + // Get the current viewport rect so we can check if bestCandidate's rect
> + // is visible before actually moving the focus to it. In case it is not
visible,
> + // we scroll in direction later on instead.
> + FrameView* frameView = node->document()->view();
> + if (!frameView)
> + return true;
> +
> + IntRect containerViewportRect = frameView->visibleContentRect();
> +
> + RenderObject* render = node->renderer();
> + if (!render)
> + return true;
> +
> + IntRect rect(render->absoluteClippedOverflowRect());
No need for 'rect' here.
> +// In a bottom-up way, this method tries to scroll |frame| in a given
direction
> +// |direction|, going up in the frame tree hierarchy in case it does not
succeed.
> +static bool scrollInDirection(Frame* frame, FocusDirection direction)
> +{
> + if (!frame)
> + return false;
> +
> + // Leave the focus on the same node but scroll.
> + ScrollDirection scrollDirection;
> +
> + switch (direction) {
> + case FocusDirectionLeft:
> + scrollDirection = ScrollLeft;
> + break;
> + case FocusDirectionRight:
> + scrollDirection = ScrollRight;
> + break;
> + case FocusDirectionUp:
> + scrollDirection = ScrollUp;
> + break;
> + case FocusDirectionDown:
> + scrollDirection = ScrollDown;
> + break;
> + default:
> + ASSERT_NOT_REACHED();
> + return false;
Remove default case.
> +// This method checks for very preferable rect to move focus to. In essence
targets
"very preferable"?
> +// whose middle falls between the top or bottom of the current rect.
This is not a complete sentence.
> +static bool takesSuperPrecedence(FocusDirection direction, IntRect&
startRect, IntRect& destRect)
You need a better term than "super precedence".
Rect params should be const.
> +{
> + if (direction == FocusDirectionLeft || direction == FocusDirectionRight)
{
> + int destMiddle = destRect.y() + abs(destRect.height()) / 2;
> + int destBottom = destRect.y() + abs(destRect.height());
> + int destEnd = destRect.x() + abs(destRect.width());
> +
> + int startBottom = startRect.y() + abs(startRect.height());
> + int startEnd = startRect.x() + abs(startRect.width());
We don't line up code like this.
> + return ((destRect.y() >= startRect.y() && destRect.y() <=
startBottom)
> + || (destMiddle >= startRect.y() && destMiddle <= startBottom)
> + || (destBottom >= startRect.y() && destBottom <= startBottom)
> + || (destRect.y() == startRect.y())
> + || (destBottom == startBottom)
> + || (destRect.y() <= startRect.y() && destBottom >=
startBottom));
Just one space after the ||.
> + return ((destMiddle >= startRect.x() && destMiddle <= startEnd)
> + || (destRect.x() >= startRect.x() && destRect.x() <= startEnd)
> + || (startRect.x() == destRect.x())
> + || (startEnd == destEnd)
> + || (destEnd >= startRect.x() && destEnd <= startEnd)
> + || (destRect.x() <= startRect.x() && destEnd >= startEnd));
Ditto.
> +static bool takesPrecedence(FocusDirection direction, IntRect& startRect,
IntRect& destRect)
Const rects.
> + if (direction == FocusDirectionLeft || direction == FocusDirectionRight)
{
> + int minY = startRect.y() /* - half */;
> + int maxY = startRect.y() + abs(startRect.height()) /* + half */;
> + int startBottom = startRect.y() + abs(startRect.height());
> +
> + int destMiddle = destRect.y() + abs(destRect.height()) / 2;
> + int destBottom = destRect.y() + abs(destRect.height());
> +
> + return ((destRect.y() >= minY && destRect.y() <= maxY)
> + || (destBottom >= minY && destBottom <= maxY)
> + || (destMiddle >= startRect.y() && destMiddle <= startBottom)
> + || (destRect.y() >= startRect.y() && destRect.y() <=
startBottom)
> + || (destBottom >= startRect.y() && destBottom <= startBottom)
> + || (destRect.y() <= startRect.y() && destBottom >=
startBottom));
Ditto.
> +// sqrt(((x1 - x2) ^ 2) + ((y1 - y2) ^ 2))
> +static long long precedenceDistance(FocusDirection direction, IntRect&
startRect, IntRect& destRect)
> +{
> + long long distance = numeric_limits<long long>::max();
> + long long sec = numeric_limits<long long>::max();
I don't understand 'sec'. Needs a better name.
> +
> + if (direction == FocusDirectionLeft) {
> + int startX = startRect.x();
> + int startY = startRect.y() + abs(startRect.height()) / 2;
> + int destX = destRect.x() + abs(destRect.width());
> + int destY = destRect.y() + abs(destRect.height()) / 2;
> +
> + long long p = ((startX - destX) * (startX - destX)) + ((startY -
destY) * (destY - destY));
> + sec = sqrt(p);
> +
> + p = ((startRect.x() - destRect.x()) * (startRect.x() -
destRect.x())) + ((startRect.y() - destRect.y()) * (startRect.y() -
destRect.y()));
> + distance = sqrt(p);
> +
> + if (sec < distance && startRect.y() == destRect.y())
> + distance = sec;
> + }
> +
> + if (direction == FocusDirectionRight) {
> + int startX = startRect.x() + abs(startRect.width());
> + int startY = startRect.y() + abs(startRect.height()) / 2;
> + int destX = destRect.x();
> + int destY = destRect.y() + abs(destRect.height()) / 2;
> +
> + long long p = ((destX - startX) * (destX - startX)) + ((destY -
startY) * (destY - startY));
> + sec = sqrt(p);
> +
> + p = ((destRect.x() - startRect.x()) * (destRect.x() -
startRect.x())) + ((destRect.y() - startRect.y()) * (destRect.y() -
startRect.y()));
> + distance = sqrt(p);
> +
> + if (sec < distance && startRect.y() == destRect.y())
> + distance = sec;
> + }
> +
> + if (direction == FocusDirectionUp) {
> + int startX = startRect.x() + abs(startRect.width()) / 2;
> + int startY = startRect.y();
> + int destX = destRect.x() + abs(destRect.width()) / 2;
> + int destY = destRect.y() + abs(destRect.height());
> +
> + long long p = ((startX - destX) * (startX - destX)) + ((startY -
destY) * (startY - destY));
> + sec = sqrt(p);
> +
> + p = ((startRect.x() - destRect.x()) * (startRect.x() -
destRect.x())) + ((startRect.y() - destRect.y()) * (startRect.y() -
destRect.y()));
> +
> + distance = sqrt(p);
> +
> + if (sec < distance && startRect.x() == destRect.x())
> + distance = sec;
> +
> + sec = (startRect.y() - destRect.y());
> +
> + if (sec < distance)
> + distance = sec;
> + }
> +
> + if (direction == FocusDirectionDown) {
> + // get middle X for current and target rects
> + int startX = startRect.x() + abs(startRect.width()) / 2;
> + int startY = startRect.y() + abs(startRect.height()) / 2;
> + int destX = destRect.x() + abs(destRect.width()) / 2;
> + int destY = destRect.y() + abs(destRect.height()) / 2;
> +
> + long long p = ((destX - startX) * (destX - startX)) + ((destY -
startY) * (destY - startY));
> + sec = sqrt(p);
> +
> + p = ((destRect.x() - startRect.x()) * (destRect.x() -
startRect.x())) + ((destRect.y() - startRect.y()) * (destRect.y() -
startRect.y()));
> + distance = sqrt(p);
> +
> + if (sec < distance && startRect.x() == destRect.x())
> + distance = sec;
> +
> + sec = (destRect.y() - startRect.y());
> +
> + if (sec < distance)
> + distance = sec;
> + }
Lots of repeated code here. Can you factor into a new method?
// namespace WebCore
> diff --git a/WebCore/page/FocusController.h b/WebCore/page/FocusController.h
> index d86408a..38c08c4 100644
> --- a/WebCore/page/FocusController.h
> +++ b/WebCore/page/FocusController.h
> @@ -58,6 +58,9 @@ namespace WebCore {
> bool isFocused() const { return m_isFocused; }
>
> private:
> + bool advanceFocusDirectionally(FocusDirection, KeyboardEvent*);
> + bool advanceFocusInDocumentOrder(FocusDirection, KeyboardEvent*,
bool initialFocus);
We try to avoid using boolean parameters because they make the code at the call
site hard to read.
> diff --git a/WebCore/page/Settings.h b/WebCore/page/Settings.h
> + bool isSpatialNavigationEnabled() { return
m_isSpatialNavigationEnabled; }
This method should be |const|.
r- for the MICROB licensing issue. I'd like to see one more round.
More information about the webkit-reviews
mailing list