migrate dialog binding to Custom Element
Categories
(Core :: XUL, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: surkov, Assigned: surkov)
References
Details
Attachments
(1 file)
Makes sense to wait for bug 1495621 (wizard binding migration), which is quite similar to this one, and which is blocked by a number of focus issues in shadow DOM (see for example bug 1544826), when a document root element (dialog or wizard) has shadow DOM attached.
|
||
Updated•6 years ago
|
|
Assignee | |
Comment 1•6 years ago
|
||
|
|
Assignee | |
Comment 2•6 years ago
|
||
Emilio, there's a weirdness with advanceFocusIntoSubtree on a shadow element [1]. It fails on Windows and Linux [2]. The test expects the default accept button will be focused [3], but instead an explicit button@id="one" gets the focus. Could you please take a look at the issue?
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=71241f24e895add70fc2efc390c32b621c551800
[2] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247963523&repo=try&lineNumber=18420
[3] https://hg.mozilla.org/try/rev/039878a633cad2c23330be6d69bc969dab344b4f#l8.593
|
||
Comment 3•6 years ago
|
||
I'll take a look but right now rr doesn't work on my machine so probably will need to wait until I sort that out.
|
||
Comment 4•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)
I'll take a look but right now rr doesn't work on my machine so probably will need to wait until I sort that out.
Here is a link to an rr trace https://pernos.co/debug/MxijZAIiKGDY76DsVI9swA/index.html (generated from https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc4226dd3932c083997e9b7330e66d10005b49f&selectedJob=248261348).
|
|
||
Comment 5•6 years ago
|
||
This is a test bug. Here's a reduced test-case that shows what's going on:
<!doctype html>
<div id="root">
<button id="b">B</button>
</div>
<script>
root.attachShadow({ mode: "open" }).innerHTML = `
<button id="a">A</button>
<slot></slot>
<button id="c">C</button>
<button id="d">D</button>
`;
window.addEventListener("focus", function(e) {
console.log("focus: ", e.target);
}, true);
</script>
Note how when tabbing from c to d you don't see any event. That's exactly what's happening and is per spec. So the right thing ends up focused, it's just that the test doesn't see it because the focus is moving inside the shadow root.
So basically the way to fix this is fixing the test. You should be able to do that easily adding the event listener to the document element's shadowRoot or such, for example.
|
|
Assignee | |
Comment 6•6 years ago
|
||
For some reason, I see focus event listener registered on window reports events from both explicit and shadow content, for example [1], where shadow extra2 button is focused. Also the test claims that explicit button@id="one" is focused when advanceFocusIntoSubtree called for a shadow accept button [2]. Just in case, I registered event listeners both on window and on shadowRoot, now both listeners reports that explicit button@id="one" is focused [3]. I'm not sure if the test should be adjusted to make it standards-friendly, but there's definitely some focus weirdness here.
[1] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247963523&repo=try&lineNumber=18370
[2] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247963523&repo=try&lineNumber=18378
[3] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248613493&repo=try&lineNumber=19909
|
|
||
Comment 7•6 years ago
|
||
It reports both because you're using originalTarget, which isn't retargeted to the shadow root. The event isn't dispatched to outside of the shadow tree when the relatedTarget and target are in the same shadow tree.
In https://hg.mozilla.org/try/rev/76c079ab7a47806784120b701623c4df51fcc24e#l7.593:
document.commandDispatcher.advanceFocusIntoSubtree(defaultButton);
That's not equivalent to the pre-existing code (and the code from the try run in comment 4) which does defaultButton.focus(). advanceFocusIntoSubtree moves to the next focus target if there's nothing in that subtree that is focusable, which can probably explain what you're seeing. Do you see the same with defaultButton.focus()?
|
|
Assignee | |
Comment 8•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
It reports both because you're using
originalTarget, which isn't retargeted to the shadow root. The event isn't dispatched to outside of the shadow tree when therelatedTargetandtargetare in the same shadow tree.
By saying that I'm using originalTarget did you mean advanceFocusIntoSubtree call or something else? It seems I don't explicitly using originalTarget anywhere but for logging inside of event listeners.
In https://hg.mozilla.org/try/rev/76c079ab7a47806784120b701623c4df51fcc24e#l7.593:
document.commandDispatcher.advanceFocusIntoSubtree(defaultButton);That's not equivalent to the pre-existing code (and the code from the try run in comment 4) which does
defaultButton.focus().advanceFocusIntoSubtreemoves to the next focus target if there's nothing in that subtree that is focusable, which can probably explain what you're seeing. Do you see the same withdefaultButton.focus()?
You're right. I have replaced defaultButton.focus() on advanceFocusIntoSubtree() for experimenting while was investigating failures. It indeed helps. Thanks a lot!
I now have another failure though on step10 where dialog itself is expected to be focused [1]. The dialog [2] has noinitialfocus button and thus dialog is expected to grab the focus itself. Since commandDispatcher.focusedElement is null, then dialog calls advanceFocusIntoSubtree what makes the noinitialfocus button focused, then advanceFocusIntoSubtree is called for noinitialfocus button which keeps it focused, and thus blur() is called at the end [3]. After that the focus goes nowhere. I suppose that blur() is expected to move the focus to dialog itself. Is it about right?
[1] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248635318&repo=try&lineNumber=20341
[2] https://searchfox.org/mozilla-central/source/toolkit/content/tests/chrome/dialog_dialogfocus2.xul
[3] https://hg.mozilla.org/try/rev/f732890eacb5951e61b4814f0bfc7c76dbf533d5#l7.416
|
|
||
Comment 9•6 years ago
|
||
Yes, when you blur the focus isn't supposed to go anywhere, but the final activeElement is supposed to be the document element (no focus event should happen).
This is what this line of code is doing, but with your test changes that no longer happens IIUC:
So you need to integrate that into the event handlers, presumably.
|
|
Assignee | |
Updated•6 years ago
|
|
||
Comment 10•6 years ago
|
||
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
|
||
Updated•5 years ago
|
Description
•