[FIX] Focus trap into webcomponents with shadowDOM

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
11 months ago
3 months ago

People

(Reporter: xaviermd, Assigned: smaug, NeedInfo)

Tracking

({nightly-community})

63 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

11 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

11 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

11 months ago
Component: Untriaged → DOM
Product: Firefox → Core
Priority: -- → P3
Assignee

Comment 2

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

Updated

11 months ago
Reporter

Comment 3

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

Comment 4

11 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

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

Comment 6

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

Comment 7

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

Updated

10 months ago
Assignee: nobody → bugs
Assignee

Updated

10 months ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 8

10 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

10 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

10 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96daed2c2310
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Reporter

Comment 13

10 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

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

Comment 15

10 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

10 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
You need to log in before you can comment on or make changes to this bug.