<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#c52">Comment # 52</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=280787&action=diff" name="attach_280787" title="patch">attachment 280787</a> <a href="attachment.cgi?id=280787&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=280787&action=review">https://bugs.webkit.org/attachment.cgi?id=280787&action=review</a>
<span class="quote">> Source/WebCore/dom/Document.cpp:3707
> +static bool nodeInSubtree(Node* node, Node* container, bool amongChildrenOnly)</span >
We should give this function a more descriptive name such as isNodeInSubtree.
<span class="quote">> Source/WebCore/dom/Document.cpp:3729
> setFocusedElement(nullptr, FocusDirectionNone, FocusRemovalEventsMode::DoNotDispatch);
> + setFocusNavigationStartingNode(focusedElement);</span >
We should a comment clarifying here why we need to set the focus navigation starting node to the focused element here
as well as why we need to call this function first before calling removeFocusNavigationNodeOfSubtree
<span class="quote">> Source/WebCore/dom/Document.cpp:3912
> + // Setting focus navigation starting node to the following nodes means that we should start
> + // the search from the beginning of the document/frame.
> + return is<HTMLIFrameElement>(node) || is<HTMLHtmlElement>(node) || is<HTMLDocument>(node);</span >
This makes no sense to me. If a focus happens to be at an iframe,
why do we want to start at the beginning of a document which contains that iframe?
<span class="quote">> Source/WebCore/dom/Document.cpp:3952
> + // When the node was removed from the document tree. This case is not specified in the spec:
> + // <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>
> + // Current behaivor is to move the sequential navigation node to / after (based on the focus direction)
> + // the previous sibling of the removed node.
> + if (m_focusNavigationStartingNodeIsRemoved) {
> + Node* nextNode = NodeTraversal::next(*node);
> + if (direction == FocusDirectionForward)
> + return ElementTraversal::previous(*nextNode);
> + if (is<Element>(*nextNode))
> + return downcast<Element>(nextNode);
> + return ElementTraversal::next(*nextNode);
> + }</span >
Why don't we do this in removeFocusNavigationNodeOfSubtree instead?</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>