[webkit-reviews] review denied: [Bug 36463] Spatial Navigation: make it work with focusable elements in overflow content : [Attachment 56483] patch v5.1
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat May 22 13:54:07 PDT 2010
Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Antonio Gomes
(tonikitoo) <tonikitoo at webkit.org>'s request for review:
Bug 36463: Spatial Navigation: make it work with focusable elements in overflow
content
https://bugs.webkit.org/show_bug.cgi?id=36463
Attachment 56483: patch v5.1
https://bugs.webkit.org/attachment.cgi?id=56483&action=review
------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
Not must a review of the actual code, but there goes my comments.
> Patch addresses the problem that Spatial Navigation currently does not
properly
> traverse scrollable contents, including scrollable div's. For that to work, a
new
> class member called scrollableEnclosingBox was introduced to FocusCandidate
class
> to keep the track of current scrollable box Node wrapping a FocusCandidate.
To make
> use of enclosingScrollableBox of FocusCandidate, the DOM traversal algorithm
routine
> (FocusController::findNextFocusableInDirection) was changed as follows: At
the
> time it encounters a scrollable Node, each focusable node that is inner of it
keeps
> track of the container reference. By the time a sibling of the scrollable
Node in case
> is encountered, there is not need to track this reference any more, and the
traversal
> algorithm continues normally.
This is a bit hard reading; consider adding a few newlines where it logically
makes sense.
> + This test ensures the content overflow traversal correctness of Spatial
Navigation
No need to use capitals for Spatial Navigation.
> + algorithm: focusable elements in an scrollable containers (e.g. <div>)
should be
> + accessible, including offscreen content.
That doesn't sounds like an algorithm to me.
> +
> + * Pre-conditions:
> + 1) DRT support for SNav enable/disable.
Just write out spatial navigation.
> -// FIXME: Make this method more modular, and simpler to understand and
maintain.
> -static void updateFocusCandidateIfCloser(Node* focusedNode, const
FocusCandidate& candidate, FocusCandidate& closest)
> +static void updateFocusCandidateInTheSameContainer(const FocusCandidate&
candidate, FocusCandidate& closest)
I would leave out the The.
> + // ### !sameContainerAsCandidate && !sameContainerAsClosest
### what does that means? is that a FIXME: ?
> void FocusController::deepFindFocusableNodeInDirection(Node* container,
Node* focusedNode,
> FocusDirection
direction, KeyboardEvent* event,
> FocusCandidate&
closestFocusCandidate)
Do not add newlines in the function definition.
> + if (RenderObject* renderer = node->renderer())
> + return (renderer->isBox() &&
toRenderBox(renderer)->canBeScrolledAndHasScrollableArea()
> + && node->hasChildNodes() && !node->isDocumentNode());
Code style issue, you will need braces here: the return spans two real lines.
> + bool isInScrollableContainer() const { return node &&
enclosingScrollableBox; }
Please check if other "isIn" methods exist, or they whether they just use "in"
More information about the webkit-reviews
mailing list