Closed Bug 1598190 Opened 1 year ago Closed 4 months ago

browserAction buttons don't persist outside the default toolbar

Categories

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

defect
Not set
normal

Tracking

(thunderbird_esr78+ fixed, thunderbird84 affected)

RESOLVED FIXED
85 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird84 --- affected

People

(Reporter: darktrojan, Assigned: TbSync)

Details

Attachments

(1 file, 3 obsolete files)

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.

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.)

Assignee: nobody → john
Status: NEW → ASSIGNED

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

Attachment #9189652 - Flags: review?(geoff)

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}"]`);

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.

Flags: needinfo?(geoff)

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.

Flags: needinfo?(geoff)

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.

Attachment #9189889 - Attachment is obsolete: true
Attachment #9190015 - Flags: review?(geoff)
Attachment #9189652 - Flags: review?(geoff)
Attachment #9189652 - Attachment is obsolete: true

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

Attachment #9190015 - Attachment is obsolete: true
Attachment #9190015 - Flags: review?(geoff)
Attachment #9190548 - Flags: review?(geoff)
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.
Attachment #9190548 - Flags: review?(geoff) → review+
Target Milestone: --- → 85 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/590f523da7cf
browserAction buttons don't persist outside the default toolbar. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

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)

Attachment #9190548 - Flags: approval-comm-esr78?

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 on attachment 9190548 [details] [diff] [review]
bug1598190_browserAction_buttons_dont_persist_outside_the_default_toolbar_v4.patch

[Triage Comment]
Approved for esr78

Attachment #9190548 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.