[FIX] Focus trap into webcomponents with shadowDOM

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
9 months ago
a month ago

People

(Reporter: xaviermd, Assigned: smaug, NeedInfo)

Tracking

({nightly-community})

63 Branch
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180716100102

Steps to reproduce:

- Go on https://dascritch.net/vrac/.projets/audiowc/index.html (NOTE : <import> polyfilled) or download https://github.com/dascritch/cpu-audio/releases/tag/5.pre1
- Use →tab to move focus in the page


Actual results:

Once the focus is into a shadowDOM of the webcomponent, the focus cannot go outside forward or backward


Expected results:

The focus must continue to go forward
(Reporter)

Comment 1

9 months ago
I find out a part of the answer : 

The shadowRoot of my WebComponent has “2 tabindex="0"” declarations in the <template> tag  :

<div class="interface" tabindex="0">
	<div class="principal">
		<img class="poster nosmall" src="" alt="" />
		<div class="pageerror">
		</div>
		<div class="pagemain">
			<a class="control" tabindex="0">

If I put different positive numbers, I have less problems, excepted that the focus became really erratic.

Afterwards ( https://github.com/dascritch/cpu-audio/tree/39e9e18697afbf18d3ef6b782b74cd130ad8fbb6 ), I added some code to support a <menu>, with tabindex="0" added via innerHTML :

line.innerHTML = `<a href="#${self.audiotag.id}&t=${cuepoint}" tabindex="0">
			<strong>${cue.text}</strong>
			<span>${cuetime}</span>
		</a>`;

Funnily, the webcomponent is not reacting the same way, as you can focus backwards via [shift]+[tab], but you're still trapped in.

I can try to add programmatically element.tabindex, but it means a lot of undesired complexity.

Chrome works as desired. May be it is a bug due to their webcomponents.js polyfill ? It means my code really needs a native <link rel="import"> support.

Updated

9 months ago
Component: Untriaged → DOM
Product: Firefox → Core
Priority: -- → P3
(Assignee)

Comment 2

9 months ago
Any chance for a minimal testcase attached to this bug?
Flags: needinfo?(xaviermd)
(Assignee)

Updated

9 months ago
(Reporter)

Comment 3

9 months ago
Yes, I will try to create it
Flags: needinfo?(xaviermd)
(Reporter)

Comment 4

9 months ago
Here is the minimal testcase.

When you move your focus with the [tab] key (or [shift]+[tab] for backwards), once the focus enters into a webcomponent, it cannot evade it.
(Reporter)

Comment 5

9 months ago
Adding style for a better visibility
Attachment #8998222 - Attachment is obsolete: true
(Assignee)

Comment 6

9 months ago
Great, thanks. I'll try to look at this tomorrow
(Assignee)

Comment 7

9 months ago
ok, hmm, I think I've fixed that issue already once before. And I thought I had added a testcase :/
(Assignee)

Updated

9 months ago
Assignee: nobody → bugs
(Assignee)

Updated

9 months ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 8

9 months ago
On could argue this is a bug in FrameTraversal, since it deals with inline elements differently.

The key point in the test is that host is not div, but span.


(there is another sequential bug in shadow DOM, so I may need to tweak this code still a bit.)
Attachment #8998675 - Flags: review?(mrbkap)
(Assignee)

Updated

9 months ago
Summary: [WebComponents] Focus trap into webcomponents with shadowDOM → [FIX] Focus trap into webcomponents with shadowDOM
Comment on attachment 8998675 [details] [diff] [review]
shadowdomw_inline_host_focus.diff

Review of attachment 8998675 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFocusManager.cpp
@@ +3599,5 @@
>        nsIContent* currentContent = frame->GetContent();
>        nsIContent* oldTopLevelHost = currentTopLevelHost;
> +      if (oldTopLevelHost != currentContent) {
> +        nsIContent* topLevel = GetTopLevelHost(currentContent);
> +        currentTopLevelHost = topLevel;

Nit: no need for the temporary `topLevel`.
Attachment #8998675 - Flags: review?(mrbkap) → review+

Comment 11

9 months ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96daed2c2310
try to ensure sequential focusing can leave shadow trees, r=mrbkap

Comment 12

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96daed2c2310
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Comment 13

8 months ago
I just checked and.... keyboard focus is still trapped in a little more complex page.
What i have missed ? 
https://dascritch.net/vrac/.projets/audiowc/index.html
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

8 months ago
Could you open a new bug, please.
The patch in this bug fixed the issue shown by the testcase.
Status: REOPENED → RESOLVED
Last Resolved: 9 months ago8 months ago
Flags: needinfo?(xaviermd)
Resolution: --- → FIXED
(Reporter)

Comment 15

8 months ago
Yes, i will.

But i still don't understand where the problem lies, and so how to reproduce it.
Flags: needinfo?(xaviermd)
(Reporter)

Updated

8 months ago
Flags: needinfo?(xaviermd)
I also encountered a complicated situations where shadow dom focusable elements cannot be tabbed to, and right now I also haven't found the virutal cause where the issue lies. Uploading the whole document and styles seems an overkill
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.