Last Comment Bug 785692 - Implement the AppButton on toolbars of all tabs
: Implement the AppButton on toolbars of all tabs
Status: RESOLVED FIXED
: ux-consistency
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Thunderbird 18.0
Assigned To: Philipp Kewisch [:Fallen]
:
:
Mentors:
: 787527 (view as bug list)
Depends on: 792849 792884 795342 807629 814956 853523
Blocks: 977028 TB-AppMenu
  Show dependency treegraph
 
Reported: 2012-08-26 02:18 PDT by Richard Marti (:Paenglab)
Modified: 2014-03-07 01:45 PST (History)
8 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Fix - v1 (129.74 KB, patch)
2012-09-08 10:10 PDT, Philipp Kewisch [:Fallen]
no flags Details | Diff | Splinter Review
Fix - v1 unbitrotted (136.38 KB, patch)
2012-09-12 13:47 PDT, Richard Marti (:Paenglab)
mconley: review+
Details | Diff | Splinter Review
Fix - v1 unbitrotted again (136.35 KB, patch)
2012-09-18 12:40 PDT, Richard Marti (:Paenglab)
richard.marti: review+
Details | Diff | Splinter Review
patch for check-in (136.30 KB, patch)
2012-09-23 10:51 PDT, Richard Marti (:Paenglab)
richard.marti: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Richard Marti (:Paenglab) 2012-08-26 02:18:42 PDT
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 1 Richard Marti (:Paenglab) 2012-08-31 12:19:17 PDT
*** Bug 787527 has been marked as a duplicate of this bug. ***
Comment 2 Sean Smith 2012-08-31 12:51:08 PDT
*** Bug 787527 has been marked as a duplicate of this bug. ***
Comment 3 Philipp Kewisch [:Fallen] 2012-09-08 10:10:43 PDT
Created attachment 659500 [details] [diff] [review]
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) 2012-09-11 09:13:46 PDT
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) 2012-09-12 13:47:56 PDT
Created attachment 660571 [details] [diff] [review]
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] 2012-09-13 02:06:37 PDT
No problem :)
Comment 7 Mike Conley (:mconley) - (needinfo me!) 2012-09-18 12:27:11 PDT
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) 2012-09-18 12:40:35 PDT
Created attachment 662275 [details] [diff] [review]
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] 2012-09-21 05:19:46 PDT
No constructor needed, looks like thats a leftover from testing. Feel free to remove before checkin.
Comment 10 Richard Marti (:Paenglab) 2012-09-23 10:51:59 PDT
Created attachment 663828 [details] [diff] [review]
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] 2012-09-23 12:16:26 PDT
https://hg.mozilla.org/comm-central/rev/53a7ba30571c
Comment 12 Mark Banner (:standard8) 2012-09-27 02:27:50 PDT
Checked in: https://hg.mozilla.org/releases/comm-aurora/rev/67b687e60287
Comment 13 Thomas D. (currently busy elsewhere; needinfo?me) 2012-12-26 01:55:27 PST
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?

Note You need to log in before you can comment on or make changes to this bug.