<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#c49">Comment # 49</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=280722&action=diff" name="attach_280722" title="Patch after the crash fix">attachment 280722</a> <a href="attachment.cgi?id=280722&action=edit" title="Patch after the crash fix">[details]</a></span>
Patch after the crash fix
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=280722&action=review">https://bugs.webkit.org/attachment.cgi?id=280722&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:11
> +</span >
Please add a URL to the spec.
<span class="quote">> Source/WebCore/dom/Document.cpp:3725
> + setFocusNavigationStartingNode(focusedElement);
> + }</span >
It seems that we're storing the focus navigation starting node here so that we can move it to somewhere else in
updateFocusNavigationStartingNodeWithNodeRemoval later.
I think it's better and more efficient to call updateFocusNavigationStartingNodeWithNodeRemoval before this function
in Document::nodeChildrenWillBeRemoved and Document::nodeWillBeRemoved instead.
<span class="quote">> Source/WebCore/dom/Document.cpp:3914
> + if (!node || isNodeFrameOrDocument(*node)) {</span >
Why is an iframe, a HTML element, or a HTML document special? This needs to be explained in the change log.
Also, if there is any semantic reason as to why they need to be treated differently from other nodes,
then isNodeFrameOrDocument should be renamed to reflect that semantics.
<span class="quote">> Source/WebCore/dom/Document.cpp:4114
> +void Document::updateFocusNavigationStartingNodeWithNodeRemoval(Node& node, bool removeChildren)</span >
This doesn't match the naming scheme of similar functions.
How about calling it removeFocusNavigationNodeOfSubtree instead?
<span class="quote">> Source/WebCore/dom/Document.cpp:4121
> + if (removeChildren)
> + return;</span >
Please extract the code in removeFocusedNodeOfSubtree for branching on removeChildren as a helper function
and use it here instead of re-implementing in a different way.
That way, if someone ends up updating removeFocusedNodeOfSubtree in the future, then we wouldn't forget to update this one.
<span class="quote">> Source/WebCore/dom/Document.cpp:4128
> + for (Node* parentNode = m_focusNavigationStartingNode->parentNode(); parentNode; parentNode = parentNode->parentNode()) {
> + if (parentNode == &node) {</span >
This loop seems completely unnecessary. We just need to check isDescendantOf instead.
<span class="quote">> Source/WebCore/page/FrameView.cpp:2107
> + document.setFocusedElement(nullptr);</span >
Why are we now setting the focusable element to nullptr?
This should be explained in the change log.
<span class="quote">> LayoutTests/ChangeLog:7
> +</span >
Please add a description as to what kind of test changes you're making as well as kind of tests you're adding.
<span class="quote">> LayoutTests/ChangeLog:9
> + * fast/dom/fragment-activation-focuses-target-expected.txt:
> + * fast/dom/fragment-activation-focuses-target.html:</span >
Please explain a change to this test.</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>