Last Comment Bug 512732 - Need a chrome://branding/content/icon48.png since mozapps css refers to this.
: Need a chrome://branding/content/icon48.png since mozapps css refers to this.
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1a1
Assigned To: Philip Chee
:
Mentors:
: 467759 (view as bug list)
Depends on:
Blocks: 465924
  Show dependency treegraph
 
Reported: 2009-08-26 10:51 PDT by Philip Chee
Modified: 2013-07-25 10:22 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Like this? (4.25 KB, image/png)
2009-10-06 02:47 PDT, Giacomo Magnini
kairo: ui‑review+
Details
Patch v1.0 (WIP) Add icon48.png (7.94 KB, patch)
2010-01-04 08:10 PST, Philip Chee
no flags Details | Diff | Review
Patch v1.1 (7.91 KB, patch)
2010-01-05 07:06 PST, Philip Chee
neil: review+
Details | Diff | Review
Patch v1.2. Nits fixed (r=neil) [Checkin: Comment 11] (8.03 KB, patch)
2010-01-11 00:36 PST, Philip Chee
philip.chee: review+
neil: superreview+
Details | Diff | Review

Description Philip Chee 2009-08-26 10:51:59 PDT
While working on Bug 465924, I noticed that global/wizard.css refers to chrome://branding/content/icon48.png, as well as toolkit/mozapps/extensions/content/update.xul

Our migration.xul doesn't use branding but all the other apps in the tree do. So we should start doing this as well

So for this bug we ALL need a SeaMonkey 48x48px icon48.png similar to suite/branding/content/icon64.png and located in the same place.

===========================
From Bug 465924 Comment 27:

>> Where does this show up? Does the default theme not have it as well?

> http://mxr.mozilla.org/comm-central/search?string=branded=%22true%22

> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/extensions/content/update.xul#55
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/migration/content/migration.xul#50
> http://mxr.mozilla.org/comm-central/source/mail/components/migration/content/migration.xul#49
> http://mxr.mozilla.org/comm-central/source/calendar/base/content/migration.xul#54

> The suite version appears to not use branded="true". Perhaps we should start
> doing so?
> http://mxr.mozilla.org/comm-central/source/suite/profile/migration.xul#43

> ...we would need to put it in chrome://branding/content/icon48.png
Comment 1 Giacomo Magnini 2009-10-06 02:47:41 PDT
Created attachment 404793 [details]
Like this?
Comment 2 Philip Chee 2010-01-04 08:10:16 PST
Created attachment 419918 [details] [diff] [review]
Patch v1.0 (WIP) Add icon48.png 

This patch:

1. Adds icon48.png from previous attachment.
2. Adds CSS to modern/global/wizard.css to turn on branding for wizards that specify it. 
3. Specifies branding for migration.xul just like Thunderbird, Calendar, and Firefox.

To test the migration wizard:
1. Bookmarks->Manage Bookmarks.
2. Tools->Import.

To test the toolkit extension update wizard:
1. Open the Error Console and enter the following string into the evalute textbox (all on one line):

(function() {var URI_EXTENSION_UPDATE_DIALOG = "chrome://mozapps/content/extensions/update.xul"; var features = "chrome,centerscreen,dialog,titlebar"; openDialog(URI_EXTENSION_UPDATE_DIALOG, "", features, []); })()
Comment 3 Philip Chee 2010-01-04 08:11:15 PST
Comment on attachment 404793 [details]
Like this?

I think KaiRo muttered something about needing a drop shadow or some mysterious visual effect.
Comment 4 neil@parkwaycc.co.uk 2010-01-04 08:50:08 PST
What's wrong with openDialog("chrome://mozapps/content/extensions/update.xul", "", "chrome,centerscreen,dialog,titlebar", []);
Comment 5 neil@parkwaycc.co.uk 2010-01-04 09:03:16 PST
Comment on attachment 419918 [details] [diff] [review]
Patch v1.0 (WIP) Add icon48.png 

>diff --git a/suite/branding/content/icon48.png b/suite/branding/content/icon48.png
>new file mode 100644
[Assuming you get ui-r=KaiRo of course]

[not enough context to show 23px margin]
> .wizard-header-description {
>   -moz-margin-start: 44px;
>   -moz-margin-end: 5px;
(Bah, these don't actually work, since the rules for label/description win.)

>+wizard[branded="true"] .wizard-header {
>+  padding-bottom: 8px;
>+}
>+
>+wizard[branded="true"] .wizard-header-icon {
>+  list-style-image: url("chrome://branding/content/icon48.png");
>+  -moz-margin-end: 6px;
>+}
I don't see any reason not to copy winstripe.
Comment 6 Philip Chee 2010-01-05 07:06:19 PST
Created attachment 420072 [details] [diff] [review]
Patch v1.1

> [not enough context to show 23px margin]
>> .wizard-header-description {
>>   -moz-margin-start: 44px;
>>   -moz-margin-end: 5px;
> (Bah, these don't actually work, since the rules for label/description win.)
I played around with various selectors but finally gave up and used !important.

>>+wizard[branded="true"] .wizard-header {
>>+  padding-bottom: 8px;
>>+}
>>+
>>+wizard[branded="true"] .wizard-header-icon {
>>+  list-style-image: url("chrome://branding/content/icon48.png");
>>+  -moz-margin-end: 6px;
>>+}
> I don't see any reason not to copy winstripe.
I was copying Kudens toolkit-modern/global/wizard.css but I've switched to using the winstripe CSS in this patch.
Comment 7 neil@parkwaycc.co.uk 2010-01-05 14:00:41 PST
(In reply to comment #6)
>> [not enough context to show 23px margin]
>>> .wizard-header-description {
>>>   -moz-margin-start: 44px;
>>>   -moz-margin-end: 5px;
>> (Bah, these don't actually work, since the rules for label/description win.)
>I played around with various selectors but finally gave up and used !important.
But you overlooked my hint :-(
Comment 8 neil@parkwaycc.co.uk 2010-01-05 14:01:40 PST
Comment on attachment 420072 [details] [diff] [review]
Patch v1.1

>   -moz-margin-start: 23px;
r=me with this fixed. (By the way optipng likes the graphic.)
Comment 9 Robert Kaiser (not working on stability any more) 2010-01-10 15:24:23 PST
Comment on attachment 404793 [details]
Like this?

I'd like it better if I knew the source of the image, but it looks like it's the dropshadow version we should be using where possible, yes.
Comment 10 Philip Chee 2010-01-11 00:36:20 PST
Created attachment 421008 [details] [diff] [review]
Patch v1.2. Nits fixed (r=neil)
[Checkin: Comment 11]

Carrying forward r=neil

> (From update of attachment 420072 [details] [diff] [review])
>>   -moz-margin-start: 23px;
> r=me with this fixed. (By the way optipng likes the graphic.)
Fixed.
Comment 11 Serge Gautherie (:sgautherie) 2010-01-12 07:15:00 PST
Comment on attachment 421008 [details] [diff] [review]
Patch v1.2. Nits fixed (r=neil)
[Checkin: Comment 11]


http://hg.mozilla.org/comm-central/rev/161b63136587
Comment 12 Robert Kaiser (not working on stability any more) 2010-01-12 08:06:01 PST
Comment on attachment 421008 [details] [diff] [review]
Patch v1.2. Nits fixed (r=neil)
[Checkin: Comment 11]

1) this approval request has no risk assessment or statement why it's needed on branch
2) the approval request was not made by the developer of the patch

For those reasons, clearing the request.
Comment 13 Serge Gautherie (:sgautherie) 2010-01-20 05:03:08 PST
(In reply to comment #12)

> 1) this approval request has no risk assessment or statement why it's needed on
> branch

I assumed:
*No/Low risk to add a missing image (and related css).
*Needed per http://mxr.mozilla.org/comm-1.9.1/search?string=icon48.png&case=on&find=%2Fwizard.css

> 2) the approval request was not made by the developer of the patch

But I may be mistaken or maybe Philip just doesn't care about this case?
Comment 14 Robert Kaiser (not working on stability any more) 2010-01-20 05:09:34 PST
(In reply to comment #13)
> *Needed per
> http://mxr.mozilla.org/comm-1.9.1/search?string=icon48.png&case=on&find=%2Fwizard.css

This only refers to the branded wizard, and comment #0 states that we are not using that one right now.
Comment 15 Philip Chee 2010-01-20 07:04:02 PST
1. Toolkit UI uses branded wizards.
2. This patch turns on branding for the one case where the suite has a wizard.
3. Since we don't do separate branded/unbranded builds like Firefox/Minefield we don't need branded wizards, certainly not on 1.9.1 branch!

The only reason I did this patch is for parity with toolkit going forward.
Comment 16 Serge Gautherie (:sgautherie) 2010-01-25 15:35:31 PST
(I understand, sorry.)
Comment 17 Philip Chee 2013-07-25 10:22:52 PDT
*** Bug 467759 has been marked as a duplicate of this bug. ***

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