1434641 - Get rid of nsITreeBoxObject

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hrm.  Nevermind, Thunderbird actually has a JS impl of nsITreeBoxObject (!), so this is not possible for the moment...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a way to get a TreeBoxObject from a BoxObject

MozReview-Commit-ID: IETJsECWAFv

Comment 3

[Boris Zbarsky [:bzbarsky]]

And unfortunately, the methods I want to remove are even no-ops in the tbird impl, but that doesn't help me any...

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Thanks for keeping Thunderbird in mind. A brief look indicates that code might be difficult to migrate off of. Looks like asuth had written that code at the time, maybe he can make a suggestion how we would get rid of it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Thanks for keeping Thunderbird in mind.

Well, I do use it... ;)

This is not seriously blocking me so far; there are other things preventing bug 
1132934 from landing at the moment.  I'll think about this one some more once I fix those.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Philipp Kewisch [:Fallen]  from comment #4)
> Thanks for keeping Thunderbird in mind. A brief look indicates that code
> might be difficult to migrate off of. Looks like asuth had written that code
> at the time, maybe he can make a suggestion how we would get rid of it.

I think the comments on the implementation[1] and its friend JSTreeSelection[2] cover it pretty well:
/**
 * Implement a fake nsITreeBoxObject so that we can keep the view
 *  nsITreeSelection selections 'live' when they are in the background.  We need
 *  to do this because nsTreeSelection changes its behaviour (and gets ornery)
 *  if it does not have a box object.
 * This does not need to exist once we abandon multiplexed tabbing.
 *
 * Sometimes, nsTreeSelection tries to turn us into an nsIBoxObject and then in
 *  turn get the associated element, and then create DOM events on that. The
 *  only event that we care about is onselect, so we get a DOM node here (with
 *  an event listener for onselect already attached), and pass its boxObject in
 *  whenever nsTreeSelection QIs us to nsIBoxObject.
 */

"Multiplexed tabbing" is the hack where all 3-pane/mail tabs are actually the single set of XUL widgets from the pre-tab world pretending to be N different tabs.  That's why the message reader pane likes to reset its position when you change tabs.

The most correct, probably most hard fix is to finish the change-over.  In theory, all the abstractions introduced in folderDisplay.js and friends were supposed to make it easier to do that by exposing a sane multi-tab-friendly API somewhat decoupled from the nsMsgDBView implementations.

There are probably some number of lesser hacks available like just keeping multiple XUL tree widgets around or more native objects.  A lot of that refactoring effort was just attempting to reverse-engineer, document, add tests for, and then fix the test failures and inconsistencies of the logic that came before the refactoring.  That the hack is done that way doesn't mean it's the best possible hack.

1: https://searchfox.org/comm-central/source/mail/base/content/folderDisplay.js#2620
2: https://searchfox.org/comm-central/source/mailnews/base/util/jsTreeSelection.js#14

Comment 7

[Magnus Melin [:mkmelin]]

If I understand this correctly, we (Thunderbird) would be able to remove this dependency if we drop support for having more than one 3-pane tab open at any time?

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

To recap: All 3-pane and message-display tabs in Thunderbird are really multiplexing over a single set of widgets.  When you switch tabs, the message body is re-streamed, and selections are updated in the folder pane and thread pane.  This is why the message position may reset when switching tabs.  The FakeTreeBoxObject discussed in comment 6 was a very specific hack to get this multiplexing to work with minimal C++ changes and minimize potential state churn due to selection events being generated as a side-effect, etc.

I see a few options:
1. Eliminate multiplexing so that each tab gets its own set of widgets rather than sharing widgets.  This potentially breaks things, especially extensions using pre-FolderDisplay abstractions.
2. Change to a different hack.  In general I was attempting to clean up, document, and add tests for what Thunderbird was already doing, so I tried to not to make massive overhauls at the same time.  Now that there is test coverage of all the selection stuff, etc., I think it's reasonable to consider less hacky solutions to the multiplexing.  (However, it was always my/our goal to eliminate the multiplexing, so I didn't look much further once I got things working.)
3. Eliminate multiplexing by not supporting multiple 3-pane/message tabs.  This seems like overkill.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Uh, and I would recommend option 1 or 2.

Comment 10

[Magnus Melin [:mkmelin]]

Thanks for the input! Just to know the likely alternatives - for the immediate issue, do you see it as a possible solution to not support 3-pane in tab? I would assume the single-message in tab doesn't really need JSTreeSelection (but maybe it does?)

We're going to have to adjust some parts quickly at any rate with bug 1482389 nearing completion.

Comment 11

[Magnus Melin [:mkmelin]]

Eh, should have been ... "Do you see it as a a possible solution not to support multiple 3-pane tabs" ...
But still support single message in tab.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

In single message mode, there's still a backing nsMsgDBView thing that you can navigate using "f" and "b" and all that.  Looking at https://searchfox.org/comm-central/source/mail/base/content/mailTabs.js#347 and the little bit of code below it, I think the full path is all hooked up there.  I was remembering something about a special mode for single messages, but I think that's for the pop-out single message window that may or may not still exist?

Comment 13

[Jorg K (CEST = GMT+2)]

I'm confused, the bug is called "Get rid of nsITreeBoxObject". Well, it has already happened, or will happen again once relanded: Bug 1482389:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd20e420f257#l51.1

I'm trying to port that stuff in bug 1518823, but I ran into trouble with the "fake" stuff, see bug 1518823 comment #11.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yes, bug 1482389 is going to fix this bug by removing nsITreeBoxObject.

And yes, it will break Thunderbird by that removal, for precisely the reasons described in this bug. See bug 1482389 comment 18.