[Webkit-unassigned] [Bug 116046] In Voice over, activating a fragment URL should transfer focus and caret to the destination

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116046

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #275781|review?                     |review-
              Flags|                            |

--- Comment #17 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 275781
  --> https://bugs.webkit.org/attachment.cgi?id=275781
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275781&action=review

> 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

I've shorted the bug title.  Please update this line accordingly.

> Source/WebCore/ChangeLog:8
> +        Used a Range object to represent the sequential focus navigation starting point. When

I don't think we should be using Range for this.

> Source/WebCore/dom/Document.cpp:693
> +        m_focusNavigationStartingPoint = nullptr;

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).

> Source/WebCore/dom/Document.cpp:3754
> +        // Set the focus starting point to the previous focused element.

This comment repeats the code beneath it.  Please remove it.

> 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;

We should just store node instead.
r- because these function calls are expensive and setFocusedElement is a hot function in Speedometer benchmark.

> Source/WebCore/dom/Document.h:1479
> +    RefPtr<Range> m_focusNavigationStartingPoint;

Why are we using Range for this?

> 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

Ditto about updating the title.

> LayoutTests/fast/dom/fragment-activation-focuses-target.html:51
> -shouldBe("document.activeElement", "link2");
> +shouldBe("document.activeElement", "document.body");

You should justify this change in the change log.

> 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>

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.

-- 
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/20160409/5ebd13ab/attachment-0001.html>