1571408 - [de-xbl] convert/remove/rework the toolbar binding

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, task)

Description

[Magnus Melin [:mkmelin]]

Convert the toolbar binding: https://searchfox.org/comm-central/rev/a2f5fb50e09945b629efa137df65814f3e648063/common/bindings/toolbar.xml#12

<toolbar> seems to be a build in element. What our binding does is allow for customization of it. It seems there would be two approaches possible:

1. just make it a CE, <toolbar is="customizable-toolbar"> or
2. go the firefox route, xref bug 1119948/bug 1011857.

#2 would probably be preferable, but hard to say how much work involved. It's complicating things that we have more than one toolbar that can be customized.

Paul, you've touched on customizableui, want to take this on?

Comment 1

[Richard Marti (:Paenglab)]

The actual toolbar supports small and normal icon size and we use only one size. Because of this and it could make the conversion easier, this functionality could be removed.

Comment 2

[Paul Morris [:pmorris]]

I looked into the Firefox route, and it will take a fair bit of work, especially since we have 25 <toolbar>s and the way toolbar items are handled is different (and we have many of them). Not all of those toolbars need to stay customizable, but enough of them will to make it more complicated. (Also, Firefox has already moved to XHTML for the relevant files, while we're still on XUL. That may or may not be a problem.)

Given that the CE conversion looks straightforward and we only have 6 XBL bindings left to do, I think it makes sense to take an incremental approach and just do a CE conversion to finish off the de-XBL work. I've started on this and should have a patch up soon.

Then we (I) can tackle porting Firefox's toolbars and customization code, which we'll want to do for the accessibility improvements (bug 1553231). For a more incremental approach, first convert just the main toolbar and then work on converting the others, a bit at a time. This will require allowing both systems to co-exist peacefully.

Edit: just for the sake of being more precise, currently 14 of the 25 <toolbar>s are customizable="true".

Comment 3

[Paul Morris [:pmorris]]

Attachment: toolbar-de-xbl-0.patch

Convert the toolbar binding to a customizable-toolbar custom element.

Comment 4

[Paul Morris [:pmorris]]

Try server run looks fine: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=31981f45898850843473db20de8f6f21417a79cf

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9084415 [details] [diff] [review]
toolbar-de-xbl-0.patch

Review of attachment 9084415 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine, with some smaller adjustments. r=mkmelin

::: common/bindings/customizable-toolbar.js
@@ +14,5 @@
> +   * @extends {MozXULElement}
> +   */
> +  class CustomizableToolbar extends MozXULElement {
> +    connectedCallback() {
> +      if (this.delayConnectedCallback()) {

you need to also make sure this doesn't run more than once. something like an _hasConnected

@@ +24,5 @@
>  
> +      if (document.readyState == "complete") {
> +        this._init();
> +      } else {
> +        // Need to wait until XUL overlays are loaded. See bug 554279.

XUL overlays do not exist since over a year, so this can be scrapped, and _init() code moved directly here into connectedCallback() instead

@@ +80,2 @@
>  
> +      let ids = (val == "__empty") ? [] : val.split(",");

can you move this down before the for loop where it's  first used?

Comment 6

[Paul Morris [:pmorris]]

Thanks for the review.

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

   // Need to wait until XUL overlays are loaded. See bug 554279.

XUL overlays do not exist since over a year, so this can be scrapped, and
_init() code moved directly here into connectedCallback() instead

Hm, but we still have overlays in calendar (and add-ons), and calendar code uses this customizable toolbar CE. I'm reluctant to take this out before we've removed the overlays from calendar code. Maybe I'm missing something?

Comment 7

[Paul Morris [:pmorris]]

Also, this code will get dropped soon when we port over Firefox's newer toolbar/customization code, so makes sense to just leave it as-is to avoid any possible regressions.

Comment 8

[Magnus Melin [:mkmelin]]

The overlaying of calendar and other add-ons is done by Overlays.jsm instead nowadays, but this is run after those events, so there is no readystatechange after it finished inserting stuff.

Comment 9

[Paul Morris [:pmorris]]

Attachment: toolbar-de-xbl-1.patch

Ah, makes sense! Thanks, I've made the requested changes.

A new try server run for good measure: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d741572aebe01d00324b9c4fb11fa5aee7b6e352

Comment 10

[Paul Morris [:pmorris]]

Try server run looks good.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d43bed230ddf
[de-xbl] Convert the toolbar binding to a custom element. r=mkmelin DONTBUILD