Closed Bug 1544916 Opened 8 months ago Closed 6 months ago

migrate dialog binding to Custom Element

Categories

(Core :: XUL, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla69
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.

Priority: -- → P3

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

Flags: needinfo?(emilio)

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.

(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).

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.

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)

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()?

Flags: needinfo?(emilio)

(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 the relatedTarget and target are 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(). 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()?

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

Flags: needinfo?(emilio)

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:

https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/toolkit/content/tests/chrome/test_dialogfocus.xul#107

So you need to integrate that into the event handlers, presumably.

Flags: needinfo?(emilio)
Assignee: nobody → surkov.alexander
Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/843c150636f8
migrate dialog binding to Custom Element r=bgrins,whimboo
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Type: defect → task
Regressions: 1557252
Regressions: 1557333
Regressions: 1576363
Regressions: 1580646
No longer regressions: 1580646
You need to log in before you can comment on or make changes to this bug.