Port Bug 1472558 Convert "richlistbox" to Custom Element

ASSIGNED
Assigned to

Status

enhancement
ASSIGNED
5 months ago
a month ago

People

(Reporter: jorgk, Assigned: arshad)

Tracking

(Blocks 1 bug, {leave-open})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [drop xbl-richlistbox])

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

5 months ago
See bug 1472558 comment #4.
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(arshdkhn1)
I hope, Arshad or Magnus can convert it before it lands. All I could do is to fork the binding.
Flags: needinfo?(richard.marti)
Reporter

Comment 2

5 months ago
Well, Magnus is away this week. I know Geoff loves listboxes ;-)
Flags: needinfo?(geoff)
Assignee

Updated

5 months ago
Assignee: nobody → arshdkhn1
Flags: needinfo?(arshdkhn1)
Assignee

Updated

5 months ago
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)
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.
Flags: needinfo?(arshdkhn1)
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.
Reporter

Comment 5

5 months ago
I guess we'll be busted :-(
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).
(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.
Flags: needinfo?(bgrinstead)
Attachment #9034315 - Attachment description: Bug 1517040 - Allow Thunderbird to continue using richhlistbox as XBL, and to extend the Custom Element class;r=paolo → Bug 1517040 - Allow Thunderbird to continue using richlistbox as XBL, and to extend the Custom Element class;r=paolo
We're busted now, but when the other patch lands on m-c, we can push this to c-c for a temporary fix.
There's surprisingly little broken. I think we could get away without deleting our bindings, and putting the needed code elsewhere.
Assignee

Comment 11

5 months ago
Posted patch xbl-richlistbox.patch (obsolete) — Splinter Review
I am changing all the instances of richlistbox with xbl-richlistbox so that there is no name conflict.
Flags: needinfo?(arshdkhn1)

Comment 13

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c6f13700602b
No bug - Disable Nightly builds until richlistbox issue is fixed. a=jorgk
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Reporter

Updated

5 months ago
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Keywords: leave-open
Resolution: FIXED → ---
Reporter

Updated

5 months ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Reporter

Comment 15

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

Comment 16

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

Comment 17

5 months ago
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?
Flags: needinfo?(arshdkhn1)
Flags: needinfo?(acelists)
Should we really rename every richlistbox to xbl-richlistbox or only the ones that need to extend the richlistbox?
Assignee

Comment 19

5 months ago
(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.
(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?
Flags: needinfo?(bgrinstead)
Assignee

Comment 21

5 months ago
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
Assignee

Comment 22

5 months ago
(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.
Flags: needinfo?(arshdkhn1)
Reporter

Comment 23

5 months ago
let bindingsStylehseet ;-)
Reporter

Comment 24

5 months ago
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?
Attachment #9034438 - Flags: feedback+

Comment 25

5 months ago
Pushed by mozilla@jorgk.com:
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'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?
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?
Reporter

Comment 28

5 months ago
Are we attaching an old XBL richlistitem to the new CE richlistbox? Is that what D15698 is about?
(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.
Reporter

Comment 31

5 months ago
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?
(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

5 months ago
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.
Flags: needinfo?(acelists)
Attachment #9034493 - Flags: review?(jorgk)
Because there don't exist styles for a xbl-richlistbox element.

Comment 35

5 months ago
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.
Reporter

Comment 36

5 months ago
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.
Attachment #9034493 - Flags: review?(jorgk) → feedback+

Comment 37

5 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/150da20417f3
use old xbl-richlistbox for cloudFileView list. rs=bustage-fix,jorgk
Reporter

Comment 39

5 months ago
Posted patch backout-temp-fixes.patch (obsolete) — Splinter Review
Backout of the two landed patches created with
  hg qbackout -s -r c39f35e517ff -r 150da20417f3

Might be handy for developers working on the bug.
Attachment #9034335 - Attachment is obsolete: true
Attachment #9034372 - Attachment is obsolete: true

Comment 40

5 months ago
Backout by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/49f231df7030
Backed out changeset c6f13700602b to re-enable Nightly builds . a=jorgk
Assignee

Comment 41

5 months ago
(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.

Updated

5 months ago
Attachment #9034493 - Attachment description: 1517040.patch cloudFileView → 1517040.patch cloudFileView [checked in in comment 37)

Updated

5 months ago
Attachment #9034438 - Attachment description: richlistbx.patch → richlistbx.patch [checked in in comment 25]
See Also: → 1517981
Attachment #9034315 - Attachment is obsolete: true
(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.
Reporter

Comment 43

4 months ago

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.

Reporter

Comment 44

4 months ago

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.

Reporter

Comment 45

4 months ago

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

Comment 46

4 months ago
Pushed by mozilla@jorgk.com:
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

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

Attachment #9036843 - Flags: review?(geoff)
Attachment #9036843 - Flags: review?(geoff) → review+

Comment 48

4 months ago

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/8e0c009b6d37
Remove unneeded bindings.css inject in aboutAddonsExtra.js. r=darktrojan

Keywords: checkin-needed
Reporter

Updated

4 months ago
Attachment #9034534 - Attachment is obsolete: true
Reporter

Updated

4 months ago
Attachment #9034493 - Attachment description: 1517040.patch cloudFileView [checked in in comment 37) → 1517040.patch cloudFileView [checked in in comment 37, backed out in 46]
Reporter

Updated

4 months ago
Attachment #9036843 - Attachment description: 1517040-no-stylesheet-inject.patch → 1517040-no-stylesheet-inject.patch [landed in comment 48]
Reporter

Comment 49

4 months ago

(In reply to Pulsebot from comment #25)

Pushed by mozilla@jorgk.com:
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.

Whiteboard: [drop xbl-richlistbox]
Depends on: 1523604
Depends on: 1523600
Depends on: 1523607
Reporter

Comment 50

4 months ago

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

Depends on: 1524512

The work to be done is tracked in the dependencies.

Depends on: 1521733, 1522472

Updated

3 months ago
Blocks: 1526699
Reporter

Comment 52

3 months ago

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.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(arshdkhn1)
Reporter

Comment 53

3 months ago

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

How are we going with this?

Still bug bug 1522472 and 1523607 to go.

Flags: needinfo?(mkmelin+mozilla)
Assignee

Updated

2 months ago
Flags: needinfo?(arshdkhn1)

Comment 55

a month ago

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.

Reporter

Comment 56

a month ago

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

You need to log in before you can comment on or make changes to this bug.