1476769 - Migrate the tabpanels binding to Custom Element using getCustomInterfaceCallback

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, task, P3)

Description

[Brian Grinstead [:bgrins]]

Once we can implement the nsIDOMXULRelatedElement interface with getCustomInterfaceCallback (Bug 1461742) and the tab-base binding is removed (Bug 1446168), I think this will be a good candidate for Custom Element conversion.

It just has a few properties / fields / methods: https://bgrins.github.io/xbl-analysis/tree/#tabpanels.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1476769 - Migrate tabpanels to a Custom Element

Comment 2

[Brian Grinstead [:bgrins]]

Try looks fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea9a9e24b16db88ecfe1bf2a59de308030af8fe

Comment 3

[Brian Grinstead [:bgrins]]

There's one failure on OSX: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea9a9e24b16db88ecfe1bf2a59de308030af8fe&selectedJob=204876586. This is related to `this.children` not being complete inside the connectedCallback (somehow only one of the <browser> elements exists: https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/widget/tests/test_bug428405.xul#20).

I knew we'd start to hit some cases where childNodes weren't complete inside connectedCallback during parse (https://github.com/w3c/webcomponents/issues/619, https://github.com/w3c/webcomponents/issues/668), so I think the right fix here (and anywhere we want to access childNodes) is to use delayConnectedCallback() to wait for DOMContentLoaded.

But this looks like an especially weird bug where accessing `this.children` seems to cause the value to "stick" and never get updated. If I comment out everything else except for a `connectedCallback() { console.log(this.children); }` then I see a list with length == 1. Even if I then inspect the element later this.children still only has length == 1 even though there are two children. To that point: `this.firstElementChild` and `this.lastElementChild` point to the first and second <browser> children! If we don't `console.log(this.children)` in connectedCallback, then if I inspect the element later on this.children has the correct length == 2. I tried to make this a smaller test case, but didn't have any luck (even with that exact xul test file - I guess it may have to do with it running in the test harness). It can be reproduced with https://phabricator.services.mozilla.com/D8458?vs=on&id=23619&whitespace=ignore-most#toc and `./mach mochitest test_bug428405.xul`.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c9dd41737f5
Migrate tabpanels to a Custom Element r=dao

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> There's one failure on OSX:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3ea9a9e24b16db88ecfe1bf2a59de308030af8fe&selectedJob=2
> 04876586. This is related to `this.children` not being complete inside the
> connectedCallback (somehow only one of the <browser> elements exists:
> https://searchfox.org/mozilla-central/rev/
> 28fe656de6a022d1fc01bbd3811023fca99cf223/widget/tests/test_bug428405.xul#20).
> 
> I knew we'd start to hit some cases where childNodes weren't complete inside
> connectedCallback during parse
> (https://github.com/w3c/webcomponents/issues/619,
> https://github.com/w3c/webcomponents/issues/668), so I think the right fix
> here (and anywhere we want to access childNodes) is to use
> delayConnectedCallback() to wait for DOMContentLoaded.

Yeah, this is very easy with anything that schedules a microtask checkpoint. For example given a x-custom-element which logs this.children.length on connectedCallback():

<my-custom-element>
  <script>document.body;</script>
  <div></div>
</my-custom-element>

the connectedCallback will log 1 instead of 2.

> But this looks like an especially weird bug where accessing `this.children`
> seems to cause the value to "stick" and never get updated. If I comment out
> everything else except for a `connectedCallback() {
> console.log(this.children); }` then I see a list with length == 1. Even if I
> then inspect the element later this.children still only has length == 1 even
> though there are two children. To that point: `this.firstElementChild` and
> `this.lastElementChild` point to the first and second <browser> children! If
> we don't `console.log(this.children)` in connectedCallback, then if I
> inspect the element later on this.children has the correct length == 2.

Yeah, so .children is created lazily, so that explains why when you inspect the element without having accessed it earlier it has the right content. Lists are invalidated in:

  https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/dom/base/nsContentList.cpp#670

And related code. You may have hit a bug either in the way the parser notifies of elements (which means that function wouldn't be called), or in the invalidation logic... I've poked at it but I haven't managed to break it, even in xhtml mode.

> I tried to make this a smaller test case, but didn't have any luck (even with
> that exact xul test file - I guess it may have to do with it running in the
> test harness). It can be reproduced with
> https://phabricator.services.mozilla.com/
> D8458?vs=on&id=23619&whitespace=ignore-most#toc and `./mach mochitest
> test_bug428405.xul`.

Finding a test-case would be amazing, does what I said ring a bell about something that might be special in this case?

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8c9dd41737f5