[Webkit-unassigned] [Bug 116046] For keyboard users, 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

--- Comment #49 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 280722
  --> https://bugs.webkit.org/attachment.cgi?id=280722
Patch after the crash fix

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

> Source/WebCore/ChangeLog:11
> +

Please add a URL to the spec.

> Source/WebCore/dom/Document.cpp:3725
> +        setFocusNavigationStartingNode(focusedElement);
> +    }

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.

> Source/WebCore/dom/Document.cpp:3914
> +    if (!node || isNodeFrameOrDocument(*node)) {

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.

> Source/WebCore/dom/Document.cpp:4114
> +void Document::updateFocusNavigationStartingNodeWithNodeRemoval(Node& node, bool removeChildren)

This doesn't match the naming scheme of similar functions.
How about calling it removeFocusNavigationNodeOfSubtree instead?

> Source/WebCore/dom/Document.cpp:4121
> +        if (removeChildren)
> +            return;

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.

> Source/WebCore/dom/Document.cpp:4128
> +    for (Node* parentNode = m_focusNavigationStartingNode->parentNode(); parentNode; parentNode = parentNode->parentNode()) {
> +        if (parentNode == &node) {

This loop seems completely unnecessary.  We just need to check isDescendantOf instead.

> Source/WebCore/page/FrameView.cpp:2107
> +            document.setFocusedElement(nullptr);

Why are we now setting the focusable element to nullptr?
This should be explained in the change log.

> LayoutTests/ChangeLog:7
> +

Please add a description as to what kind of test changes you're making as well as kind of tests you're adding.

> LayoutTests/ChangeLog:9
> +        * fast/dom/fragment-activation-focuses-target-expected.txt:
> +        * fast/dom/fragment-activation-focuses-target.html:

Please explain a change to this test.

-- 
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/20160607/98ff7bed/attachment.html>