Closed Bug 250867 Opened 17 years ago Closed 16 years ago

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

Categories

(Toolkit :: Toolbars and Toolbar Customization, defect)

1.8 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: mvl, Assigned: sipaq)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 257456
Component: Toolbars → Toolbars and Toolbar Customization
Product: Firefox → Toolkit
Assignee: bugs → nobody
QA Contact: bugzilla → nobody
No longer blocks: 257456
QA Contact: nobody → toolbars
-> me
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Patch was tested on Windows for Firefox, Thunderbird and Sunbird (all trunk). Testing on Linux and especially Mac would be greatly appreciated.
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.
Attachment #203640 - Flags: first-review?(bugs.mano)
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?
Attachment #203640 - Flags: first-review?(bugs.mano) → first-review-
(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?
Attached patch Patch v2Splinter Review
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.
Attachment #203640 - Attachment is obsolete: true
Attachment #203838 - Flags: first-review?(bugs.mano)
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.
Attachment #203838 - Flags: first-review?(bugs.mano) → first-review+
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!
Attachment #203838 - Flags: second-review?(mscott)
Attachment #203838 - Flags: second-review?(mscott) → second-review+
Could someone please check this in for me.
(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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha1
This appears to have caused an issue where the sage extension now shows text in toolbars when in icons only mode.
Depends on: 320015
Attached patch 1.8 branch patch (obsolete) — Splinter Review
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?).
Attachment #213754 - Flags: first-review?(bugzilla)
(I tested Firefox with the last patch, but not Thunderbird or Sunbird)
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.
Attachment #213754 - Flags: first-review?(bugzilla) → first-review-
Attached patch real 1.8 branch patch (obsolete) — Splinter Review
Doh, I attached the wrong patch. The whitespace changes are just getting rid of DOS style line endings.
Attachment #213754 - Attachment is obsolete: true
Attachment #213768 - Flags: first-review?(bugzilla)
(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 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.
Attachment #213768 - Flags: first-review?(bugzilla) → first-review+
Attachment #213768 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #213768 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
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
Attachment #213768 - Attachment is obsolete: true
Keywords: fixed1.8.1
Target Milestone: mozilla1.9alpha1 → mozilla1.8.1
Version: unspecified → 1.8 Branch
(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).
You need to log in before you can comment on or make changes to this bug.