250867 - icons only and other toolbar modes are not created in a global place

Status: RESOLVED FIXED

(Toolkit :: Toolbars and Toolbar Customization, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

To make toobars with mode="icons" really display only icons, there is some css
magic. But that magic is in an app specific theme file. It should be global, so
you don't have to copy it for other apps.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/themes/pinstripe/browser/browser.css&rev=1.2#213
for example.
customizable toolbars and icons only toolbars are a toolkit feature, so should
be created in toolkit, not in browser and copied to every other app.

Comment 1

[Simon Paquet [:sipaq]]

-> me

Comment 2

[Simon Paquet [:sipaq]]

Attachment: Patch v1

Patch was tested on Windows for Firefox, Thunderbird and Sunbird (all trunk). Testing on Linux and especially Mac would be greatly appreciated.

Comment 3

[Simon Paquet [:sipaq]]

To test this on Mac, you should test, whether toolbar customization still works and if switching between the different toolbar modes (large and small icons, large and small icons + text, just text) still works.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 203640 [details] [diff] [review]
Patch v1

The browser and toolkit parts are fine (also, tested), but:

1. mail/ also has these rules in the address book and composition components.
2. The pinstripe style rules are different in the calendar version, is this change intended?

Comment 5

[Simon Paquet [:sipaq]]

(In reply to comment #4)
> 1. mail/ also has these rules in the address book and composition components.

Ah, ok I forgot those. I'll attach a new patch with this stuff.

> 2. The pinstripe style rules are different in the calendar version, is this
> change intended?

Yes. Pinstripe on Calendar is lagging behind the main pinstripe theme in FF. I thought it would be the best solution to move the Sunbird and Thunderbird pinstripe code to the much better tested and maintained FF code.

Would you agree on that assumption?

Comment 6

[Simon Paquet [:sipaq]]

Attachment: Patch v2

Patch updated to review comments. It now also contains the changes for the Thunderbird addressbook and compose window.
Everything tested on Windows, testing on Mac highly appreciated.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 203838 [details] [diff] [review]
Patch v2

r=mano.

I've only tested the browser and the sunbird portions; you need mscott's moa on the mail/ chnages.

Comment 8

[Simon Paquet [:sipaq]]

Comment on attachment 203838 [details] [diff] [review]
Patch v2

Scott, can I get your module owner approval (moa) for this patch. It already has review+ from mano. Thanks!

Comment 9

[Simon Paquet [:sipaq]]

Could someone please check this in for me.

Comment 10

[Asaf Romano (gone)]

(Trunk)
mozilla/toolkit/themes/gnomestripe/global/toolbar.css 1.6
mozilla/toolkit/themes/pinstripe/global/toolbar.css 1.4
mozilla/toolkit/themes/qute/global/toolbar.css 1.5
mozilla/toolkit/themes/winstripe/global/toolbar.css 1.11
mozilla/browser/base/content/browser.xul 1.273
mozilla/browser/themes/pinstripe/browser/browser.css 1.14
mozilla/browser/themes/winstripe/browser/browser.css 1.22
mozilla/mail/themes/pinstripe/mail/mailWindow1.css 1.3
mozilla/mail/themes/pinstripe/mail/primaryToolbar.css 1.4
mozilla/mail/themes/pinstripe/mail/addrbook/addressbook.css 1.2
mozilla/mail/themes/pinstripe/mail/compose/messengercompose.css 1.5
mozilla/mail/themes/qute/mail/mailWindow1.css 1.7
mozilla/mail/themes/qute/mail/primaryToolbar.css 1.5
mozilla/mail/themes/qute/mail/addrbook/addressbook.css 1.3
mozilla/mail/themes/qute/mail/compose/messengercompose.css 1.7
mozilla/calendar/sunbird/base/content/calendar.xul 1.33
mozilla/calendar/sunbird/themes/pinstripe/sunbird/calendar.css  1.13
mozilla/calendar/sunbird/themes/winstripe/sunbird/calendar.css 1.29

Comment 11

[Bill Gianopoulos [:WG9s]]

This appears to have caused an issue where the sage extension now shows text in toolbars when in icons only mode.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: 1.8 branch patch

This is patch v2 ported to the 1.8 branch. Simon, can you double check that I ported it correctly? If you diff the two patches, the only differences are in context, whitespace, and the fact that the changes to calendar.css have already been made on the 1.8 branch (does that mean Sunbird toolbars are currently broken there?).

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(I tested Firefox with the last patch, but not Thunderbird or Sunbird)

Comment 14

[Simon Paquet [:sipaq]]

Comment on attachment 213754 [details] [diff] [review]
1.8 branch patch

>This is patch v2 ported to the 1.8 branch. Simon, can you double 
>check that I ported it correctly? If you diff the two patches, the 
>only differences are in context, whitespace, and the fact that the >changes to calendar.css have already been made on the 1.8 branch 
>(does that mean Sunbird toolbars are currently broken there?).

I don't see any changes in context or whitespace (the change dates at the beginning of each file diff is the one from the trunk). And both patches still have the calendar.css changes.

Are you sure, that you supplied the right patch?

> #button-newcard:hover {
>+/* Don't use the listbox appearance as that gives us a blue-grey top border
>+   that collides with the primary toolbar border */
>+#dirTree {
>+  -moz-appearance:  none;
>+  background-color: -moz-Field;
>+  color: -moz-FieldText;
>+  -moz-border-left-colors: ThreeDShadow ThreeDDarkShadow;
>+  -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
>+  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
>+  border-left: 1px solid;
>+  border-right: 1px solid;
>+  border-bottom: 1px solid;
>+}

This doesn't look like it should belong into this patch.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: real 1.8 branch patch

Doh, I attached the wrong patch. The whitespace changes are just getting rid of DOS style line endings.

Comment 16

[Simon Paquet [:sipaq]]

(In reply to comment #15)
> Doh, I attached the wrong patch. The whitespace changes are just getting rid of
> DOS style line endings.

Looks good on first sight. But I'll have to try compiling Sunbird on the 1.8 branch with these changes on the weekend first, before I can give you review+

Comment 17

[Simon Paquet [:sipaq]]

Comment on attachment 213768 [details] [diff] [review]
real 1.8 branch patch

r=sipaq

But I would appreciate it, if you could also test this with Thunderbird before checking this in.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Attachment: checked in on the 1.8 branch

mozilla/browser/base/content/browser.xul		1.268.2.22
mozilla/calendar/sunbird/base/content/calendar.xul	1.28.2.5
mozilla/browser/themes/winstripe/browser/browser.css	1.17.2.17
mozilla/browser/themes/pinstripe/browser/browser.css	1.11.4.10
mozilla/toolkit/themes/winstripe/global/toolbar.css	1.7.2.5
mozilla/toolkit/themes/qute/global/toolbar.css		1.3.10.2
mozilla/toolkit/themes/pinstripe/global/toolbar.css	1.3.10.1
mozilla/toolkit/themes/gnomestripe/global/toolbar.css	1.5.4.1
mozilla/mail/themes/qute/mail/compose/messengercompose.css	1.4.2.2
mozilla/mail/themes/qute/mail/addrbook/addressbook.css	1.1.4.4
mozilla/mail/themes/qute/mail/primaryToolbar.css	1.2.2.2
mozilla/mail/themes/qute/mail/mailWindow1.css		1.5.2.5
mozilla/mail/themes/pinstripe/mail/compose/messengercompose.css	1.3.2.1
mozilla/mail/themes/pinstripe/mail/addrbook/addressbook.css	1.1.4.1
mozilla/mail/themes/pinstripe/mail/primaryToolbar.css		1.3.2.2
mozilla/mail/themes/pinstripe/mail/mailWindow1.css		1.2.2.1

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> But I would appreciate it, if you could also test this with Thunderbird before
> checking this in.

I tested this with Thunderbird, toolbars worked fine (though bug 320015 now needs to land on the branch).