I applied
https://hg.mozilla.org/integration/mozilla-inbound/rev/af06731b5203
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd20e420f257
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1cfc595892f
and got compile errors, so far only
#include "nsITreeBoxObject.h" not found.

Looks like we also need to do this:

•  tree.treeBoxObject.ensureRowIsVisible(i);
  

•  tree.ensureRowIsVisible(i);
  

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd20e420f257#l14.75

in a bunch of places:
https://searchfox.org/comm-central/search?q=treeBoxObject.ensureRowIsVisible&case=false&regexp=false&path=

What about this:
https://searchfox.org/comm-central/search?q=treeBox.ensureRowIsVisible&case=false&regexp=false&path=

Wow, that code got interpreted as formatting :-(

-      tree.treeBoxObject.ensureRowIsVisible(i);
+      tree.ensureRowIsVisible(i);

This appears to compile so far, I commented out three lines with XXX that resisted compilation. Needs further looking at.

This compiles, and no more commented-out code :-)

Over to the JS part. I'm going to do what I said in comment #2, so s/treeBoxObject.ensureRowIsVisible/ensureRowIsVisible/.

Paolo, what about treebox.ensureRowIsVisible()? That's something else? Sorry, pattern-matchers at work ;-)

Wow, the JS part is going to be massive, all access via .treeBoxObject needs to go.

Magnus, we have a problem here. nsITreeBoxObject is removed and we still have it in FakeTreeBoxObject in folderDisplay.js.

JS patch coming in a minute.

This is what I have so far. The folder tree displays, but the thread pane/message list doesn't.

I see:
this.treeBox.getFirstVisibleRow is not a function folderDisplay.js:1742
TypeError: treeBox.getFirstVisibleRow is not a function folderDisplay.js:2473:17

OK, with this patch, the thread pane has returned.

I've also looked at
https://searchfox.org/comm-central/search?q=treeBox.ensureRowIsVisible&case=false&regexp=false&path=

'treeBox' was an object property here. I've renamed it to 'tree'.

Oops, never sent this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=51ddd5d04999b44262dec60685c7e7da1a506d21

It's totally red. Locally I see:
JavaScript error: chrome://messenger/content/messageWindow.js, line 105: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsITreeView.setTree]

Also, Geoff pointed out that there is treeBoxObject in XML files :-(

Grrr, messageWindow.js, line 105 is that fake stuff that is biting us now :-(

this.view.dbView.setTree(this._fakeTreeBox);

OK, this elimitates treeBoxObject from mailWidgets.xml. I don't think it will make test better, but we can see:

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

For the "fake stuff", take a look at bug 1434641 comment #6 downwards.

OK, had a stab at the fake stuff:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=21f9b94d52279b95fac2d46f07be1ec42df27361

I still see
NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsITreeView.setTree] folderDisplay.js:1776

Looks like the removal of nsITreeBoxObject also took away the possibility of simulating a fake tree in JS, right?

OK, now with:
1776: // this.view.dbView.setTree(this._fakeTree);

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4455b52bda0236de9dcb19b0cb0e4e0a2b54bed2

And another one with folderDisplay.js also
105: // this.view.dbView.setTree(this._fakeTree);

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

(In reply to Jorg K (GMT+1) from comment #15)

I still see
NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsITreeView.setTree] folderDisplay.js:1776

Looks like the removal of nsITreeBoxObject also took away the possibility of simulating a fake tree in JS, right?

Does your change fake object QI to the right interface?

No, it doesn't, see comment #6. It's the:
https://searchfox.org/comm-central/rev/58f568fcebd755ece1b6c6622c45a62decd481d7/mail/base/content/folderDisplay.js#2662

But with the removal of nsITreeBoxObject, the XPCOM-ness is gone, no? What should I QI to?

The IDL says XULTreeElement: https://hg.mozilla.org/try/rev/59b0eb21a4fb#l53.31

But how you do that and whether it actually matters or not, I don't know.

I also get:
JavaScript error: chrome://messenger/content/threadPane.js, line 70: TypeError:
col.value is undefined
JavaScript error: chrome://messenger/content/threadPane.js, line 64: TypeError:
col.value is undefined

That's from tree.getCellAt(event.clientX, event.clientY, row, col, elt);

(In reply to Geoff Lankow (:darktrojan) from comment #20)

The IDL says XULTreeElement: https://hg.mozilla.org/try/rev/59b0eb21a4fb#l53.31
But how you do that and whether it actually matters or not, I don't know.

Yes, that let's me remove the
// this.view.dbView.setTree(this._fakeTree);

Let's try again, but the errors from comment 21 don't help :-(
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=43f09c2ebcf540656adf12af734c3cebe141aa82

oops, we need to replace the getCellAt() call:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af06731b5203#l2.15

OK, fixed the QI and all the getCellAt() calls.

If I throw this in, the thread pane shows and I can open a message in a tab and a new window. Closing the tab crashes, but hey, you can't have everything.

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

Fixed another typo in a separate patch:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2c0781e246e6b9d912197eaefb53bf7c34a59f37

Note: Since this is not based on "normal" M-C but on a try changeset, the jobs can't be cancelled :-( - well, or only individually with the TaskCluster tool.

Fixed another one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b994f90aa3173ecae2c58cff599c51070648aab8

OK, I've folded the misc. fixes into the main patch here.

With disable-fake.patch the try had three of the four Z's green (modulo the pesky testTaskView.js).

Z2 is red with multiple failures in four tests:
folder-display/test-deletion-from-virtual-folders.js
folder-display/test-deletion-with-multiple-displays.js
folder-display/test-folder-pane-visibility.js
folder-display/test-pane-focus.js
folder-display/test-close-window-on-delete.js

Magnus, what's the way forward here?

Sigh, another two call sites of getCellAt() needed fixing. Here's another try with the fake stuff disabled:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=717849595eb4a84c7f00169b3bc1e9b4bfdf8ecb

Looking, but doesn't look like fun.

Looks like the try form comment #29 is worse than the one from comment #27 :-(

Comment on attachment 9035386 [details] [diff] [review]
1518823-nsITreeBoxObject.patch (v1)

(In reply to Jorg K (GMT+1) from comment #4)

Paolo, what about treebox.ensureRowIsVisible()?

What's been done in this bug seems alright. Methods like this one that were on nsITreeBoxObject are now called on the <tree> element. As for FakeTreeBoxObject, probably you have to look into why it exists and whether there's an alternative way of achieving the same effect. In some cases I found that complex code could be converted to a one-liner in the original XBL binding, and I think we can consider a patch for a solution like this for the <tree> element if necessary.

Sadly there are too many bugs involved in this area. In M-C we have bug 1434641 and bug 1482389, in C-C we have this bug here and also bug 1514624 which removed a snippet.

The main reading re. the "fake stuff" is in bug 1434641 comment #6 and further down (cmt 8 and 12).

Comment on attachment 9035386 [details] [diff] [review]
1518823-nsITreeBoxObject.patch (v1)

This will have to land soon.

This is sadly necessary.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8015b21deff6
No bug - Disable Nightly builds until XUL tree issue is fixed. a=jorgk DONTBUILD

BTW, message display in a tab works and "b", "f" and "n" (what does that do?) appear to work.

(In reply to Jorg K (GMT+1) from comment #38)

BTW, message display in a tab works and "b", "f" and "n" (what does that
do?) appear to work.

Go to previous, next, next unread message.

Andrew, thanks for the explanation in bug 1434641 comment #6 and further down.

Would it be possible to move the implementation of the "fake tree" to C++ code?
https://searchfox.org/comm-central/rev/a8aec0d10f547e8181430a41db331e710a5c9382/mail/base/content/folderDisplay.js#2588

As I understand the immediate problem, nsITreeBoxObject is no longer an XPCOM object, so the tree can't be implemented in JS any more, that's why we had to disable these assignments: https://hg.mozilla.org/comm-central/rev/561033e56bac

For the uninitiated, your comment six in the other bug is hard to digest since it refers to the "pre-tab" times and mentions a completing a "change-over".

Not the standard way to turn off tests, but this was quickest.

(In reply to Jorg K (GMT+1) from comment #40)

Would it be possible to move the implementation of the "fake tree" to C++ code?
https://searchfox.org/comm-central/rev/a8aec0d10f547e8181430a41db331e710a5c9382/mail/base/content/folderDisplay.js#2588

There's no reason the fake tree need be implemented in JS. It was just very easy and there were no scaling problems. If you can implement a C++ subclass and aren't going to run into problems where methods aren't virtual, then that does seem like a way to maintain the status quo.

As I understand the immediate problem, nsITreeBoxObject is no longer an XPCOM object, so the tree can't be implemented in JS any more, that's why we had to disable these assignments: https://hg.mozilla.org/comm-central/rev/561033e56bac

I'm not really following the work in the other bugs, just trying to provide context.

For the uninitiated, your comment six in the other bug is hard to digest since it refers to the "pre-tab" times and mentions a completing a "change-over".

Thunderbird used to not have any tabs. Then tabs were implemented as a major hack where the user thought they had N 3-pane UI's working as separate, parallel things. But in fact it's just one set of widgets having hacky things done to them when the tab changes. The Folder Display abstractions were created to normalize the hack and provide a (more) sane API to add-ons as a first step to changing-over to not having any hack.

I'm going to see whether changing the FakeTree implementation in folderDisplay.js to C++ is going to fix the immediate issue and maintain the status quo. That's a lot of boiler plate to be written :-(

I got confused and spent some time in the wrong boat :-(

FakeTree doesn't implement a "tree view" (nsITreeView) which we could extend, but the former nsITreeBoxObject which was replaced by the final class XULTreeElement:
https://searchfox.org/comm-central/rev/3d98ee9af358660c44644d24b738f17261165ca6/mozilla/dom/xul/XULTreeElement.h#29

So nothing can be derived from it and it mostly doesn't have virtual methods.

TB 66 beta 3:
https://hg.mozilla.org/releases/comm=beta/rev/df1d9106a00f4c8436a0256750ae9882a1c6c478

EDIT: Destroyed the correct links do the c-b landing doesn't prevent this from showing up in my uplift query.

Tests disabled here:
test-close-window-on-delete.js - 7
test-deletion-from-virtual-folders.js - 12
test-deletion-with-multiple-displays.js - 21

Geoff notices that 9 can be re-enabled since they pass now or never failed and were just a victim of hasty whole-sale disabling. Thanks Geoff. There will still be tests disabled in all three files.

Magnus, what's the way forward here? Can you put one person from your de-XBL team onto this job. Duplicates are piling up. The malfunction is that deletion of messages opened in a tab or stand-alone window doesn't behave as expected any more.

This isn't very polished and it may well cause other problems, but it does cause 20 previously-failing tests to pass.

I've done two Try runs with some retriggering and not had a green Z2 on Linux yet. (Green on Mac first time on both runs.) I don't think that is related, but it could be.

Thanks, Geoff.

I had a bit of trouble looking at those try runs, like this one: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0367a0aac8478088e5108a8e49a4a70cff8c6598

So I started my own:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e24022251ffeae43ec05cc6a774693e396f0b5f2

Ben I'd be interested to know if attachment 9067268 [details] [diff] [review] would solve your bug 1551973.

All tests fixed. This try run has very similar code, but I'll do another with the final changes just to make sure.

Wow, "all tests fixed" is referring to all the 31 tests, not just the 20 from the previous patch. Fantastic!!

Linux debug shows a failure (doesn't it always?…), but I'm pretty sure it's just doing things out-of-order and I can fix it by making the test wait for the expected value to appear.

Geoff: Thanks for working on this!

> +        setTimeout(() => {

Yes, please never use timeouts in production code. Esp. never ever as a workaround. They are fine in test code, but not in the real app code.

(In reply to Magnus Melin [:mkmelin] from comment #66)

I get an area under the status bar, which is apparently showing the fake
tree?

It should have a height of 0. Why doesn't it?

seems like an odd change to declare the shouldReturn. Isn't the only change
the this._deleteInProgress = false; addition?

_deleteInProgress needs to be false, after calling messageDisplay.onMessagesRemoved. You have a better way of doing it?

why the timeouts?

It seems like something things are happening out-of-order. At these points I think the selected message in the tree has changed but whatever the title-setting code needs is further down the event loop.

(In reply to Geoff Lankow (:darktrojan) from comment #68)

(In reply to Magnus Melin [:mkmelin] from comment #66)

I get an area under the status bar, which is apparently showing the fake
tree?

It should have a height of 0. Why doesn't it?

Can't say. Dev tools says the element is 46px, but is confused as to why. Using display: none hides it though.

When starting up with a few messages open in tabs, I got a few of these trees down there, and then some infinite jumping around.

why the timeouts?

It seems like something things are happening out-of-order. At these points I
think the selected message in the tree has changed but whatever the
title-setting code needs is further down the event loop.

Would be good to track this down, or at the minimum add a TODO item linking to a bug to do that, and some explanation.

(In reply to Magnus Melin [:mkmelin] from comment #69)

Can't say. Dev tools says the element is 46px, but is confused as to why. Using display: none hides it though.

Using display: none also stops it from working.

visibility: collapse seems to be okay. The extra tree view never showed for me but it did on TaskCluster, sometimes. Anyway, this works:

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

I asked earlier, but looking more, I suspect not since onMessagesRemoved checks that.

(In reply to Magnus Melin [:mkmelin] from comment #72)

::: mail/base/content/folderDisplay.js
@@ +1197,5 @@

onMessagesRemoved() {
FolderDisplayListenerManager._fireListeners("onMessagesRemoved",
[this]);

• let shouldReturn = this.messageDisplay.onMessagesRemoved();

maybe name it handled, instead of shouldReturn

Yeah go on, rename it. I'm not fussy.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8c7373a378b6
Replace FakeTree with a real tree and re-enable 31 disabled tests. r=mkmelin DONTBUILD

Landed with the variable renamed. I'll leave the target milestone at TB 66 since that's when the initial stuff landed. The final patch landed at TB 69 and will be backported to TB 68.

I've built trunk and verified that this indeed fixes bug 1551973. That was the worst regression in TB 68, and highly annoying, so thanks a lot for fixing this!

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/eac25078f845f5f26929ccaeb39c938623e6a666

when will this fix be available to those of us running 67.0b3 32 bit version

It will ship in 68.0 beta 1 next week.

Thank you for fixing this! I quit using Daily because of it. Drove me nuts. Now I will go back.