Focus unfocusable node upon reference fragment navigation

Focus unfocusable node upon reference fragment navigation

Currently, when the page navigates to a non-focusable reference fragment,
the currently focused element does not change. In Firefox/IE, the focus is
moved to the referenced element even when this element is non-focusable.

This mismatch in behavior became apparent when Blink started supporting
a:focus (https://crbug.com/388666). In IE/FF, clicking on <a href="#ref">
causes the link to loose focus, and therefore :focus does not get applied
any more. In Chrome, however, :focus remains activated because the focus
does not change.

This patch changes the behavior of Blink: When the pages navigates to a
reference fragment that refers to a non-focusable node, then the focus
is removed from the previously focused element.

See also https://www.w3.org/Bugs/Public/show_bug.cgi?id=26907.

BUG=417636, 262171
TEST=Tools/Scripts/run-webkit-tests fast/dom/fragment-activation-focuses-target.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183455

[robwu, 2014-10-08 10:12:58]

rob@robwu.nl changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-08 10:21:20]

Can you add some text to the CL description outlining the change you're making
and the impact it will have on the web? I read through the bug, but it's not
clear to me which of the several options you've outlined you ended up choosing.

[robwu, 2014-10-08 10:49:37]

On 2014/10/08 10:21:20, Mike West wrote:
> Can you add some text to the CL description outlining the change you're making
> and the impact it will have on the web? I read through the bug, but it's not
> clear to me which of the several options you've outlined you ended up
choosing.

Hi Mike, I've edited the CL description.

[Mike West, 2014-10-08 11:05:42]

mkwst@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Mike West, 2014-10-08 11:05:43]

+dmazzoni

This change LGTM, but please wait for Dominic to take a look at the
accessibility impacts.

[Mike West, 2014-10-08 11:08:14]

On 2014/10/08 11:05:43, Mike West wrote:
> +dmazzoni
> 
> This change LGTM, but please wait for Dominic to take a look at the
> accessibility impacts.

(I think it's a positive change, but I'd like him to sign off on it)

[robwu, 2014-10-08 15:47:03]

Patchset #2 (id:20001) has been deleted

[robwu, 2014-10-08 16:10:20]

Patchset #3 (id:60001) has been deleted

[robwu, 2014-10-08 16:32:24]

I've removed the test and comment about accessibility (sequential focus
navigation (SFN) as defined at
https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navi...),
because its implementation turns out to be more complicated than expected.

Blink stores and maintains a pointer to the active element. When we try to focus
a non-focusable element, then the pointer is null'd. Consequently the SFN
algorithm starts at the top of the page instead of the non-focusable element
(like Firefox). IE behaves in the same way as this patch, as demonstrated by
http://jsfiddle.net/o5bkzx0s/1/. When the non-focusable node gets activated, SFN
continues at the top of the page (just click on the button "go to #x" and press
tab. The first <input> should get focused (IE+Blink), Firefox continues at the
button instead).

If we want to take non-focusable nodes into account in the SFN algorithm (like
Firefox), then we have to store another pointer to the node that is used for
SFN. I've not taken this path because maintaining this pointer is complex (e.g.
node removal, refocusing, unfocusing, style recalculation when a saved
non-focusable element gets the ability to be focused by changing tabIndex,
etc.).

Mike: Could you review my changes?

[dmazzoni, 2014-10-08 18:08:29]

I think this is bug 262171, right? That seems like a better primary bug for this
change; the other one you linked seems somewhat tangential.

[dmazzoni, 2014-10-08 18:19:06]

First of all, thank you for looking into this issue. I would very much like to
see it fixed, but I'm not convinced this is the right fix.

I believe what your change does is set focus to an anchor node that isn't
actually focusable. That means that document.activeElement would return that
element - and I don't think that's consistent with what other browsers do. I
could be wrong, but I believe that other browsers don't actually focus the node,
they just use that node when computing the *next* focused element.

Try this html snippet - note that if you follow the link to one of the anchors,
the active element is the body - but if you press Tab, it continues from the
position of the anchor. That's what we want.

If I'm wrong and that's how Chrome behaves already with this change, then maybe
this is pretty close. Let me know.

<a href="#1">Go to 1</a> |
<a href="#2">Go to 2</a> |
<a href="#3">Go to 3</a>
<hr>
<div id="a" style="border: 1px solid #000; padding: 20px;"></div>
<hr>
<a name="1">
<h2>Section 1</h2>
<button>Foo</button><button>Bar</button><button>Baz</button>
<div style="height: 400px;"></div>
<a name="2">
<h2>Section 2</h2>
<button>Foo</button><button>Bar</button><button>Baz</button>
<div style="height: 400px;"></div>
<a name="3">
<h2>Section 3</h2>
<button>Foo</button><button>Bar</button><button>Baz</button>
<div style="height: 400px;"></div>
<script>
setInterval(function() {
  document.getElementById('a').textContent = 'Active element: ' + document.acti\
veElement.outerHTML.substr(0, 20);
}, 1000);
</script>

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1365: // If it does not accept keyboard focus,
then still remove the focus for consistency with other browsers
(http://crbug.com/417636)
This comment doesn't make sense - you say "remove the focus", but your change
seems to *set* the focus to the anchor node.

We don't normally link to bugs in comments unless it's an open bug or temporary
hack.

[robwu, 2014-10-08 20:10:23]

2014/10/08 18:08:29, dmazzoni wrote:
> I think this is bug 262171, right? That seems like a better primary bug for
this
> change; the other one you linked seems somewhat tangential.

The bug in the CL description is correct. I initially thought that it would also
be an easy way to fix the bug you mentioned, but as explained in my previous
comment, it turns out to be more complicated (hence I took the test and comment
out in patch set 3.

I elaborated on the potential way of fixing your bug because I had put some
thought in the feature before scrapping the feature from this CL.

> Try this html snippet - note that if you follow the link to one of the
anchors,
> the active element is the body - but if you press Tab, it continues from the
> position of the anchor. That's what we want.

This is the behavior in Firefox as well, but not IE. I agree that it is
desirable behavior, but it is not the objective of this CL.

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1365: // If it does not accept keyboard focus,
then still remove the focus for consistency with other browsers
(http://crbug.com/417636)
On 2014/10/08 18:19:06, dmazzoni wrote:
> This comment doesn't make sense - you say "remove the focus", but your change
> seems to *set* the focus to the anchor node.

setFocusedElement first unfocuses the old element. Then, iff the node argument
is focusable, then the node will be used as the new focused element. Otherwise
the node will not be stored, see
https://chromium.googlesource.com/chromium/blink/+/31dab16c269e2de356a920e5d2...

> We don't normally link to bugs in comments unless it's an open bug or
temporary
> hack.

I'll remove the link at the next patchset.

[dmazzoni, 2014-10-08 20:26:28]

lgtm

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1365: // If it does not accept keyboard focus,
then still remove the focus for consistency with other browsers
(http://crbug.com/417636)
On 2014/10/08 20:10:23, robwu wrote:
> On 2014/10/08 18:19:06, dmazzoni wrote:
> > This comment doesn't make sense - you say "remove the focus", but your
change
> > seems to *set* the focus to the anchor node.
> 
> setFocusedElement first unfocuses the old element. Then, iff the node argument
> is focusable, then the node will be used as the new focused element. Otherwise
> the node will not be stored, see

Oh, I get it now. I'd probably write the comment something like this:

// If anchorNode is not focusable, setFocusedElement() will still clear focus,
which matches the behavior of other browsers.

[robwu, 2014-10-09 06:12:44]

The CQ bit was checked by rob@robwu.nl

[robwu, 2014-10-09 06:12:57]

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/637323002/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1365: // If it does not accept keyboard focus,
then still remove the focus for consistency with other browsers
(http://crbug.com/417636)
On 2014/10/08 20:26:27, dmazzoni wrote:
> On 2014/10/08 20:10:23, robwu wrote:
> > On 2014/10/08 18:19:06, dmazzoni wrote:
> > > This comment doesn't make sense - you say "remove the focus", but your
> change
> > > seems to *set* the focus to the anchor node.
> > 
> > setFocusedElement first unfocuses the old element. Then, iff the node
argument
> > is focusable, then the node will be used as the new focused element.
Otherwise
> > the node will not be stored, see
> 
> Oh, I get it now. I'd probably write the comment something like this:
> 
> // If anchorNode is not focusable, setFocusedElement() will still clear focus,
> which matches the behavior of other browsers.

Done.

[commit-bot: I haz the power, 2014-10-09 06:13:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637323002/100001

[commit-bot: I haz the power, 2014-10-09 07:22:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-09 07:22:21]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[robwu, 2014-10-09 09:01:32]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-10-09 09:01:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637323002/120001

[commit-bot: I haz the power, 2014-10-09 11:30:41]

Committed patchset #5 (id:120001) as 183455