<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><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> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #276287 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<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#c32">Comment # 32</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=276287&action=diff" name="attach_276287" title="patch">attachment 276287</a> <a href="attachment.cgi?id=276287&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=276287&action=review">https://bugs.webkit.org/attachment.cgi?id=276287&action=review</a>
<span class="quote">> Source/WebCore/dom/Document.cpp:3944
> + </span >
There's whitespace in this line.
Ditto for every blink line hereafter in this file.
<span class="quote">> Source/WebCore/dom/Document.cpp:3969
> + Node* nextNode = NodeTraversal::nextSkippingChildren(*node);</span >
Why are we calling nextSkippingChildren here given we're moving to the container node in nodeChildrenWillBeRemoved already?
<span class="quote">> Source/WebCore/dom/Document.cpp:4099
> + // We should update m_focusNavigationStartingNode if it was removed.</span >
This comment repeats what the code below says. Please remove it.
<span class="quote">> Source/WebCore/dom/Document.cpp:4101
> + for (Node* nodeToBeRemoved = container.firstChild(); nodeToBeRemoved; nodeToBeRemoved = nodeToBeRemoved->nextSibling()) {
> + if (m_focusNavigationStartingNode.get() == nodeToBeRemoved) {</span >
This is a very inefficient way of checking that m_focusNavigationStartingNode is getting removed.
Instead, just check "m_focusNavigationStartingNode && m_focusNavigationStartingNode->parentNode() == this".
Also, this code has a bug that it doesn't check the possibility that an ancestor of m_focusNavigationStartingNode is getting removed.
e.g. m_focusNavigationStartingNode is at the span in <div><span></span></div> and div is getting removed.
So we need to walk up the ancestor chain of m_focusNavigationStartingNode and check whether any of it matches this.
r- because of this bug.
Also, please add a test case for this.
<span class="quote">> Source/WebCore/dom/Document.cpp:4131
> + // We should update m_focusNavigationStartingNode if it was removed.
> + if (m_focusNavigationStartingNode.get() == &n) {
> + m_focusNavigationStartingNode = n.previousSibling();
> + if (!m_focusNavigationStartingNode)
> + m_focusNavigationStartingNode = n.parentNode();
> + m_focusNavigationStartingNodeIsRemoved = true;
> + }</span >
Please use add a helper function instead of duplicating the code here.
<span class="quote">> Source/WebCore/page/FrameView.cpp:2100
> + </span >
Nit: Superfluous blank line & whitespace.
<span class="quote">> LayoutTests/fast/events/sequential-focus-navigation-starting-point-expected.txt:2
> +PASS document.activeElement.id is 'next'</span >
This output doesn't tell us why document.activeElement.id should be "next".
<span class="quote">> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:14
> +test1();
> +test2();
> +test3();
> +test4();</span >
There's no need to spit into four different functions like this.
<span class="quote">> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:23
> + container.innerHTML = '<input id=prev><div style="height:200px;"><span>text</span></div><input id=next>';
> + hoverOverElement(container.querySelector('span'));
> + eventSender.mouseDown();
> + eventSender.keyDown('\t');
> + shouldBe("document.activeElement.id", "'next'");</span >
This code can be improved as follows:
shouldBe("container.innerHTML = '<input id=prev><div style="height:200px;"><span>text</span></div><input id=next>'; focusSpan(); moveFocus('forward'); document.activeElement.id", "'next'");
where
function focusSpan() { hoverOverElement(container.querySelector('span')); }
function moveFocus(direction) { eventSender.keyDown('\t', direction == 'forward' ? [] : ['shiftKey']); }
This way, the expected result contains the markup as well as the code that runs prior to the assertion
and makes it blatantly obvious why activeElement.id should be 'next'.
Ditto for other test cases.</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>