Migrate the tabpanels binding to Custom Element using getCustomInterfaceCallback

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
4 months ago
a month ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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.
Priority: -- → P3
(Assignee)

Comment 1

a month ago
Created attachment 9016438 [details]
Bug 1476769 - Migrate tabpanels to a Custom Element
(Assignee)

Updated

a month ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a month ago
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/﷒0﷓).

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

a month ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c9dd41737f5
Migrate tabpanels to a Custom Element r=dao
(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/﷒1﷓

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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c9dd41737f5
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.