<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 - In Voice over, 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 #275781 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_ASSIGNED "
title="ASSIGNED - In Voice over, activating a fragment URL should transfer focus and caret to the destination"
href="https://bugs.webkit.org/show_bug.cgi?id=116046#c17">Comment # 17</a>
on <a class="bz_bug_link
bz_status_ASSIGNED "
title="ASSIGNED - In Voice over, 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=275781&action=diff" name="attach_275781" title="patch">attachment 275781</a> <a href="attachment.cgi?id=275781&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=275781&action=review">https://bugs.webkit.org/attachment.cgi?id=275781&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:3
> + FKA: activating a fragment ID URL (in-page link or external link with hash) should transfer focus and caret to the fragment target element with ID or name matching the fragment identifier</span >
I've shorted the bug title. Please update this line accordingly.
<span class="quote">> Source/WebCore/ChangeLog:8
> + Used a Range object to represent the sequential focus navigation starting point. When</span >
I don't think we should be using Range for this.
<span class="quote">> Source/WebCore/dom/Document.cpp:693
> + m_focusNavigationStartingPoint = nullptr;</span >
I'd call this focusNavigationStartingNode instead since "point" usually refers to either a graphical coordinate
or a single point in DOM tree (e.g. Position / VisiblePosition).
<span class="quote">> Source/WebCore/dom/Document.cpp:3754
> + // Set the focus starting point to the previous focused element.</span >
This comment repeats the code beneath it. Please remove it.
<span class="quote">> Source/WebCore/dom/Document.cpp:3952
> + if (!m_focusNavigationStartingPoint)
> + m_focusNavigationStartingPoint = Range::create(*this);
> +
> + ExceptionCode ec = 0;
> + m_focusNavigationStartingPoint->selectNodeContents(node, ec);
> + if (ec)
> + m_focusNavigationStartingPoint = nullptr;</span >
We should just store node instead.
r- because these function calls are expensive and setFocusedElement is a hot function in Speedometer benchmark.
<span class="quote">> Source/WebCore/dom/Document.h:1479
> + RefPtr<Range> m_focusNavigationStartingPoint;</span >
Why are we using Range for this?
<span class="quote">> LayoutTests/ChangeLog:3
> + FKA: activating a fragment ID URL (in-page link or external link with hash) should transfer focus and caret to the fragment target element with ID or name matching the fragment identifier</span >
Ditto about updating the title.
<span class="quote">> LayoutTests/fast/dom/fragment-activation-focuses-target.html:51
> -shouldBe("document.activeElement", "link2");
> +shouldBe("document.activeElement", "document.body");</span >
You should justify this change in the change log.
<span class="quote">> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:5
> +<script src="../../resources/testharness.js"></script>
> +<script src="../../resources/testharnessreport.js"></script>
> +<script src="../forms/resources/common.js"></script></span >
For these WebKit specific tests, we'd prefer using js-test-pre.js instead of testharness.js since testharness.js is only useful for tests to be exported to W3C.</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>