1495621 - migrate wizard binding to CE

Status: RESOLVED FIXED

(Core :: XUL, task, P3)

Comment 1

[alexander :surkov (:asurkov)]

the wizzard binding changes insertion points for wizardpage elements and all other elements (stringbundle, script). Should I proceed with @slot mechanism and put @slot="wizardpage" attribute on each wizardpage element? or is there nicer approach?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #1)
> the wizzard binding changes insertion points for wizardpage elements and all
> other elements (stringbundle, script). Should I proceed with @slot mechanism
> and put @slot="wizardpage" attribute on each wizardpage element? or is there
> nicer approach?

It would be nice if there was a way to have the SD be in charge of what gets slotted in, instead of the page having to request it, but that's not part of the spec. I'm not sure why it was specced to flip it so that decision is made by the calling code instead of the shadow host. If we wanted to simulate this, I could imagine writing some code in which the CE sets the [slot] attribute automatically on all child wizardpage's in connectedCallback, and then sets up a MutationObserver to detect when new ones are added and sets the attribute on them as well. From what I hear the MO doesn't have too much overhead, so that could be viable (esp if we aren't watching subtree and just direct children on the CE).

Setting the slot attribute in the calling markup seems OK to me as well.

Comment 3

[Brian Grinstead [:bgrins]]

Or more simply, we can make the wizardpage Custom Element set the correct slot attribute in its constructor.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Or more simply, we can make the wizardpage Custom Element set the correct
> slot attribute in its constructor.

yeah, that one should work (I hope it can't cause some kind of re-insertions and thus lead to intermittents).

Comment 5

[alexander :surkov (:asurkov)]

Ran into a problem of nsXULCommandDispatcher::AdvanceFocusIntoSubtree, which hangs when called for a slotted wizardpage. It appears it falls into infinite recursion in nsFocusManager::GetNextTabIndex [1].

[1] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#3991

Olli, do you have ideas how to approach it? Is anyone familiar with this code?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: unified patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

testcase please for the hang? But won't be able to look at it before end of this month.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #7)
> testcase please for the hang?

No tests but steps to reproduce with the patch applied: Bookmarks -> Show all bookmarks -> Import data from another browser -> Continue

> But won't be able to look at it before end of
> this month.

Thank you. It feels a sort of stopper for slotting into shadow DOM though, it doesn't feel specific for wizard CE. If you can think of anyone who could take a look before you, please cc :)

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: alexander's patch rebased

I rebased the patch and tried comment 8. The browser still hangs when I clicked [Continue].

(Also, the style is missing because document sheet does not apply to Shadow DOM content)

Comment 10

[alexander :surkov (:asurkov)]

Attachment: Bug 1495621 - convert wizard binding to Custom Element

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo; no longer work for MoCo) from comment #9)

Created attachment 9035849 [details] [diff] [review]
alexander's patch rebased

I rebased the patch and tried comment 8. The browser still hangs when I clicked [Continue].

rebased the patch, hangs present

(Also, the style is missing because document sheet does not apply to Shadow DOM content)

I linked widgets styles into markup, now it seems ok

Comment 12

[alexander :surkov (:asurkov)]

cc'ing Emilio for ideas on comment #5

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Edgar fixed some focus issues inside Shadow DOM not long ago. Is this issue still present? I guess it is per comment 11, and relevant str is still comment 8.

I was going to ni? him since he had more chances to remember this code, but he's on leave. I'll try to take a look at some point of the week.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

Edgar fixed some focus issues inside Shadow DOM not long ago. Is this issue still present? I guess it is per comment 11, and relevant str is still comment 8.

yeah, still an issue

I was going to ni? him since he had more chances to remember this code, but he's on leave. I'll try to take a look at some point of the week.

thank you!

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I could import bookmarks from other browsers without issues, what am I doing wrong? :)

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

I could import bookmarks from other browsers without issues, what am I doing wrong? :)

hm, are you sure you have the patch applied: clicking 'Continue' in Import Wizard Dialog, hangs the browser for me. If I click 'Create Profile' button in Profile Dialog (./mach run -P), then the browser crashes. I can see that on OS X on today's locally built central.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to alexander :surkov (:asurkov) from comment #16)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

I could import bookmarks from other browsers without issues, what am I doing wrong? :)

hm, are you sure you have the patch applied: clicking 'Continue' in Import Wizard Dialog, hangs the browser for me. If I click 'Create Profile' button in Profile Dialog (./mach run -P), then the browser crashes. I can see that on OS X on today's locally built central.

Pretty sure, I can also repro that crash. Focus handling on OSX is quite a bit different from Linux though.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

(In reply to alexander :surkov (:asurkov) from comment #16)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #15)

I could import bookmarks from other browsers without issues, what am I doing wrong? :)

hm, are you sure you have the patch applied: clicking 'Continue' in Import Wizard Dialog, hangs the browser for me. If I click 'Create Profile' button in Profile Dialog (./mach run -P), then the browser crashes. I can see that on OS X on today's locally built central.

Pretty sure, I can also repro that crash. Focus handling on OSX is quite a bit different from Linux though.

I hope you can help with the crash then, and then let's cross the fingers the crash fixes the hang :)

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Ok, will take a look at the crash tomorrow.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Well, the crash happens because of the <link rel="stylesheet"> that you're
adding. We have code to lazily query the visited state of that link, for
which we need the history service. But obviously there's no profile yet when
that happens.

Other browsers don't seem to support styling <link> elements with
:visited, fwiw, but we do. The second <link> element should be purple.

Seems like other browsers don't ever match :link / :any-link on <link>
elements.

<style>
  body { margin: 0 }
  a, head, link { display: block }
  a, link { width: 100px; height: 100px; background-color: red }
  :any-link { background-color: blue }
  a:visited, link:visited { background-color: purple }
</style>
<link href="https://google.es">
<link href="">
<a href="https://mozilla.org"></a>
<a href=""></a>

Per spec, I think we're right:

https://drafts.csswg.org/selectors-4/#location

Or at least, don't see anything that would make <link> special there.

But I don't think there's any use-case for this, and I think we could just make
<link> not respect the link state.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)

Well, the crash happens because of the <link rel="stylesheet"> that you're
adding. We have code to lazily query the visited state of that link, for
which we need the history service. But obviously there's no profile yet when
that happens.

widgets.css is linked to a shadow DOM of xul:tree custom element already, https://searchfox.org/mozilla-central/source/toolkit/content/widgets/tree.js#515. If presence of :visited style is a problem, then it should crash as well, no?

Per spec, I think we're right:

https://drafts.csswg.org/selectors-4/#location

Or at least, don't see anything that would make <link> special there.

But I don't think there's any use-case for this, and I think we could just make
<link> not respect the link state.

Is it something you could fix?

Comment 22

[Emilio Cobos Álvarez (:emilio)]

https://github.com/w3c/csswg-drafts/issues/3817 is the spec issue, will write a work-around applicable only for XUL documents for now.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

(In reply to alexander :surkov (:asurkov) from comment #21)

widgets.css is linked to a shadow DOM of xul:tree custom element already, https://searchfox.org/mozilla-central/source/toolkit/content/widgets/tree.js#515. If presence of :visited style is a problem, then it should crash as well, no?

Well, it's only a problem if there's no profile at the time we query the visited state. Which looks like it can only happen in a few selected places.

Comment 24

[alexander :surkov (:asurkov)]

I cannot reproduce a hang when I click 'Continue' button in Profile Manager, however it hangs when I press 'Back' button afterwards. It falls into infinite recursion in nsFocusManager::GetNextTabbableContentInAncestorScopes, where startContent and its owner is a wizard element, see https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#3202.

Emilio, any ideas on this?

Comment 25

[alexander :surkov (:asurkov)]

Another problem is in test_update_wizard.py test [1] which fails to grab wizard-buttons, a shadow DOM element of <wizard> element, in wizard.py [2].

I expected that changing it to

self.element.find_element(By.CSS_SELECTOR, '.wizard-buttons')

will work out per [3], but apparently |shadowRoot| is never set [4].

Not sure, who could help with it. Maybe :ato has ideas?

[1] ./mach test testing/firefox-ui/tests/puppeteer/test_update_wizard.py TestUpdateWizard.test_elements
[2] https://searchfox.org/mozilla-central/source/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/update_wizard/wizard.py#53
[3] https://searchfox.org/mozilla-central/source/testing/marionette/element.js#279
[4] https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#2009

Comment 26

[Brian Grinstead [:bgrins]]

I don't expect this is it - but is it possible the element hasn't been upgraded yet due to some weirdness with it being used in a XBL binding or something? Like if you force container.ownerDocument.defaultView.customElements.upgrade(container) before https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/testing/marionette/element.js#352 do you see the same thing? And can you confirm that shadowRoot is null on that line for the failing case?

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #26)

I don't expect this is it - but is it possible the element hasn't been upgraded yet due to some weirdness with it being used in a XBL binding or something? Like if you force container.ownerDocument.defaultView.customElements.upgrade(container) before https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/testing/marionette/element.js#352 do you see the same thing? And can you confirm that shadowRoot is null on that line for the failing case?

I prototyped find_element extension to get an element from a property, it works. So it shouldn't be an upgrade issue.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to alexander :surkov (:asurkov) from comment #24)

I cannot reproduce a hang when I click 'Continue' button in Profile Manager, however it hangs when I press 'Back' button afterwards. It falls into infinite recursion in nsFocusManager::GetNextTabbableContentInAncestorScopes, where startContent and its owner is a wizard element, see https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#3202.

Emilio, any ideas on this?

I cannot repro that, neither when creating a profile nor when importing bookmarks from another browser.

But does that mean that FindOwner(wizard) returns the same wizard element? That's bad, since FindOwner always looks at an ancestor.

Comment 29

[Andreas Tolfsen ❲:ato❳]

(In reply to alexander :surkov (:asurkov) from comment #25)

Another problem is in test_update_wizard.py test [1] which fails
to grab wizard-buttons, a shadow DOM element of <wizard> element,
in wizard.py [2].

Marionette doesn’t support Shadow DOM, but the trick bgrins mentioned
might help?

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #29)

(In reply to alexander :surkov (:asurkov) from comment #25)

Another problem is in test_update_wizard.py test [1] which fails
to grab wizard-buttons, a shadow DOM element of <wizard> element,
in wizard.py [2].

Marionette doesn’t support Shadow DOM

It references shadowRoot at https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/testing/marionette/element.js#352. What type of object is container here?

Comment 31

[Andreas Tolfsen ❲:ato❳]

(In reply to Brian Grinstead [:bgrins] from comment #30)

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #29)

Marionette doesn’t support Shadow DOM

It references shadowRoot at
https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516a
ffe4d28cb7fa220a/testing/marionette/element.js#352. What type of
object is container here?

I thought it was an implementation of one of the very first versions
of Shadow DOM, so I wouldn’t expect it to work. Though there is a
test in
testing/marionette/harness/marionette_harness/tests/unit/test_shadow_dom.py
that seems to indicate that it does. I haven’t checked if what it
does is correct. AutomatedTester may know more.

container is a {"window": <WindowProxy>, "shadowRoot": <Element>}.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #28)

(In reply to alexander :surkov (:asurkov) from comment #24)

I cannot reproduce a hang when I click 'Continue' button in Profile Manager, however it hangs when I press 'Back' button afterwards. It falls into infinite recursion in nsFocusManager::GetNextTabbableContentInAncestorScopes, where startContent and its owner is a wizard element, see https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#3202.

Emilio, any ideas on this?

I cannot repro that, neither when creating a profile nor when importing bookmarks from another browser.

But does that mean that FindOwner(wizard) returns the same wizard element? That's bad, since FindOwner always looks at an ancestor.

FindOwner returns a wizard for wizard, because wizard is a root element, and thus it has no nsIContent parent and FindOwner fallsbacks into a root element, which is a wizard element [1].

I'm not sure what is expected behavior of FindOwner, but the hang can be fixed if nsFocusManager::GetNextTabbableContentInAncestorScopes will compare startContent and owner and will break the loop if they match. I'm not sure however if there are any other underlying issues leading to this problem.

[1] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2999

Comment 33

[Emilio Cobos Álvarez (:emilio)]

There's definitely some oddness when the shadow root is the root element. Compare this test-case with and without the onload handler commented out:

<!doctype html>
<div id="host"><a href="#">This is focusable too</a></div>
<script>
  let host = document.getElementById("host");
  host.attachShadow({ mode: "open" }).innerHTML = `
    <a href="#">This is focusable</a>
    <slot></slot>
    <a href="#">So is this</a>
  `;
  onload = function () {
    document.documentElement.remove();
    document.appendChild(host);
  }
</script>

Will file a bug for that.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Can you confirm that the patch in bug 1544826 fixes the issue you're seeing?

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)

Can you confirm that the patch in bug 1544826 fixes the issue you're seeing?

it doesn't, let me see where it hangs now

Comment 36

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #35)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)

Can you confirm that the patch in bug 1544826 fixes the issue you're seeing?

it doesn't, let me see where it hangs now

It keeps reentering GetNextTabbableContent for a wizard element as a rootContent/startContent [1] in the while(doc) loop inside FocusManager::DetermineElementToMoveFocus(), in the end of the loop null startContent is assigned to rootContent and the loop keeps rolling all over again.

[1] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2728
[2] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2862

Comment 37

[Emilio Cobos Álvarez (:emilio)]

So I assume that forDocumentNavigation is false, right? How's the callstack that gets you there?

Because otherwise I guess you're supposed to hit:

https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/dom/base/nsFocusManager.cpp#2860

And break out. So that probably can happen when navigating around when no single focusable element exists at all...

The other thing that is supposed to avoid hanging there is:

https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/dom/base/nsFocusManager.cpp#2869

What are the values of originalStartContent and startContent as you iterate?

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #37)

So I assume that forDocumentNavigation is false, right?

correct

How's the callstack that gets you there?

#6 0x000000011ba5d67f in nsFocusManager::DetermineElementToMoveFocus(nsPIDOMWindowOuter*, nsIContent*, int, bool, nsIContent**) at /Users/heh/mozilla/central/dom/base/nsFocusManager.cpp:2746
#7 0x000000011ba5c724 in nsFocusManager::MoveFocus(mozIDOMWindowProxy*, mozilla::dom::Element*, unsigned int, unsigned int, mozilla::dom::Element**) at /Users/heh/mozilla/central/dom/base/nsFocusManager.cpp:523
#8 0x000000011ceda089 in nsXULCommandDispatcher::AdvanceFocusIntoSubtree(mozilla::dom::Element*) at /Users/heh/mozilla/central/dom/xul/nsXULCommandDispatcher.cpp:213

Because otherwise I guess you're supposed to hit:

https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/dom/base/nsFocusManager.cpp#2860

And break out. So that probably can happen when navigating around when no single focusable element exists at all...

The other thing that is supposed to avoid hanging there is:

https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/dom/base/nsFocusManager.cpp#2869

What are the values of originalStartContent and startContent as you iterate?

originalStartContent is a wizardpage (<wizardpage id='explanation'>), startContent is a wizard itself (<wizard id='createProfileWizard'>).

Comment 39

[Emilio Cobos Álvarez (:emilio)]

So originalStartContent is the slotted content of the wizard, I guess? How do we get to that situation? Is that element somehow focusable? If so, how do we end up not finding it anymore?

Comment 40

[alexander :surkov (:asurkov)]

Attachment: Bug 1495621 - marionette get_property fails to return wizard properties

Comment 41

[alexander :surkov (:asurkov)]

crash healed itself, so the only blocking is left is failing marionette tests, but it appears I have a fix for this case.

Comment 42

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #41)

crash healed itself, so the only blocking is left is failing marionette tests, but it appears I have a fix for this case.

so the crash didn't heal itself completely, it became an intermittent one - https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245010505&repo=try&lineNumber=4616. I cannot reproduce it locally, which is sad.

Comment 43

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #42)

(In reply to alexander :surkov (:asurkov) from comment #41)

crash healed itself, so the only blocking is left is failing marionette tests, but it appears I have a fix for this case.

so the crash didn't heal itself completely, it became an intermittent one - https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245010505&repo=try&lineNumber=4616. I cannot reproduce it locally, which is sad.

this crash actually is something new, however it also related with advanceFocusIntoSubtree. I tried to pass a document into advanceFocusIntoSubtree (the crash stack is above), and a wizard page (crash stack is https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245113033&repo=try&lineNumber=17555).

Emilio, do the stacks say anything to you?

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Not much, that's just where the process was killed. The entry point for that stack is XULCommandDispatcher::AdvanceFocusIntoSubtree called from JS. Do you know what the JS stack looks at that point? (You can see it calling DumpJSStack() from the debugger).

Comment 45

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #44)

Not much, that's just where the process was killed. The entry point for that stack is XULCommandDispatcher::AdvanceFocusIntoSubtree called from JS. Do you know what the JS stack looks at that point? (You can see it calling DumpJSStack() from the debugger).

I think this is AdvanceFocusIntoSubtree called by a wizard on document "onload" after timeout (setTimeout(0)). Does it help?

I can't reproduce it locally, is there DumpJSStack() options available on try?

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49df528add26
marionette get_property fails to return wizard properties r=ato

Comment 47

[Emilio Cobos Álvarez (:emilio)]

(In reply to alexander :surkov (:asurkov) from comment #45)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #44)

Not much, that's just where the process was killed. The entry point for that stack is XULCommandDispatcher::AdvanceFocusIntoSubtree called from JS. Do you know what the JS stack looks at that point? (You can see it calling DumpJSStack() from the debugger).

I think this is AdvanceFocusIntoSubtree called by a wizard on document "onload" after timeout (setTimeout(0)). Does it help?

I can't reproduce it locally, is there DumpJSStack() options available on try?

Not much, no, I'd really need to be able to reproduce this with a debugger :(

DumpJSStack() should be available on debug builds, you should be able to call it from AdvanceFocusIntoSubtree or such.

Comment 48

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/49df528add26

Comment 49

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #39)

So originalStartContent is the slotted content of the wizard, I guess? How do we get to that situation? Is that element somehow focusable? If so, how do we end up not finding it anymore?

originalStartContent is a slotted content to the wizzard, correct. So originally the loop is entered for the slotted <wizardpage id='explanation'> element, but when we fail to find a focusable content inside it by GetNextTabbableContent [1], then startContent is dropped to root at [2], and it loops endlessly. I wonder if originalContent should be dropped to root as well to make this condition working [3]. At least it fixes the hang.

[1] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2741
[2] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2876
[3] https://searchfox.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2882

Comment 50

[Emilio Cobos Álvarez (:emilio)]

Why doesn't the algorithm visit originalStartContent again?

Comment 51

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #50)

Why doesn't the algorithm visit originalStartContent again?

I afraid I didn't I capture your question, this loop keeps calling GetNextTabbableContent for the originalStartContent.

Comment 52

[Emilio Cobos Álvarez (:emilio)]

(In reply to alexander :surkov (:asurkov) from comment #49)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #39)

So originalStartContent is the slotted content of the wizard, I guess? How do we get to that situation? Is that element somehow focusable? If so, how do we end up not finding it anymore?

originalStartContent is a slotted content to the wizzard, correct. So originally the loop is entered for the slotted <wizardpage id='explanation'> element, but when we fail to find a focusable content inside it by GetNextTabbableContent [1], then startContent is dropped to root at [2], and it loops endlessly. I wonder if originalContent should be dropped to root as well to make this condition working [3]. At least it fixes the hang.

Honestly it's hard to say without being able to reproduce it locally. It only happens on mac, right? If you're on a mac, can you repro it? I tried on Linux with a mac-like set of a11y prefs and I couldn't get it to repro.

Comment 53

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #52)

Honestly it's hard to say without being able to reproduce it locally. It only happens on mac, right? If you're on a mac, can you repro it? I tried on Linux with a mac-like set of a11y prefs and I couldn't get it to repro.

yeah, it happens on mac and I can reproduce it locally: just clicking 'Create Profile' button in profile manager window triggers it.

Comment 54

[alexander :surkov (:asurkov)]

Attachment: Bug 1495621 - prevent infinite looping in nsFocusManager::DetermineElementToMoveFocus

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Is the issue that there is nothing focusable in the window? (on Mac buttons aren't tab-focusable, IIRC).
But what happen currently, without CE?

Comment 56

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #55)

Is the issue that there is nothing focusable in the window? (on Mac buttons aren't tab-focusable, IIRC).
But what happen currently, without CE?

Right, that's my theory, but then I should be able to reproduce this with accessibility.tabfocus=1 on Linux, but I don't.

Oh, but as I'm writing this I realized that all the #ifdef XP_MACOSX stuff in there does affect the test (I thought not) because the test uses the mouse.

Comment 57

[Neil Deakin]

The following is the log I get with this patch applied:

[(null) 83469: Main Thread]: D/FocusNavigation Focus Navigation Start Content wizardpage
[(null) 83469: Main Thread]: D/FocusNavigation Forward: 1 Tabindex: 1 Ignore: 1 DocNav: 0
[(null) 83469: Main Thread]: D/FocusNavigation GetNextTabbable: wizardpage
[(null) 83469: Main Thread]: D/FocusNavigation tabindex: 1
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable popupgroup:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 0
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable hbox:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 0
<----------------- Here is where the patch above sets 'breakReentry' to true
[(null) 83469: Main Thread]: D/FocusNavigation GetNextTabbable: wizard
[(null) 83469: Main Thread]: D/FocusNavigation tabindex: 1
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable wizard:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 1
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable popupgroup:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 1
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable hbox:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 1
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable popupgroup:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 0
[(null) 83469: Main Thread]: D/FocusNavigation Next Tabbable hbox:
[(null) 83469: Main Thread]: D/FocusNavigation with tabindex: -1 expected: 0

The starting wizardpage is never visited again at least for the logging done here.

Comment 58

[alexander :surkov (:asurkov)]

Curious, how does the log with no patch applied look like? How do you enable this logging?

Comment 59

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #55)

Is the issue that there is nothing focusable in the window? (on Mac buttons aren't tab-focusable, IIRC).

it keeps re-entrying for <wizardpage id='explanation'> (see comment #49), which contains no focusables inside [1]. I suppose something in the window should be focusable (maybe not tab focusable though): wizard buttons or at least document itself.

[1] https://searchfox.org/mozilla-central/source/toolkit/profile/content/createProfileWizard.xul#29

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

https://searchfox.org/mozilla-central/rev/c606cdd6d014fee4034da1702d484c0d41b604c9/dom/base/nsFocusManager.cpp#87-88

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR/Reference/NSPR_LOG_MODULES

Comment 61

[alexander :surkov (:asurkov)]

Attachment: not-affected.log

(In reply to Olli Pettay [:smaug] from comment #55)

But what happen currently, without CE?

currently it goes through anonymous content of XBL wizard binding twice (!): wizard header, then wizardpages (each of them twice again) and then it finishes by traversing anonymous XBL buttons, then it ends up with "Element to be focused: (none)". Does it look like something is broken (or not optimized at least) that it traverses same things twice?

After the patch, it enters for a first wizardpage (wizardpage@id='explanation'), not for a wizard itself, then it jumps to the wizard, but it seems never traverse its shadow content.

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

I'd expect it to traverse with different tabindexes.

Comment 63

[alexander :surkov (:asurkov)]

I filed bug 1558990 for hanging issue.

Comment 64

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #63)

I filed bug 1558990 for hanging issue.

so, wizard hangs when I advanceFocusIntoSubtree is called on a slotted wizardpage (see bug 1558990 for isolated testcase). The original wizard's version used to call advanceFocusIntoSubtree for a document element, so I reverted the behavior for CE wizard, and it solved a problem. So filed bug 1558990, since it's a separate issue and doesn't longer block this one.

I started a try server build and if it's green, then this wizard thing can be landed.

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62ac95a65617
convert wizard binding to Custom Element r=bgrins

Comment 66

[Narcis Beleuzu [:NarcisB]]

Backed out for marionette mozprocess timed out failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/5263ebe8baf2a3c76b224e547b960c2e68057d88
Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Ctest-macosx1014-64-shippable%2Fopt-marionette-e10s%2C%28mn%29&revision=62ac95a65617d4a0bdcf494986d4b9c9626cc7f9&selectedJob=251725222
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251725321&repo=autoland&lineNumber=55597

Comment 67

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/62ac95a65617

Comment 68

[Brian Grinstead [:bgrins]]

(In reply to Stefan Hindli [:stefan_hindli] from comment #67)

https://hg.mozilla.org/mozilla-central/rev/62ac95a65617

I'm confused - shouldn't the backout (Comment 66 / https://hg.mozilla.org/integration/autoland/rev/5263ebe8baf2a3c76b224e547b960c2e68057d88) have merged alongside this, and this not have been resolved?

If it didn't I expect the Mn failures from Comment 66 to be on m-c.

Comment 69

[Stefan Hindli [:stefan_hindli]]

:bgrins we will merge the backout in ~ 2 hours if everything goes well. The plan was to merge the backout also but there were some issues with some failed jobs and decided to use an earlier changeset.

Comment 70

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/62ac95a65617

Comment 71

[alexander :surkov (:asurkov)]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #70)

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/62ac95a65617

wizard tries to move the focus into a slotted wizardpage, and it hangs, so we need bug 1558990 get resolved before landing this one.

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6fd4af8043d
convert wizard binding to Custom Element

Comment 73

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a6fd4af8043d