For keyboard users, activating a fragment URL should transfer focus and caret to the destination

[James Craig]

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

This needs to work for full keyboard access (FKA) as well as VoiceOver, and should work with in-page links (href="#foo") as well as on page load of external links with hash fragments (href="./bar.html#baz"). The patch for bug 17450 only fixed the problem for one use case: VoiceOver users using VO navigation only, not Tab navigation. 

This should bring WebKit in-line with the expected behavior in both Gecko and Trident.

[Radar WebKit Bug Importer]

<rdar://problem/13876837>

[chris fleizach]

(In reply to comment #0)
> 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
> 
> This needs to work for full keyboard access (FKA) as well as VoiceOver, and should work with in-page links (href="#foo") as well as on page load of external links with hash fragments (href="./bar.html#baz"). The patch for bug 17450 only fixed the problem for one use case: VoiceOver users using VO navigation only, not Tab navigation. 

This https://bugs.webkit.org/show_bug.cgi?id=17450 should have fixed it for tab navigation. VoiceOver users have this functionality built into VoiceOver

> 
> This should bring WebKit in-line with the expected behavior in both Gecko and Trident.

[James Craig]

Probably uses a frag ID that does not strictly *focusable*… I’ll try to come up with some more test cases that exercise or break the existing implementation. Will need @name and @id variants of each of these. 

1. focusable elements.
2. non-focusable elements.
3. script-focusable elements (tabindex="-1")

Will also need in-page links and full-refresh fragment identifier links. 

I count at least twelve variants to test.

[James Craig]

*** Bug 23249 has been marked as a duplicate of this bug. ***

[James Craig]

related to bug 124877

[James Craig]

Created attachment 254942 [details]
Landing page for one-half of the externally linked test cases

[James Craig]

Created attachment 254943 [details]
Test case with in-page links and external links to previous attachment

[James Craig]

Created attachment 254944 [details]
Test case with in-page, external, and iframe links to first attachment

[James Craig]

*** Bug 141136 has been marked as a duplicate of this bug. ***

[James Craig]

Related Chromium patch:
https://bugs.chromium.org/p/chromium/issues/detail?id=454172

[James Craig]

This is officially in the specs now.

"Move the sequential focus navigation starting point to target."
https://html.spec.whatwg.org/multipage/browsers.html#scroll-to-fragid

[Nan Wang]

Created attachment 275757 [details]
Initial patch

Did it the Chromium way.

[Build Bot]

Comment on attachment 275757 [details]
Initial patch

Attachment 275757 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1107576

New failing tests:
fast/events/sequential-focus-navigation-starting-point.html

[Build Bot]

Created attachment 275759 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[chris fleizach]

Comment on attachment 275757 [details]
Initial patch

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

> Source/WebCore/dom/Document.cpp:3933
> +    return node && (is<HTMLIFrameElement>(*node) || is<HTMLHtmlElement>(*node) || is<HTMLDocument>(*node));

you could make this Node& node and dispense with the node check

> Source/WebCore/dom/Document.cpp:3964
> +        Node* node = &m_focusNavigationStartingPoint->startContainer();

you can probably leave this in & form

> LayoutTests/fast/events/sequential-focus-navigation-starting-point-expected.txt:6
> +PASS After removing a focused element from the documen tree, sequential focus navigation should start at a place where the focused element was. 

documen -> document

[Nan Wang]

Created attachment 275781 [details]
patch

- applied review comments
- ignored the new test on iOS

[Ryosuke Niwa]

Comment on attachment 275781 [details]
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.

[Ryosuke Niwa]

Comment on attachment 275781 [details]
patch

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

> Source/WebCore/ChangeLog:11
> +

Please add a URL to the relevant sections of the HTML specification.

[Nan Wang]

Comment on attachment 275781 [details]
patch

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

>> Source/WebCore/dom/Document.h:1479
>> +    RefPtr<Range> m_focusNavigationStartingPoint;
> 
> Why are we using Range for this?

I'm using Range so that we can tell if the node has been removed from document tree and easily pick the next node. Is there a way to tell if a node has been removed or is no longer valid?

[Ryosuke Niwa]

Comment on attachment 275781 [details]
patch

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

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

On my second look, this is really
https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point
so we should call it exactly that: sequentialFocusNavigationStartingPoint with that URL in a comment.

>>> Source/WebCore/dom/Document.h:1479
>>> +    RefPtr<Range> m_focusNavigationStartingPoint;
>> 
>> Why are we using Range for this?
> 
> I'm using Range so that we can tell if the node has been removed from document tree and easily pick the next node. Is there a way to tell if a node has been removed or is no longer valid?

Is that really necessary?  Where in the HTML spec does it say that we need to use the next node in such a case?

[Nan Wang]

(In reply to comment #20)
> Comment on attachment 275781 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275781&action=review
> 
> >> 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).
> 
> On my second look, this is really
> https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-
> navigation-starting-point
> so we should call it exactly that: sequentialFocusNavigationStartingPoint
> with that URL in a comment.
> 
> >>> Source/WebCore/dom/Document.h:1479
> >>> +    RefPtr<Range> m_focusNavigationStartingPoint;
> >> 
> >> Why are we using Range for this?
> > 
> > I'm using Range so that we can tell if the node has been removed from document tree and easily pick the next node. Is there a way to tell if a node has been removed or is no longer valid?
> 
> Is that really necessary?  Where in the HTML spec does it say that we need
> to use the next node in such a case?

I'm following this spec https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point
I think it's reasonable to have some fallback node in case the starting node is removed. We don't want to jump to the top of document, right?

[Ryosuke Niwa]

(In reply to comment #21)
>
> I'm following this spec
> https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-
> navigation-starting-point
> I think it's reasonable to have some fallback node in case the starting node
> is removed. We don't want to jump to the top of document, right?

Which part of the spec says how we should deal with a removed node?

[Nan Wang]

(In reply to comment #22)
> (In reply to comment #21)
> >
> > I'm following this spec
> > https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-
> > navigation-starting-point
> > I think it's reasonable to have some fallback node in case the starting node
> > is removed. We don't want to jump to the top of document, right?
> 
> Which part of the spec says how we should deal with a removed node?

I think there's only fixup rules for focusing. 
Chrome has this situation covered:
https://developers.google.com/web/updates/2016/03/focus-start-point?hl=en
So I thought it would be nice for us to have it too.

If you don't think it's necessary for us to handle this case based on the spec, I can just simply reset the starting node when it has been removed.

[Ryosuke Niwa]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > >
> > > I'm following this spec
> > > https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-
> > > navigation-starting-point
> > > I think it's reasonable to have some fallback node in case the starting node
> > > is removed. We don't want to jump to the top of document, right?
> > 
> > Which part of the spec says how we should deal with a removed node?
> 
> I think there's only fixup rules for focusing. 
> Chrome has this situation covered:
> https://developers.google.com/web/updates/2016/03/focus-start-point?hl=en
> So I thought it would be nice for us to have it too.
> 
> If you don't think it's necessary for us to handle this case based on the
> spec, I can just simply reset the starting node when it has been removed.

In general, we would like our behavior to be interoperable with other browsers. And the way to do that will be following standards as well as improving the standards themselves as needed.

Having some sort of fallback behavior seems useful here but that should probably be precisely specified in the relevant spec (HTML specification in this case) if it's not done so already.  As far as I read the section you mentioned, I can't see any mentioning of what happens when a node is removed.

[Ryosuke Niwa]

To put it differently, there are two orthogonal but related questions:
1. What does the spec says about when the node designated as the sequential navigation starting point is removed from the document.
2. What is the desirable behavior when in the aforementioned situation regardless of the spec'ed behavior.

[James Craig]

(In reply to comment #25)
> To put it differently, there are two orthogonal but related questions:
> 1. What does the spec says about when the node designated as the sequential
> navigation starting point is removed from the document.
> 2. What is the desirable behavior when in the aforementioned situation
> regardless of the spec'ed behavior.

Seems like the desirable behavior is to move the sequential navigation point after the previous sibling of the removed node.

[James Craig]

(In reply to comment #26)
> (In reply to comment #25)
> > To put it differently, there are two orthogonal but related questions:
> > 1. What does the spec says about when the node designated as the sequential
> > navigation starting point is removed from the document.
> > 2. What is the desirable behavior when in the aforementioned situation
> > regardless of the spec'ed behavior.
> 
> Seems like the desirable behavior is to move the sequential navigation point
> after the previous sibling of the removed node.

That would result in undesirable behavior if a focus was lost from a pseudo-dialog (commonly a div appended to the end of the document body). Ideally we'd want to go back to the last known sequential navigation point that remains in the DOM.

We could also try to retain a stack of previously visited sequential navigation points. That way when focus is "lost" we pop items off the stack until it returns an insertion point or node that remains in the DOM. In the case of the pseudo dialog, focus might return to the button that triggered the dialog, rather than to the end of the document body where the dialog had been.

[James Craig]

Somebody changed the title to be VoiceOver-specific. It's not. This is a general keyboard access bug that also affects VoiceOver.

[chris fleizach]

(In reply to comment #28)
> Somebody changed the title to be VoiceOver-specific. It's not. This is a
> general keyboard access bug that also affects VoiceOver.

Given the absence of spec guidance and that Chrome does the same, I think the current approach is good enough for the time being. 

I agree, James' edge case is a real thing, but before we go over board we should probably wait for the spec to state that clearly, otherwise there will be more cases that we haven't covered.

[Ryosuke Niwa]

(In reply to comment #29)
> (In reply to comment #28)
> > Somebody changed the title to be VoiceOver-specific. It's not. This is a
> > general keyboard access bug that also affects VoiceOver.
> 
> Given the absence of spec guidance and that Chrome does the same, I think
> the current approach is good enough for the time being.

We should still give the feedback to W3C/WHATWG about this.

I definitely would not like us to be using Range here because of the performance implications (it slows down DOM). It's not the right kind of object to be pointing at a Node either.

A better approach would be adding a RefPtr<Node>, and then updating it in Document::nodeWillBeRemoved and Document::nodeChildrenWillBeRemoved manually.

[Nan Wang]

Created attachment 276287 [details]
patch

Get rid of Range

[Ryosuke Niwa]

Comment on attachment 276287 [details]
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.

[Nan Wang]

Comment on attachment 276287 [details]
patch

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

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

Because in nodeWillBeRemoved we are moving to the previousSibling if possible. I think running ElementTraversal::previous on  nextSkippingChildren will cover both cases.
I just found a bug with this in such case: <div id="container"><input id="removed"><input><input></div>
After removing first input, next focused element will be the last input but we want the second one. I think we should use next instead of nextSkippingChildren.

>> Source/WebCore/dom/Document.cpp:4101
>> +        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.

Good point, will handle this case.

[Nan Wang]

Created attachment 277305 [details]
patch

Applied review comments

[Ryosuke Niwa]

Comment on attachment 277305 [details]
patch

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

> Source/WebCore/dom/Document.cpp:3922
> +    // Current behaivor is to move the sequential navigation node to/after(based on the focus direction)

Please add a space between "/", "after" and "(".

> Source/WebCore/dom/Document.cpp:3934
> +    if (is<Element>(*node))
> +        return downcast<Element>(node);

Step 2 checks of https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point says:
> If there is a sequential focus navigation starting point defined and it is inside starting point, then let starting point be the sequential focus navigation starting point instead.

Why aren't we checking that m_focusNavigationStartingNode is inside the starting point (m_focusedElement).

> Source/WebCore/dom/Document.cpp:3936
> +    if (Element* sibling = direction == FocusDirectionForward ? ElementTraversal::previous(*node) : ElementTraversal::next(*node))
> +        return sibling;

Isn't this backwards?

> Source/WebCore/dom/Document.cpp:4089
> +    // When removed node equals m_focusNavigationStartingNode, we move m_focusNavigationStartingNode
> +    // to the previous sibling if possible. Otherwise set it to the parent node.

This comment repeats what the code says. Please remove.
We only add *why* comments.

> Source/WebCore/dom/Document.cpp:4094
> +        m_focusNavigationStartingNodeIsRemoved = true;

We prefer early returns over else.
Please add a return and remove else clause below.

> Source/WebCore/dom/Document.cpp:4098
> +        Node* parentNode = m_focusNavigationStartingNode->parentNode();
> +        while (parentNode) {

We should use for loop instead.

> LayoutTests/fast/events/sequential-focus-navigation-starting-point.html:15
> +}
> +function focusDiv() { 

Please add a blank line between functions.

[Nan Wang]

Comment on attachment 277305 [details]
patch

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

Will address other issues.

>> Source/WebCore/dom/Document.cpp:3934
>> +        return downcast<Element>(node);
> 
> Step 2 checks of https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point says:

We always return the m_focusedElement when it's not null. m_focusableElement will be set to null if we are trying to focus on unfocusable element, and m_focusNavigationStartingPoint will take place.

>> Source/WebCore/dom/Document.cpp:3936
>> +        return sibling;
> 
> Isn't this backwards?

Here we are using the sibling as a search starting point to find the next focusable element. So when going forward, next element might be the one we want to focus on when hitting tab. We use previous here to make sure we won't skip the next Element. Same goes for the going backwards case.

[Ryosuke Niwa]

Comment on attachment 277305 [details]
patch

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

>>> Source/WebCore/dom/Document.cpp:3934
>>> +        return downcast<Element>(node);
>> 
>> Step 2 checks of https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point says:
> 
> We always return the m_focusedElement when it's not null. m_focusableElement will be set to null if we are trying to focus on unfocusable element, and m_focusNavigationStartingPoint will take place.

That seems wrong.  The spec says we should m_focusNavigationStartingNode if it's inside m_focusableElement even if m_focusableElement was not null.

[Nan Wang]

(In reply to comment #37)
> Comment on attachment 277305 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277305&action=review
> 
> >>> Source/WebCore/dom/Document.cpp:3934
> >>> +        return downcast<Element>(node);
> >> 
> >> Step 2 checks of https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point says:
> > 
> > We always return the m_focusedElement when it's not null. m_focusableElement will be set to null if we are trying to focus on unfocusable element, and m_focusNavigationStartingPoint will take place.
> 
> That seems wrong.  The spec says we should m_focusNavigationStartingNode if
> it's inside m_focusableElement even if m_focusableElement was not null.

Ok, will add this case as well.

[Nan Wang]

Created attachment 278175 [details]
patch

Updates from review

[Ryosuke Niwa]

Comment on attachment 278175 [details]
patch

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

> Source/WebCore/dom/Document.cpp:3941
> +    if (Element* sibling = direction == FocusDirectionForward ? ElementTraversal::previous(*node) : ElementTraversal::next(*node))
> +        return sibling;

I think we want to add a comment saying that sibling needs to be an element prior to the next focusable element.

> Source/WebCore/dom/Document.cpp:4085
> +    return node.previousSibling() ?: node.parentNode();

Use || instead.

> Source/WebCore/dom/Document.cpp:4101
> +    // We need to update m_focusNavigationStartingNode if its ancestor has been removed.

This comment is tautological. Remove it.

[Nan Wang]

(In reply to comment #40)
> > Source/WebCore/dom/Document.cpp:4085
> > +    return node.previousSibling() ?: node.parentNode();
> 
> Use || instead.
What do you mean by this? We need to return a Node object from this function.

[Ryosuke Niwa]

(In reply to comment #41)
> (In reply to comment #40)
> > > Source/WebCore/dom/Document.cpp:4085
> > > +    return node.previousSibling() ?: node.parentNode();
> > 
> > Use || instead.
> What do you mean by this? We need to return a Node object from this function.

Oh oops, you're right. Can't do that in C++. Please do:
node.previousSibling() ? node.previousSibling() : node.parentNode()
instead as that's what we do elsewhere in our codebase.

Alternatively,
if (node.previousSibling())
    return node.previousSibling();
return node.parentNode();

[Nan Wang]

Applied all the comments and committed.
http://trac.webkit.org/changeset/200479

[Ryan Haddad]

fast/events/sequential-focus-navigation-starting-point.html is crashing on El Capitan and Yosemite Release WK1:
<https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r200479%20(5964)/results.html>

[Nan Wang]

(In reply to comment #44)
> fast/events/sequential-focus-navigation-starting-point.html is crashing on
> El Capitan and Yosemite Release WK1:
> <https://build.webkit.org/results/
> Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r200479%20(5964)/results.html>

I think WK1 mouse event is handled differently? Should I just ignore this test on mac-wk1?

[Ryosuke Niwa]

(In reply to comment #45)
> (In reply to comment #44)
> > fast/events/sequential-focus-navigation-starting-point.html is crashing on
> > El Capitan and Yosemite Release WK1:
> > <https://build.webkit.org/results/
> > Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r200479%20(5964)/results.html>
> 
> I think WK1 mouse event is handled differently? Should I just ignore this
> test on mac-wk1?

We should fix the crash. It looks like there is a bug in the code I landed in http://trac.webkit.org/changeset/200464.

[Ryosuke Niwa]

Huh, I can't reproduce this crash on my machine :(

[Nan Wang]

Created attachment 280722 [details]
Patch after the crash fix

- Addressed comments in #40
- Fixed some conflicts

[Ryosuke Niwa]

Comment on attachment 280722 [details]
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.

[Nan Wang]

Comment on attachment 280722 [details]
Patch after the crash fix

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

>> Source/WebCore/dom/Document.cpp:3725
>> +    }
> 
> 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.

setFocusedElement(nullptr) will also reset the focusNavigationStartingNode. So that's why updateFocusNavigationStartingNodeWithNodeRemoval needs to be after this function.

[Nan Wang]

Created attachment 280787 [details]
patch

addressed comments

[Ryosuke Niwa]

Comment on attachment 280787 [details]
patch

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

> Source/WebCore/dom/Document.cpp:3707
> +static bool nodeInSubtree(Node* node, Node* container, bool amongChildrenOnly)

We should give this function a more descriptive name such as isNodeInSubtree.

> Source/WebCore/dom/Document.cpp:3729
>          setFocusedElement(nullptr, FocusDirectionNone, FocusRemovalEventsMode::DoNotDispatch);
> +        setFocusNavigationStartingNode(focusedElement);

We should a comment clarifying here why we need to set the focus navigation starting node to the focused element here
as well as why we need to call this function first before calling removeFocusNavigationNodeOfSubtree

> Source/WebCore/dom/Document.cpp:3912
> +    // Setting focus navigation starting node to the following nodes means that we should start
> +    // the search from the beginning of the document/frame.
> +    return is<HTMLIFrameElement>(node) || is<HTMLHtmlElement>(node) || is<HTMLDocument>(node);

This makes no sense to me. If a focus happens to be at an iframe,
why do we want to start at the beginning of a document which contains that iframe?

> Source/WebCore/dom/Document.cpp:3952
> +    // When the node was removed from the document tree. This case is not specified in the spec:
> +    // https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation-starting-point
> +    // Current behaivor is to move the sequential navigation node to / after (based on the focus direction)
> +    // the previous sibling of the removed node.
> +    if (m_focusNavigationStartingNodeIsRemoved) {
> +        Node* nextNode = NodeTraversal::next(*node);
> +        if (direction == FocusDirectionForward)
> +            return ElementTraversal::previous(*nextNode);
> +        if (is<Element>(*nextNode))
> +            return downcast<Element>(nextNode);
> +        return ElementTraversal::next(*nextNode);
> +    }

Why don't we do this in removeFocusNavigationNodeOfSubtree instead?

[Nan Wang]

Comment on attachment 280787 [details]
patch

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

Will address other issues.

>> Source/WebCore/dom/Document.cpp:3952
>> +    }
> 
> Why don't we do this in removeFocusNavigationNodeOfSubtree instead?

Because the direction is not available in removeFocusNavigationNodeOfSubtree.
Also in removeFocusNavigationNodeOfSubtree we just fallback to a node, in this function we are getting the valid Element from that node.

[Nan Wang]

Created attachment 280814 [details]
patch

more review comments.

[Ryosuke Niwa]

Comment on attachment 280814 [details]
patch

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

> Source/WebCore/dom/Document.cpp:3952
> +    // The sibling needs to be an element prior to the next focusable element.
> +    if (Element* sibling = direction == FocusDirectionForward ? ElementTraversal::previous(*node) : ElementTraversal::next(*node))

Why don't we declare a local variable with a descriptive name instead of adding a comment?
e.g. elementBeforeNextFocusableElement

> Source/WebCore/page/FrameView.cpp:2108
> +            // Set the focused element to nullptr here so that we can use the focus navigation starting node
> +            // to search for the next focus candidate.

I don't think this comment is necessary. Please remove.

[Nan Wang]

Addressed the final comments and committed.
http://trac.webkit.org/changeset/201832

[Patrick H. Lauke]

Possibly related to this bug: https://www.youtube.com/watch?v=LvlM8Z71oWE (issue involving URL with fragment identifier and VoiceOver - opening the URL initially visually scrolls to the correct place in the page, but then sets focus to the first focusable element at the start of the document, making URLs with fragment identifiers useless when running VoiceOver)

[David Kilzer (:ddkilzer)]

(In reply to comment #57)
> Possibly related to this bug: https://www.youtube.com/watch?v=LvlM8Z71oWE
> (issue involving URL with fragment identifier and VoiceOver - opening the
> URL initially visually scrolls to the correct place in the page, but then
> sets focus to the first focusable element at the start of the document,
> making URLs with fragment identifiers useless when running VoiceOver)

Please file a new bug with steps to reproduce, if possible.  Thanks!

[Patrick H. Lauke]

(In reply to comment #58)
> Please file a new bug with steps to reproduce, if possible.  Thanks!

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

[James Craig]

Since the most recent commit did not resolve all the issues. Refiling as bug 185264

[Simon Fraser (smfr)]

Comment on attachment 280814 [details]
patch

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

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

This can set the m_focusNavigationStartingNode to the Document itself, creating a ref cycle.

> Source/WebCore/dom/Document.cpp:4054
> +    removeFocusNavigationNodeOfSubtree(container, true /* amongChildrenOnly */);

There are two other callers of removeFocusedNodeOfSubtree(), in Element::removeShadowRoot() and FrameLoader::clear(). Do they also need to call removeFocusNavigationNodeOfSubtree()? Do we need a function that does both?

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #61)
> Comment on attachment 280814 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280814&action=review
> 
> > Source/WebCore/dom/Document.cpp:3724
> > +        setFocusNavigationStartingNode(focusedElement);
> 
> This can set the m_focusNavigationStartingNode to the Document itself,
> creating a ref cycle.

Tracked by bug 188722.