[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]

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"