1517040 - Port Bug 1472558 Convert "richlistbox" to Custom Element

Status: RESOLVED FIXED

(Thunderbird :: Upstream Synchronization, enhancement)

Description

[Jorg K (CEST = GMT+2)]

See bug 1472558 comment #4.

Comment 1

[Richard Marti (:Paenglab)]

I hope, Arshad or Magnus can convert it before it lands. All I could do is to fork the binding.

Comment 2

[Jorg K (CEST = GMT+2)]

Well, Magnus is away this week. I know Geoff loves listboxes ;-)

Comment 3

[Brian Grinstead [:bgrins]]

Arshad, out of the options listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1472558#c4 which are you thinking? Or do you have another idea? I'd like to land that bug this week if possible, and am wondering if we should initially expose the base MozRichlistbox class out onto MozElements so you can extend it (option 1).

One more thing I noticed with option 3 is that there's an IsNonList helper function (https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/xul/nsXULElement.cpp#384) that's checking the richlistbox tag name and seems used by some XUL element focus code, so renaming all existing richlistboxes to a new tag name would mean they didn't match there either.

One final thought is that if we could build some generic way 'opt-out' of the Custom Element from being registered then you could skip this from being loaded in TB, fork out the XBL binding in the meantime, and opt back in when the other bindings support it. Feels like it could get a bit complicated though, so unless if you feel it's required I'd prefer to ship it unconditionally.

Comment 4

[Brian Grinstead [:bgrins]]

The patch for m-c is on inbound. I'd be OK to take a patch that somehow skips the call to customElements.define in thunderbird (https://hg.mozilla.org/integration/mozilla-inbound/rev/ff84fa856102#l4.1833) if you want to restore the richlistbox binding while you work through inherited bindings. But with the warning that once it becomes a CE, calling code in toolkit won't be testing the XBL case. Although since no calling code needed to be updated for the initial landing here I don't think there will be too much moving around.

Comment 5

[Jorg K (CEST = GMT+2)]

I guess we'll be busted :-(

Comment 6

[Brian Grinstead [:bgrins]]

Forking the binding and then updating consumers to inherit from the forked one is doable (although behavior with both XBL and CE attached to an element is undefined) and could be done now.

Combining it with a quick fix in the custom element to skip the call to `define` in Thunderbird (I guess if AppConstants.MOZ_APP_NAME is "thunderbird" or something) would get rid of the undefined behavior and fix it (this is basically a better option 3 from https://bugzilla.mozilla.org/show_bug.cgi?id=1472558#c4).

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Forking the binding and then updating consumers to inherit from the forked
> one is doable (although behavior with both XBL and CE attached to an element
> is undefined) and could be done now.
> 
> Combining it with a quick fix in the custom element to skip the call to
> `define` in Thunderbird (I guess if AppConstants.MOZ_APP_NAME is
> "thunderbird" or something) would get rid of the undefined behavior and fix
> it (this is basically a better option 3 from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1472558#c4).

Yes, please do that for now Brian. I'll fork the binding until we can fix this properly.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: Bug 1517040 - Allow Thunderbird to continue using richlistbox as XBL, and to extend the Custom Element class;r=paolo

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1517040-richlist-binding-1.diff

We're busted now, but when the other patch lands on m-c, we can push this to c-c for a temporary fix.

Comment 10

[Geoff Lankow (:darktrojan)]

There's surprisingly little broken. I think we could get away without deleting our bindings, and putting the needed code elsewhere.

Comment 11

[Arshad Khan [:arshad]]

Attachment: xbl-richlistbox.patch

I am changing all the instances of richlistbox with xbl-richlistbox so that there is no name conflict.

Comment 12

[Arshad Khan [:arshad]]

Mybad, that ll mess up IsNonList helper function (https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/xul/nsXULElement.cpp#384).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c6f13700602b
No bug - Disable Nightly builds until richlistbox issue is fixed. a=jorgk

Comment 14

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b1fcd0c07e0dae9b375a14c7835f10a71f26b726&selectedJob=219941711

Comment 15

[Jorg K (CEST = GMT+2)]

Looking promising. Can you check why the remaining tests are failing and fix them? Or should I just disable them?

Comment 16

[Jorg K (CEST = GMT+2)]

Looks like Mozmill downloads/test-about-downloads.js only fails on Mac. Works here on Windows, and works on the tree on Linux.

Comment 17

[Jorg K (CEST = GMT+2)]

On Linux there appear to be four failures:

test-attachment.js::test_selected_attachments_are_cleared
test-attachment.js::test_delete_attachments
test-attachment.js::test_rename_attachment
test-attachment.js::test_open_attachment

On Windows I can only see the last three fail.

Aceman, Arshad, can we improve the patch or should I just disable those tests?

Comment 18

[Richard Marti (:Paenglab)]

Should we really rename every richlistbox to xbl-richlistbox or only the ones that need to extend the richlistbox?

Comment 19

[Arshad Khan [:arshad]]

(In reply to Richard Marti (:Paenglab) from comment #18)
> Should we really rename every richlistbox to xbl-richlistbox or only the
> ones that need to extend the richlistbox?

one upon which xbl binding is applied.. I wasn't aware of this.

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Geoff Lankow (:darktrojan) from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > Forking the binding and then updating consumers to inherit from the forked
> > one is doable (although behavior with both XBL and CE attached to an element
> > is undefined) and could be done now.
> > 
> > Combining it with a quick fix in the custom element to skip the call to
> > `define` in Thunderbird (I guess if AppConstants.MOZ_APP_NAME is
> > "thunderbird" or something) would get rid of the undefined behavior and fix
> > it (this is basically a better option 3 from
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1472558#c4).
> 
> Yes, please do that for now Brian. I'll fork the binding until we can fix
> this properly.

Alright, there's a patch up that does this at: https://phabricator.services.mozilla.com/D15698. But it's not clear to me if you still want it now - are you planning to fork and keep the tag name or go with a new tag name?

Comment 21

[Arshad Khan [:arshad]]

Attachment: richlistbx.patch [checked in in comment 25]

In this approach, we use xbl richlist box only for the bindings that are inheriiting it while the other instancesof richlistbox will use fx's richlistbox. Hopefully this might solve our test crisis.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5339bfa01f51c4a8fd7fe5b1f362bbfd1d5c23b9

Comment 22

[Arshad Khan [:arshad]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> (In reply to Geoff Lankow (:darktrojan) from comment #7)
> > (In reply to Brian Grinstead [:bgrins] from comment #6)
> > > Forking the binding and then updating consumers to inherit from the forked
> > > one is doable (although behavior with both XBL and CE attached to an element
> > > is undefined) and could be done now.
> > > 
> > > Combining it with a quick fix in the custom element to skip the call to
> > > `define` in Thunderbird (I guess if AppConstants.MOZ_APP_NAME is
> > > "thunderbird" or something) would get rid of the undefined behavior and fix
> > > it (this is basically a better option 3 from
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=1472558#c4).
> > 
> > Yes, please do that for now Brian. I'll fork the binding until we can fix
> > this properly.
> 
> Alright, there's a patch up that does this at:
> https://phabricator.services.mozilla.com/D15698. But it's not clear to me if
> you still want it now - are you planning to fork and keep the tag name or go
> with a new tag name?

Lemme see how the above try run turns out and then I ll let you know which approach are we going for.

Comment 23

[Jorg K (CEST = GMT+2)]

let bindingsStylehseet ;-)

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9034438 [details] [diff] [review]
richlistbx.patch [checked in in comment 25]

I'll land this after fixing the typo. Still two different test failures which didn't happen with the other patch:
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/cloudfile/test-cloudfile-manager.js | test-cloudfile-manager.js::test_load_accounts_and_properly_order
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/cloudfile/test-cloudfile-manager.js | test-cloudfile-manager.js::test_external_link

Running it manually I think this change helps:
https://hg.mozilla.org/try-comm-central/rev/57a38059b3f2c971183b0f9b11912de2fdd3daa1#l36.33
but also doesn't make it pass.

Aceman, can you get these two to pass?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c39f35e517ff
Port bug 1472558: fork richlistbox.xml until we have our code converted. rs=bustage-fix,jorgk

Comment 26

[Brian Grinstead [:bgrins]]

I'm assuming https://phabricator.services.mozilla.com/D15698 should still land? With the fix in Comment 25 you are still running richlistbox with both XBL and CE attached to it, correct?

Comment 27

[Richard Marti (:Paenglab)]

I think we don't use both in a richlistbx. All elements that extends to richlistbox use now the forked xbl-richlistbox. And the normal richlistbox we use as a normal richlistbox use the CE one.

Or are we assuming wrong that we splitted them with this approach?

Comment 28

[Jorg K (CEST = GMT+2)]

Are we attaching an old XBL richlistitem to the new CE richlistbox? Is that what D15698 is about?

Comment 29

[Richard Marti (:Paenglab)]

richlistitem is still XBL on m-c. https://searchfox.org/mozilla-central/source/toolkit/content/widgets/richlistbox.xml#15

Comment 30

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #28)
> Are we attaching an old XBL richlistitem to the new CE richlistbox? Is that
> what D15698 is about?

This does two things:

1) Expose the base richlistbox class to the global so that you can extend it for migrations from XBL to CE.
2) Don't register richlistbox as a CE in TB. This is to avoid attaching both the xbl-richlistbox XBL binding _and_ the richlistbox Custom Element on the same node.

The behavior for a node having both XBL and a CE attached is totally undefined - although if it's working fine for you with the fix from Comment 25 then maybe you don't need (2). I'm assuming you'll want (1) either way in order to remove the forked bindings (although not as a bustage fix). Let me know what you want to land.

Comment 31

[Jorg K (CEST = GMT+2)]

Thanks, Brian. The timing is really bad here. Magnus who has been overseeing the de-XBL work is on leave this week. He'll be back on Monday. My feeling is that it's preferable without the temporary M-C fix if we can hang on otherwise for a while. Thanks for preparing it though!

I wish I could comment on 2) in a qualified way. I see it like Richard said in comment #27. In general we do already use the new CE richlistbox. In a few spots we're using the forked xbl-richlistbox and extend it. So are there cases where both are attached to a node?

Comment 32

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #31)
> I wish I could comment on 2) in a qualified way. I see it like Richard said
> in comment #27. In general we do already use the new CE richlistbox. In a
> few spots we're using the forked xbl-richlistbox and extend it. So are there
> cases where both are attached to a node?

AFAICT based on https://hg.mozilla.org/comm-central/rev/c39f35e517ff:

(a) Normal richlistboxes will use the m-c Custom Element version
(b) Inherited bindings will still be applied to tags with the name of `richlistbox`. The binding name itself (xbl-richlistbox vs richlistbox) doesn't matter - what matters is the CSS selector that applies the binding to nodes. So for those elements, they will have both the Custom Element and the inherited binding attached.

If (a) is correct, then yeah, I don't think we should land part (2) from Comment 30. And part (1) can definitely wait.

Comment 33

[:aceman]

Attachment: 1517040.patch cloudFileView [checked in in comment 37, backed out in 46]

This finishes Jorg's idea and makes the cloud list work.
But it is unstyled (e.g. no border) so something is missing.
But I do not feel this is right, I don't know why this particular list has to be the old xbl-richlistbox.

Comment 34

[Richard Marti (:Paenglab)]

Because there don't exist styles for a xbl-richlistbox element.

Comment 35

[:aceman]

The test without the patch fails here:
JavaScript error: mozilla/comm/mail/test/mozmill/shared-modules/test-cloudfile-helpers.js, line 103: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this.selectedItems is undefined" {file: "chrome:/
/global/content/elements/richlistbox.js" line: 159}]'[JavaScript Error: "this.selectedItems is undefined" {file: "chrome://global/content/elements/richlistbox.js" line: 159}]' when calling method: [nsIRequestObserver::onStopRequest]

So it may be we do not need to use the old xbl binding, just find out what is wrong here with the new custom element.

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 9034493 [details] [diff] [review]
1517040.patch cloudFileView [checked in in comment 37, backed out in 46]

Those are a few hunks from Arshad's original patch:
https://hg.mozilla.org/try-comm-central/rev/57a38059b3f2c971183b0f9b11912de2fdd3daa1#l20.13

However, he had
<?xml-stylesheet href="chrome://messenger/content/bindings.css"?>
in aboutDownloads.xul:
https://hg.mozilla.org/try-comm-central/rev/57a38059b3f2c971183b0f9b11912de2fdd3daa1#l31.9

That may be preferred. I'll try it.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/150da20417f3
use old xbl-richlistbox for cloudFileView list. rs=bustage-fix,jorgk

Comment 38

[:aceman]

For those interested, the code populating the richlistbox (that fails in the test) is at https://searchfox.org/comm-central/source/mail/components/preferences/applications.js#530 and then https://searchfox.org/comm-central/source/mail/components/preferences/applications.js#557 .

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: backout-temp-fixes.patch

Backout of the two landed patches created with
  hg qbackout -s -r c39f35e517ff -r 150da20417f3

Might be handy for developers working on the bug.

Comment 40

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/49f231df7030
Backed out changeset c6f13700602b to re-enable Nightly builds . a=jorgk

Comment 41

[Arshad Khan [:arshad]]

(In reply to Brian Grinstead [:bgrins] from comment #30)
> (In reply to Jorg K (GMT+1) from comment #28)
> > Are we attaching an old XBL richlistitem to the new CE richlistbox? Is that
> > what D15698 is about?
> 
> This does two things:
> 
> 1) Expose the base richlistbox class to the global so that you can extend it
> for migrations from XBL to CE.
> 2) Don't register richlistbox as a CE in TB. This is to avoid attaching both
> the xbl-richlistbox XBL binding _and_ the richlistbox Custom Element on the
> same node.
> 
> The behavior for a node having both XBL and a CE attached is totally
> undefined - although if it's working fine for you with the fix from Comment
> 25 then maybe you don't need (2). I'm assuming you'll want (1) either way in
> order to remove the forked bindings (although not as a bustage fix). Let me
> know what you want to land.

Brian, the richlistbox.patch copies xbl binding to cc and consumers inherit that binding while all the other places where richlistbox is used, i am using m-c richlistbox. how do you like this option? I went for this one so that nsxulelement.cpp code also works and we also get bindings that inherits from old xbl binding, work.

If I understood right, part1 of ur patch D15698 makes richlistbox public so that we can extend it in future so we surely want that but the other part that stops richlistbox custom element for getting defined in TB is not required as of now.

Comment 42

[Brian Grinstead [:bgrins]]

(In reply to Arshad Khan [:arshad] from comment #41)
> (In reply to Brian Grinstead [:bgrins] from comment #30)
> > (In reply to Jorg K (GMT+1) from comment #28)
> > > Are we attaching an old XBL richlistitem to the new CE richlistbox? Is that
> > > what D15698 is about?
> > 
> > This does two things:
> > 
> > 1) Expose the base richlistbox class to the global so that you can extend it
> > for migrations from XBL to CE.
> > 2) Don't register richlistbox as a CE in TB. This is to avoid attaching both
> > the xbl-richlistbox XBL binding _and_ the richlistbox Custom Element on the
> > same node.
> > 
> > The behavior for a node having both XBL and a CE attached is totally
> > undefined - although if it's working fine for you with the fix from Comment
> > 25 then maybe you don't need (2). I'm assuming you'll want (1) either way in
> > order to remove the forked bindings (although not as a bustage fix). Let me
> > know what you want to land.
> 
> Brian, the richlistbox.patch copies xbl binding to cc and consumers inherit
> that binding while all the other places where richlistbox is used, i am
> using m-c richlistbox. how do you like this option? I went for this one so
> that nsxulelement.cpp code also works and we also get bindings that inherits
> from old xbl binding, work.
> 
> If I understood right, part1 of ur patch D15698 makes richlistbox public so
> that we can extend it in future so we surely want that but the other part
> that stops richlistbox custom element for getting defined in TB is not
> required as of now.

Alright, spun out part 1 into Bug 1517981 and dropped part 2.

Comment 43

[Jorg K (CEST = GMT+2)]

Note that switching the cloud file provider list to xbl-richlistbox makes the test cloudfile/test-cloudfile-manager.js pass, but one can't select a cloudfile provider now. See comment #35 and bug 1519653.

Comment 44

[Jorg K (CEST = GMT+2)]

More precisely: Comment #35 and bug 1519653 show the same error. I have a patch for bug 1519653 and with that patch we can switch to the normal richlistbox here and the test passes.

Comment 45

[Jorg K (CEST = GMT+2)]

For the record: When bug 1519653 lands, we can back out https://hg.mozilla.org/comm-central/rev/150da20417f3.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3ca205e21c25
Backed out changeset 150da20417f3 to return cloudFileView list back to richlistbox after bug 1519653. a=jorgk DONTBUILD

Comment 47

[Richard Marti (:Paenglab)]

Attachment: 1517040-no-stylesheet-inject.patch [landed in comment 48]

The bindings.css injection into the Addons Manager seems not needed. We have nothing forked here that needs this.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8e0c009b6d37
Remove unneeded bindings.css inject in aboutAddonsExtra.js. r=darktrojan

Comment 49

[Jorg K (CEST = GMT+2)]

(In reply to Pulsebot from comment #25)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c39f35e517ff
Port bug 1472558: fork richlistbox.xml until we have our code converted.
rs=bustage-fix,jorgk

I have no idea why the fork we created on 2019-01-04 did not include the changes M-C made to that file four days before here:
https://hg.mozilla.org/mozilla-central/rev/8b2453d488ae#l5.2

Looks like we copied an old version :-( - I filed bug 1521309 to correct that.

Comment 50

[Jorg K (CEST = GMT+2)]

So for the record: What is left to do here is to convert the xbl-richlistbox.

Comment 51

[Magnus Melin [:mkmelin]]

The work to be done is tracked in the dependencies.

Comment 52

[Jorg K (CEST = GMT+2)]

How are we going with this? Lately some attachment tests are failing, see here for disabled tests:
https://hg.mozilla.org/comm-central/rev/6210f90e0832ae852976c7d39fdaa4d087bea406
Maybe I disabled them in the wrong bug.

Currently we have a crash in the MozMill Z1 run in the attachment area, which takes the entire directory and those which follow down with:

TEST-UNEXPECTED-FAIL | Disconnect Error: Application unexpectedly closed
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run

You can see ti by clicking on a "Z1". Here is a log:
https://taskcluster-artifacts.net/KCNcnCe2SQG9C39Uh4-Ejg/0/public/logs/live_backing.log

It has lots of the errors already reported in bug 1526699:
JavaScript error: chrome://messenger/content/richlistbox.xml, line 857: TypeError: this.currentItem._fireEvent is not a function

I'm not sure whether that's related to the current bustage. I haven't been able to pin-point the failure yet.

Anyway, I can't see any progress here.

Comment 53

[Jorg K (CEST = GMT+2)]

Sorry, got confused, the disabled tests are about attachment reminders. Never mind, something fishy in the attachment area.

Comment 54

[Magnus Melin [:mkmelin]]

How are we going with this?

Still bug bug 1522472 and bug 1523607 to go.

Comment 55

[Axel Grude]

I have some problems with the missing insertItemAt method - is there any workaround function that can be used with richlistbox? I need this functionality for my quickFilters Add-on so I can keep supporting multi-select cut / paste / copy / alphabetic sorting, now that listbox is gone.

Comment 56

[Jorg K (CEST = GMT+2)]

https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_63
XUL element method .insertItemAt(), replacement: .insertBefore().