[Webkit-unassigned] [Bug 116046] For keyboard users, activating a fragment URL should transfer focus and caret to the destination

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116046

--- Comment #35 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 277305
  --> https://bugs.webkit.org/attachment.cgi?id=277305
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277305&action=review

> Source/WebCore/dom/Document.cpp:3922
> +    // Current behaivor is to move the sequential navigation node to/after(based on the focus direction)

Please add a space between "/", "after" and "(".

> Source/WebCore/dom/Document.cpp:3934
> +    if (is<Element>(*node))
> +        return downcast<Element>(node);

Step 2 checks of https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point says:
> If there is a sequential focus navigation starting point defined and it is inside starting point, then let starting point be the sequential focus navigation starting point instead.

Why aren't we checking that m_focusNavigationStartingNode is inside the starting point (m_focusedElement).

> Source/WebCore/dom/Document.cpp:3936
> +    if (Element* sibling = direction == FocusDirectionForward ? ElementTraversal::previous(*node) : ElementTraversal::next(*node))
> +        return sibling;

Isn't this backwards?

> Source/WebCore/dom/Document.cpp:4089
> +    // When removed node equals m_focusNavigationStartingNode, we move m_focusNavigationStartingNode
> +    // to the previous sibling if possible. Otherwise set it to the parent node.

This comment repeats what the code says. Please remove.
We only add *why* comments.

> Source/WebCore/dom/Document.cpp:4094
> +        m_focusNavigationStartingNodeIsRemoved = true;

We prefer early returns over else.
Please add a return and remove else clause below.

> Source/WebCore/dom/Document.cpp:4098
> +        Node* parentNode = m_focusNavigationStartingNode->parentNode();
> +        while (parentNode) {

We should use for loop instead.

> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:15
> +}
> +function focusDiv() { 

Please add a blank line between functions.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160505/3808a1d9/attachment.html>