Closed Bug 1517040 Opened 6 years ago Closed 4 years ago

Port Bug 1472558 Convert "richlistbox" to Custom Element

Categories

(Thunderbird :: Upstream Synchronization, enhancement)

Product:
Thunderbird
Component:
Upstream Synchronization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr78 unaffected)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0
Tracking Flags:
Tracking Status
thunderbird_esr78 --- unaffected

People

(Reporter: jorgk-bmo, Assigned: arshad)

References

Details

(Keywords: leave-open, Whiteboard: [drop xbl-richlistbox])

Attachments

(3 files, 4 obsolete files)

richlistbx.patch [checked in in comment 25]
(deleted), patch
jorgk-bmo
: feedback+
Details | Diff | Splinter Review
1517040.patch cloudFileView [checked in in comment 37, backed out in 46]
(deleted), patch
jorgk-bmo
: feedback+
Details | Diff | Splinter Review
1517040-no-stylesheet-inject.patch [landed in comment 48]
(deleted), patch
darktrojan
: review+
Details | Diff | Splinter Review
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)
Well, Magnus is away this week. I know Geoff loves listboxes ;-)
Flags: needinfo?(geoff)
Assignee: nobody → arshdkhn1
Flags: needinfo?(arshdkhn1)
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.
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
Attached patch 1517040-richlist-binding-1.diff (obsolete) (deleted) — — Splinter Review
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.
Attached patch xbl-richlistbox.patch (obsolete) (deleted) — — Splinter Review
I am changing all the instances of richlistbox with xbl-richlistbox so that there is no name conflict.
Flags: needinfo?(arshdkhn1)
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
Closed: 6 years ago
Resolution: --- → FIXED
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Keywords: leave-open
Resolution: FIXED → ---
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Looking promising. Can you check why the remaining tests are failing and fix them? Or should I just disable them?
Looks like Mozmill downloads/test-about-downloads.js only fails on Mac. Works here on Windows, and works on the tree on Linux.
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?
(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)
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
(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)
let bindingsStylehseet ;-)
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+
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?
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.
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.
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.
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 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+
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/150da20417f3 use old xbl-richlistbox for cloudFileView list. rs=bustage-fix,jorgk
Attached patch backout-temp-fixes.patch (obsolete) (deleted) — — 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
Backout by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/49f231df7030 Backed out changeset c6f13700602b to re-enable Nightly builds . a=jorgk
(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.
Attachment #9034493 - Attachment description: 1517040.patch cloudFileView → 1517040.patch cloudFileView [checked in in comment 37)
Attachment #9034438 - Attachment description: richlistbx.patch → richlistbx.patch [checked in in comment 25]
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.

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.

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.

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

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+

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
Attachment #9034534 - Attachment is obsolete: true
Attachment #9034493 - Attachment description: 1517040.patch cloudFileView [checked in in comment 37) → 1517040.patch cloudFileView [checked in in comment 37, backed out in 46]
Attachment #9036843 - Attachment description: 1517040-no-stylesheet-inject.patch → 1517040-no-stylesheet-inject.patch [landed in comment 48]

(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

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
Blocks: 1526699

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)

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 bug 1523607 to go.

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

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.

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

Component: General → Upstream Synchronization
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 4 years ago
status-thunderbird_esr78: --- → unaffected
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: