1519090 - keyboard focus is trapped inside <slot>

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[cronon12]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36 OPR/57.0.3098.106

Steps to reproduce:

Version: 64.0
Build ID: 20181207224003
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:64.0) Gecko/20100101 Firefox/64.0

1. Open https://codepen.io/anon/pen/XoPORj
2. Click on white space
3. Press tab key few times

Actual results:

Focus starts moving only between buttons inside the <slot> tag.

Expected results:

Focus should go through both "inside" and "outside" buttons

Comment 1

[Daniel Bodea [:danibodea]]

This issue occurs as described on Windows, Mac OS and Ubuntu on all three main versions of Firefox.
Component chosen is Keyboard Navigation.
Thank you for your contribution!

Comment 2

[Edgar Chen [:edgar]]

It looks like we don't handle focus navigation with unassigned slot well.

Comment 3

[Edgar Chen [:edgar]]

(In reply to cronon12 from comment #0)

Focus starts moving only between buttons inside the <slot> tag.

It is because the FindOwner [1] call of "inside" button returns <slot>, and then GetNextTabbableContentInAncestorScopes redirects startContent to <slot> given that it could not find next tabbable content, then the following frame/tree traversa [3]l starts from <slot> and stop at the first "inside" button again.

The owner of "inside" button should not be the <slot> and we should not run GetNextTabbableContentInAncestorScopes either since there is no shadow dom involved at all...

[1] https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/dom/base/nsFocusManager.cpp#2952
[2] https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/dom/base/nsFocusManager.cpp#3211
[3] https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/dom/base/nsFocusManager.cpp#3233-3267

Comment 4

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #3)

The owner of "inside" button should not be the <slot> and we should not run GetNextTabbableContentInAncestorScopes either since there is no shadow dom involved at all...

I was wrong, per spec, slot element is a also a scope owner even if it is not in shadow dom or has no assigned nods. So the issue here is that in some cases we only check shadow host, but we should also check slot as well.

Comment 5

[Edgar Chen [:edgar]]

Attachment: Bug 1519090 - Part 1: Some minor nsFocusManager cleanup;

Comment 6

[Edgar Chen [:edgar]]

Attachment: Bug 1519090 - Part 2: Skip the node under slot scope in frame traversal;

The nodes under slot scope is already being traversed in
GetNextTabbableContentIn*Scope.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Bug 1519090 - Part 3: nsFocusManager::GetNextTabbableContentInScope should also check slot if aSkipOwner is false;

Slot element could be also focusable.

Comment 8

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=293c3f5f7f95a66b2f4c8b7c66627b2680154531

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12ffc4e92ff6
Part 1: Some minor nsFocusManager cleanup; r=smaug
https://hg.mozilla.org/integration/autoland/rev/48f01f2f5573
Part 2: Skip the node under slot scope in frame traversal; r=smaug
https://hg.mozilla.org/integration/autoland/rev/4e227c741b4b
Part 3: nsFocusManager::GetNextTabbableContentInScope should also check slot if aSkipOwner is false; r=smaug

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/12ffc4e92ff6
https://hg.mozilla.org/mozilla-central/rev/48f01f2f5573
https://hg.mozilla.org/mozilla-central/rev/4e227c741b4b

Comment 11

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/cedar/rev/12ffc4e92ff6a552e36d5367abe11f79f458bdb8
Bug 1519090 - Part 1: Some minor nsFocusManager cleanup; r=smaug

https://hg.mozilla.org/projects/cedar/rev/48f01f2f5573921e1f02617ebc9c5426eaef9973
Bug 1519090 - Part 2: Skip the node under slot scope in frame traversal; r=smaug

https://hg.mozilla.org/projects/cedar/rev/4e227c741b4bb4ebb15d869a02acb83c1a656a54
Bug 1519090 - Part 3: nsFocusManager::GetNextTabbableContentInScope should also check slot if aSkipOwner is false; r=smaug

Comment 12

[Daniel Bodea [:danibodea]]

This issue is now verified in Nightly v66.0a1 from 2019-01-23 on Windows 10, Ubuntu 16 and Mac OS 10.14.2.

Thanks!