1598190 - browserAction buttons don't persist outside the default toolbar

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[Geoff Lankow (:darktrojan)]

As mentioned in bug 1584160 comment 14, if you take a browserAction and move it to a different toolbar, it does not come back after a restart. We need to refresh the currentSets at some point after the extensions load.

Comment 1

[CEP]

This has persisted up through 78.4.0. Other threads appear to infer that it's due to the Mail Toolbar being a default. In my setup, I hide the Mail Toolbar entirely... and have since the ability to hide it was added back in the 20s versions. (It's an impediment on a laptop screen, especially wearing bifocals.)

Comment 2

[John Bieling]

Attachment: bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar

Checking the xul store, if the button has been added to one of the non default toolbars.

Comment 3

[Geoff Lankow (:darktrojan)]

I think we can shift this stuff into ToolbarButtonAPI (onUninstall can be moved completely, if you generalise it). And in the paint and onManifestEntry methods, instead of handling only the toolbar we named as this.toolbarId, loop through all toolbars associated with the toolbox in this.toolboxId (but still treat this.toolbarId as the default). This way the code will be future-proofed against changes to the toolbars. Here's one way to get all of the toolbars for a toolbox:

document.querySelectorAll(`#${toolboxId} toolbar, toolbar[toolboxid="${toolboxId}"]`);

Comment 4

[John Bieling]

Attachment: bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v2.patch

I am not entirely happy with this. Moving this into ToolbarButtonAPI is indeed very nice and the toolbox -> toolbar lookup is great - but onUninstall is static and I have no access to the class this which I need for proper cleanup.

I moved the uninstall part into onShutdown where I can only differentiate between shutdown and disable. So the position of the buttons is now kept between restarts, but will be reset to default on disable/enable. Is there any way we can detect uninstall in onShutdown? I failed to find a way.

Furthermore, this currently breaks composeAction, as it does things differently and would need adjustment (which is doable but I skipped it for now). Have not looked at messageDisplayAction, but as this is currently not even a customizable toolbar, this needs further adjustments.

If I would have a say, I would keep it in browserAction, but use the nice toolbox -> toolbar lookup.

Edit: I forgot a console.log in the patch. That has to be removed of course.

Comment 5

[Geoff Lankow (:darktrojan)]

Ah. I forgot about it being static and that's why it's written the weird way it is. Sorry, that's probably been a waste of your time.

If I would have a say, I would keep it in browserAction, but use the nice toolbox -> toolbar lookup.

Yeah, do that.

Comment 6

[John Bieling]

Attachment: bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v3.patch

Hm, the issue remains, onUninstall is static, which means I can have the nice startup code but I still have to use the hardcoded windowURL and toolbar IDs in onUninstall. Having this half/half looks a bit inconsistent?

I let you decide.

Comment 7

[John Bieling]

Attachment: bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v4.patch

As requested, moved toolbar loop into ToolbarButtonAPI and added comment about the querySelectorAll statement. All test run fine locally.

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 9190548 [details] [diff] [review]
bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v4.patch

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

Okay, that makes sense to me. Let's do this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/590f523da7cf
browserAction buttons don't persist outside the default toolbar. r=darktrojan

Comment 10

[John Bieling]

Comment on attachment 9190548 [details] [diff] [review]
bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v4.patch

[Approval Request Comment]
User impact if declined:
Action buttons cannot be moved to other toolbars.

Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a0811bf90663c708f20a1c65ceee55639c74ca24

Risk to taking this patch (and alternatives if risky):
None that I know of. Patch applies on current comm-esr78 (for 78.7.0)

Comment 11

[John Bieling]

Remark: I have created a try run which includes all bugs I want to uplift for TB 78.7 and shows a working patch order. This bug is the 4th one:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=3cdff293d5d1bd77777f5da371caaa02c4b05de3

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 9190548 [details] [diff] [review]
bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v4.patch

[Triage Comment]
Approved for esr78

Comment 13

[Dave Miller [:justdave]]

Thunderbird 78.7.0:
https://hg.mozilla.org/releases/comm-esr78/rev/3d6b48aae0e9