<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_ASSIGNED "
title="ASSIGNED - For keyboard users, activating a fragment URL should transfer focus and caret to the destination"
href="https://bugs.webkit.org/show_bug.cgi?id=116046#c35">Comment # 35</a>
on <a class="bz_bug_link
bz_status_ASSIGNED "
title="ASSIGNED - For keyboard users, activating a fragment URL should transfer focus and caret to the destination"
href="https://bugs.webkit.org/show_bug.cgi?id=116046">bug 116046</a>
from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=277305&action=diff" name="attach_277305" title="patch">attachment 277305</a> <a href="attachment.cgi?id=277305&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=277305&action=review">https://bugs.webkit.org/attachment.cgi?id=277305&action=review</a>
<span class="quote">> Source/WebCore/dom/Document.cpp:3922
> + // Current behaivor is to move the sequential navigation node to/after(based on the focus direction)</span >
Please add a space between "/", "after" and "(".
<span class="quote">> Source/WebCore/dom/Document.cpp:3934
> + if (is<Element>(*node))
> + return downcast<Element>(node);</span >
Step 2 checks of <a href="https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point">https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point</a> says:
<span class="quote">> 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.</span >
Why aren't we checking that m_focusNavigationStartingNode is inside the starting point (m_focusedElement).
<span class="quote">> Source/WebCore/dom/Document.cpp:3936
> + if (Element* sibling = direction == FocusDirectionForward ? ElementTraversal::previous(*node) : ElementTraversal::next(*node))
> + return sibling;</span >
Isn't this backwards?
<span class="quote">> 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.</span >
This comment repeats what the code says. Please remove.
We only add *why* comments.
<span class="quote">> Source/WebCore/dom/Document.cpp:4094
> + m_focusNavigationStartingNodeIsRemoved = true;</span >
We prefer early returns over else.
Please add a return and remove else clause below.
<span class="quote">> Source/WebCore/dom/Document.cpp:4098
> + Node* parentNode = m_focusNavigationStartingNode->parentNode();
> + while (parentNode) {</span >
We should use for loop instead.
<span class="quote">> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:15
> +}
> +function focusDiv() { </span >
Please add a blank line between functions.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>