[Webkit-unassigned] [Bug 116046] For keyboard users, activating a fragment URL should transfer focus and caret to the destination
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 4 21:43:16 PDT 2016
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>
More information about the webkit-unassigned
mailing list