Closed Bug 1495621 Opened 6 years ago Closed 5 years ago

migrate wizard binding to CE

Categories

(Core :: XUL, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
Priority: -- → P3
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?
(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.
Or more simply, we can make the wizardpage Custom Element set the correct slot attribute in its constructor.
Depends on: 1495622
(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).
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?
Flags: needinfo?(bugs)
Attached patch unified patch (obsolete) (deleted) — Splinter Review
Assignee: nobody → surkov.alexander
testcase please for the hang? But won't be able to look at it before end of this month.
Flags: needinfo?(bugs)
(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 :)
Attached patch alexander's patch rebased (deleted) — Splinter Review

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)

Attachment #9017783 - Attachment is obsolete: true
Attachment #9035849 - Attachment description: alexande's patch rebased → alexander's patch rebased

(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

cc'ing Emilio for ideas on comment #5

Flags: needinfo?(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.

(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!

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

Flags: needinfo?(emilio) → needinfo?(surkov.alexander)

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

Flags: needinfo?(surkov.alexander)

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

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

Ok, will take a look at the crash tomorrow.

Flags: needinfo?(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.

Flags: needinfo?(emilio)

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

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

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

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?

Flags: needinfo?(emilio)

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

Flags: needinfo?(ato)

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?

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

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(ato)

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

Flags: needinfo?(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>}.

Flags: needinfo?(ato)

(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

Flags: needinfo?(emilio)
Flags: needinfo?(bugs)

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.

Flags: needinfo?(emilio)

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

Flags: needinfo?(bugs) → needinfo?(surkov.alexander)

(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

Flags: needinfo?(surkov.alexander)

(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

Flags: needinfo?(emilio)
Depends on: 1544906

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?

Flags: needinfo?(emilio) → needinfo?(surkov.alexander)

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

Flags: needinfo?(surkov.alexander)
Flags: needinfo?(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?

Flags: needinfo?(emilio)

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

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

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

Flags: needinfo?(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).

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)
Pushed by asurkov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49df528add26 marionette get_property fails to return wizard properties r=ato

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

Flags: needinfo?(emilio)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Type: enhancement → task

(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

Flags: needinfo?(emilio)

Why doesn't the algorithm visit originalStartContent again?

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

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

Flags: needinfo?(emilio)

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

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?

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

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.

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

(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

Attached patch not-affected.log (deleted) — Splinter Review

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

Flags: needinfo?(bugs)

I'd expect it to traverse with different tabindexes.

Flags: needinfo?(bugs)

I filed bug 1558990 for hanging issue.

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

Attachment #9071062 - Attachment is obsolete: true
Pushed by asurkov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/62ac95a65617 convert wizard binding to Custom Element r=bgrins
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED

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

Flags: needinfo?(shindli)

: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.

Flags: needinfo?(shindli)

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

Depends on: 1558990
Flags: needinfo?(surkov.alexander)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: