[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

Ryosuke Niwa <rniwa at webkit.org> changed:

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

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

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

> Source/WebCore/dom/Document.cpp:3944
> +    

There's whitespace in this line.
Ditto for every blink line hereafter in this file.

> Source/WebCore/dom/Document.cpp:3969
> +        Node* nextNode = NodeTraversal::nextSkippingChildren(*node);

Why are we calling nextSkippingChildren here given we're moving to the container node in nodeChildrenWillBeRemoved already?

> Source/WebCore/dom/Document.cpp:4099
> +    // We should update m_focusNavigationStartingNode if it was removed.

This comment repeats what the code below says. Please remove it.

> Source/WebCore/dom/Document.cpp:4101
> +    for (Node* nodeToBeRemoved = container.firstChild(); nodeToBeRemoved; nodeToBeRemoved = nodeToBeRemoved->nextSibling()) {
> +        if (m_focusNavigationStartingNode.get() == nodeToBeRemoved) {

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.

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

Please use add a helper function instead of duplicating the code here.

> Source/WebCore/page/FrameView.cpp:2100
> +            

Nit: Superfluous blank line & whitespace.

> LayoutTests/fast/events/sequential-focus-navigation-starting-point-expected.txt:2
> +PASS document.activeElement.id is 'next'

This output doesn't tell us why document.activeElement.id should be "next".

> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:14
> +test1();
> +test2();
> +test3();
> +test4();

There's no need to spit into four different functions like this.

> 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'");

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.

-- 
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/20160425/ec7500c3/attachment.html>