Status

()

enhancement
P3
normal
REOPENED
8 months ago
11 days ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 affected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

8 months ago
Priority: -- → P3
(Assignee)

Comment 1

8 months ago
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
See Also: → 1495946
(Assignee)

Comment 4

8 months ago
(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).
(Assignee)

Comment 5

7 months ago
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)
(Assignee)

Comment 6

7 months ago
Posted patch unified patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
See Also: → 1496242
testcase please for the hang? But won't be able to look at it before end of this month.
Flags: needinfo?(bugs)
(Assignee)

Comment 8

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

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
See Also: → 1397876
(Assignee)

Comment 11

a month ago

(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

(Assignee)

Comment 12

a month ago

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.

(Assignee)

Comment 14

a month ago

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

Comment 16

a month ago

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

(Assignee)

Comment 18

a month ago

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

Comment 21

a month ago

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

(Assignee)

Comment 24

a month ago

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

Comment 25

a month ago

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?

(Assignee)

Comment 27

a month ago

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

Comment 32

a month ago

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

Comment 35

a month ago

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

Comment 36

a month ago

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

Updated

a month ago
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)
(Assignee)

Comment 38

a month ago

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

Updated

a month ago
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)
(Assignee)

Comment 41

13 days ago

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

(Assignee)

Comment 42

12 days ago

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

(Assignee)

Comment 43

12 days ago

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

Comment 45

11 days ago

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

Comment 46

11 days ago
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)

Comment 48

11 days ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 11 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.