785692 - Implement the AppButton on toolbars of all tabs

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Richard Marti (:Paenglab)]

With landing of bug 650170 the main toolbar has an AppButton and the menubar is hidden. If you switch to a other tab like Chat or Calendar this button is no more accessible and you have to press ALT to show the menubar. For usability it would be better if this button is available on every, or almost every tab toolbar.

- Is it possible to set this on every "third party" toolbar? 
- If yes on which tab types should this button be inserted?

As written above Chat and Calendar should become this button. I think the Add-on tab don't needs this button because I don't see much what we need from this menu for the Add-ons tab.

- If the button is removed from main toolbar, should we then show the button on other toolbars?

I think not, when the user removes the button, he has no use for it. Or if he put it to the tabs-toolbar he has all the time access and don't need it on a other toolbar.

- What, when the user moves the button to the left or the middle of the main toolbar?

If the button is the first or last element in the main toolbar then the button can be set similar on the other toolbars. If the button is set somewhere else in the toolbar, we can't say where he wants the button exactly and we set it at the default position. The user can then move it where he wants it. And this should be saved.

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This patch takes care by providing a certain type of button that can be added to the toolbar. The extension still needs to add this button to its toolbarpallette, but its just a matter of adding the button via xul, and maybe making it visible by default.

The selector for this button type is the class "button-appmenu". I originally wanted to use the attribute type="appmenu", but in the customize toolbar dialog the type attribute is stripped out to avoid menus showing up there.

I've added the button to chat, but I didn't find anything to migrate the button into the toolbar. For me it magically showed up without migration, but I don't know why. Maybe someone (Florian, Mike?) can take over that step.

Comment 4

[Richard Marti (:Paenglab)]

I tested it without Lightning because of the actual bustage. But it looks good and adds the button to the Chat toolbar.

Only one thing: the button-appmenu needs now a !important for the min-width: 35px; because it's now only a class and other rules override this.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: Fix - v1 unbitrotted

Bug 784676 bitrotted Philipp's patch. I've only unbitrotted it and added the !important to the width rules.

Philipp, I hope it's okay I've done this.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

No problem :)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 660571 [details] [diff] [review]
Fix - v1 unbitrotted

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

Just one question below - otherwise, code looks good.

You've been bit-rotted in chat-messenger-overlay.xul, but it's a trivial fix.

::: mail/base/content/mailWidgets.xml
@@ +2833,5 @@
>    </binding>
> +
> +  <binding id="appmenu-vertical" extends="chrome://global/content/bindings/toolbarbutton.xml#menu-vertical">
> +    <implementation>
> +      <constructor><![CDATA[

If this is empty, do you even need a constructor?

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Fix - v1 unbitrotted again

(In reply to Mike Conley (:mconley) from comment #7)
> Comment on attachment 660571 [details] [diff] [review]
> Fix - v1 unbitrotted
> 
> Review of attachment 660571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just one question below - otherwise, code looks good.
> 
> You've been bit-rotted in chat-messenger-overlay.xul, but it's a trivial fix.

Fixed again.

> ::: mail/base/content/mailWidgets.xml
> @@ +2833,5 @@
> >    </binding>
> > +
> > +  <binding id="appmenu-vertical" extends="chrome://global/content/bindings/toolbarbutton.xml#menu-vertical">
> > +    <implementation>
> > +      <constructor><![CDATA[
> 
> If this is empty, do you even need a constructor?

This is Philipp's code, so let's him reply.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

No constructor needed, looks like thats a leftover from testing. Feel free to remove before checkin.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Patch for check-in with the constructor code removed.

[Approval Request Comment]
This patch gives the AppButton functionality to the chat toolbar and also to Add-ons like Lightning.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/53a7ba30571c

Comment 12

[Mark Banner (:standard8)]

Checked in: https://hg.mozilla.org/releases/comm-aurora/rev/67b687e60287

Comment 13

[Thomas D. (:thomas8)]

AppButton doesn't work for standalone windows e.g. external .eml files or "open in new window" - Bug 814956 (currently waiting for input of Philipp [:Fallen])

Furthermore, do we have a bug for introducing AppButton to composition window?