1509389 - Installation of extension with browser_action destroys toolbar

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: General, defect)

Description

[brummolix]

Attachment: empty toolbar.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

- use TB64.0b2 (it was also in beta 1)
- Install an addon which includes a browser_action, for example https://github.com/Brummolix/AutoarchiveReloaded/releases/download/0.9.9.7_beta/AutoArchiveReloaded.0.9.9.7_beta.xpi



Actual results:

The main toolbar (with reply, create, address book, hamburger menu etc.) is emptied, only the extension action is visible (see attached screenshot).

If you close Thunderbird and reopen it, the original toolbar is restored. If you edit the toolbar before closing, the broken state is saved.


Expected results:

The toolbar should be as before (I'm not sure if the extension icon should be added or not)

Comment 1

[Geoff Lankow (:darktrojan)]

I *think* I know what causes this, in which case there's already a fix on the way. Was your toolbar in the default state before installing the extension?

Comment 2

[brummolix]

You are right, it was in default state. I tried it again. If the toolbar is already changed, the extension button is correctly added at the right side fof the toolbar.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1509389-empty-toolbar-1.diff

Even though I have this in bug 1509246, it won't be happening there any time soon.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9027441 [details] [diff] [review]
1509389-empty-toolbar-1.diff

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

::: mail/components/extensions/ExtensionToolbarButtons.jsm
@@ +144,5 @@
>      }
> +    let currentSet = toolbar.getAttribute("currentset");
> +    if (!currentSet) {
> +      currentSet = toolbar.getAttribute("defaultset");
> +    }

could just make it

let currentSet = toolbar.getAttribute("currentset") || toolbar.getAttribute("defaultset");

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1509389-empty-toolbar-2.diff

I suppose the toolbar *could* have an empty current set, so we'd better account for that.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9027774 [details] [diff] [review]
1509389-empty-toolbar-2.diff

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

My suggestion handles that (since falsy...)

Comment 7

[Geoff Lankow (:darktrojan)]

No, you misunderstand, the toolbar might be *deliberately empty*. Why, I've no idea, but people are weird.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ebdff9e20009
Use toolbar default set if current set doesn't exist. r=mkmelin

Comment 9

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/84b7159cc2d5590d4ca37ae99f76b429a8c02eb4