1737361 - (sn-utb-rebuild) [meta] Rebuild the toolbar to only have one dynamic implementation

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, task, P2)

Description

[Alessandro Castellani [:aleca]]

I'm not sure if this is technically doable, but maybe it's something worth exploring.

Rebuild our toolbar in order to have only one dynamic implementation
Currently, we have a dedicated toolbar for each tab. This is kinda necessary because each tab has different toolbarbuttons and different needs.

This approach falls short once we start needing to implement the same button on each toolbar. The most obvious example is the main app menu button, which we need to initialize multiple times for each toolbar location.

It also turns tedious when new buttons are implemented that need to be available in every toolbar or when new tabs are implemented (new address book missing the app menu button).

Proposal
We should have only one toolbar.
We should then find a decent mechanism that allows us to:

• Have all the available <toolbarbutton>s defined in one single file.
• Independently define a defaultset of toolbarbuttons available per tab.
• Independently define a set of all customizable toolbarbuttons available per tab.

What do you guys think?

Comment 1

[Richard Marti (:Paenglab)]

That would be great as we then can move the toolbar out of the tabmail-container into the navigation-toolbox.

Comment 2

[Martin Giger [:freaktechnik]]

Recently when investigating if I could figure out how to make the chat tab button not disabled in non-3pane tabs I came across the command controllers (for example https://searchfox.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js). these seem to already dynamically enable and disable commands depending on the tab. Whatever we do should probably at least be related to this, given that toolbarbuttons are often using commands as their handler.

Comment 3

[Alessandro Castellani [:aleca]]

I'm gonna take this and make it happen after bug 1736437 landed, since we need to have only 1 toolbar if we want to keep having proper background image styling for LW-Themes.

Comment 4

[Lasana Murray]

I'm thinking we should have some kind of registry service (non-xpcom) that keeps watch over the toolbar and changes the configuration on behalf of the respective areas instead of them implementing it themselves. The buttons could be generated here from lightweight configuration objects that are stored in a Map so we can easily switch between them by name.

Comment 5

[Alessandro Castellani [:aleca]]

I've been thinking a bit about this during the break and this is the conclusion I reached.

What's needed
A way to let each tab view define:

• A list of toolbar buttons.
• A list of currentset and defaultset to keep a working customization.
• A way to "use" an existing buttons set from another view.

The almost sure approach for this is to rely on the TabMonitor to react to tab changes.
On every tab change, we can see if the current tab needs a toolbar and which buttons we should show, and if needed we can update the UI to reflect that.

Using a <slot> element as general template
The general idea would see the introduction of a <slot> element, mimicking the VueJS Slots components https://vuejs.org/v2/guide/components-slots.html, which was inspired by the Web Components spec draft https://github.com/WICG/webcomponents/blob/gh-pages/proposals/Slots-Proposal.md.

The <slot> component would act kind of like a template, so not rendered in the page but used by the onTabSwitched() method to check if the current tab has it and what's inside.
This approach would allow us to write those toolbarbuttons directly in the XHTML, which maybe it makes it easier to maintain(?).

We could also introduce a data-attribute that can allow us to use a the same set of buttons from another <slot> by referencing its ID, in case multiple tabs have the same configuration.

A possible example could be something like this:

// Mail tab
<slot id="mail-toolbar" defaultset="...">
  <toolbarbutton/>
  <toolbarbutton/>
  <toolbarbutton/>
  ...
</slot>

// Calendar tab
<slot id="calendar-toolbar" defaultset="...">
  <toolbarbutton/>
  <toolbarbutton/>
  <toolbarbutton/>
  ...
</slot>

// Tasks tab
<slot data-slot="calendar-toolbar"/>

With this approach we can have:

• Each <slot> element living inside the tab they belong to.
• For those tabs that don't need a toolbar (preferences, account managers, setup, etc), we simply don't add a <slot>.
• JS will only handle clearing and repopulating the main toolbar when a tab is changed.
• We can still leverage the defaultset and currentset to maintain customization and store those changes in the XUL Storage.

Thoughts?

Comment 6

[Magnus Melin [:mkmelin]]

I'd think the toolbar belongs (is owned by) the tab though. Especially after Geoff's new 3pane work.
So there's no clearing/repopulating a main toolbar: the tab needs to render a toolbar the way it wants it, if it wants it.

Comment 7

[Geoff Lankow (:darktrojan)]

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

I'd think the toolbar belongs (is owned by) the tab though. Especially after Geoff's new 3pane work.
So there's no clearing/repopulating a main toolbar: the tab needs to render a toolbar the way it wants it, if it wants it.

I don't agree. While it seems nice conceptually it's not practical. It would require an implementation for every tab type with all of the problems that brings such as making the same change in multiple places. Adding a toolbar to the new Address Book was a nightmare and yet it still doesn't have some of the things it should have, like the app menu. Nor is it customisable which might not matter for the Address Book but will for other tab types.

We already have one menu bar for every tab, and nobody's suggesting that it be owned by the tabs.

Comment 8

[Martin Giger [:freaktechnik]]

Attachment: Bug 1737361 - Add unified toolbar to messenger window. r=aleca

Comment 9

[Phabricator Automation]

Comment on attachment 9289464 [details]
Bug 1737361 - Add unified toolbar to messenger window. r=aleca

Revision D154397 was moved to bug 1784289. Setting attachment 9289464 [details] to obsolete.

Comment 10

[Martin Giger [:freaktechnik]]

All ongoing work is tracked in meta bug 1811380