Closed Bug 936898 Opened 11 years ago Closed 10 years ago

Add global alert icons in missing sizes to the modern theme

Categories

(SeaMonkey :: Themes, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.26

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Details

(Keywords: modern)

Attachments

(2 files, 2 obsolete files)

As noted in bug 842439, several global icons are missing which are present in the Toolkit themes, thus should be added for future availability.

(Quoting Philip Chee from bug 842439 comment #26)
> (In reply to neil@parkwaycc.co.uk from bug 842439 comment #20)
> > (In reply to rsx11m from bug 842439 comment #19)
> >> Since the text is spanning two lines anyway, I'm wondering if I should
> >> replace the warning-16.png with the larger warning-24.png? Might look better.
> > Modern doesn't have a warning-24.png,
> 
> We have carte blanche from Kuden to reuse the CSS and images in Past Modern.

(Quoting Philip Chee from bug 842439 comment #38)
> Created attachment 829857 [details]
> pastmodern-info-and-question-png.zip
> 
> From Past Modern:
> information.png : in sizes 16, 24, 48, 64
> (Modern already has information-16)
> 
> question.png in sizes 16, 24, 48, 64
> (Modern already has question-16.png question-64.png, and Question.png (32px))
> 
> Please note before checking in any png graphics you should run them through
> pngcrush and/or optipng. This is what I do:
> 
> c:\DEV\OptiPNG\pngcrush -rem gAMA -rem cHRM -rem iCCP -rem sRGB
> contentPluginMissing.png contentPluginMissing-1.png
> c:\DEV\OptiPNG\optipng -zc1-9 -zm1-9 -zs0-3 -f0-5  -nx
> contentPluginMissing.png
> 
> Also see Bug 631392 Comment 3
It appears that my patch in bug 842439 is actually the only "customer" for alert-information.gif and alert-question.gif at this time, oh well. Maybe we should use the opportunity to replace all global alert-*.gif icons with their squared 48px PNG counterparts (specifically alert-exclam.gif and alert-error.gif) rather than focusing on specific icons as they come up? Also, having all sizes consistently covered in the modern theme as well certainly would be valuable either way.
Summary: Add global information and question icons in missing sizes to the modern theme → Add global alert icons in missing sizes to the modern theme
> As noted in bug 842439, several global icons are missing which are present in
> the Toolkit themes, thus should be added for future availability.
We should only add images as when needed. After all PastModern-0.4.10.02.06.jar isn't going anywhere.
I have identified the following CSS sheets which are currently using the odd-size "alert-*.gif" icons:

  suite/themes/modern/global/config.css
  suite/themes/modern/global/console/console.css
  suite/themes/modern/global/global.css
  suite/themes/modern/global/netError.css

So this looks like a reasonable amount of effort to make that switch to 48px PNG icons. Adding the other sizes is trivial as they aren't used yet.
Attached patch Proposed patch (obsolete) — Splinter Review
This patch adds all sizes (16,24,32,48,64) for error, information, question, and warning icons to the modern theme, thus providing parity with the Toolkit themes (and probably also making add-on authors happy who want to use them).

As discussed, those were taken from the Pastmodern theme and are thus expected to be subject to the MPLv2 license.

 error-24.png        error-48.png        error-64.png
 information-24.png  information-32.png  information-48.png  information-64.png
 question-24.png     question-48.png     authentication-48.png*
 Warning.png**       warning-64.png      warning-large.png**

Some remarks:

 *  alert-security.gif was used in multiple contexts for netError.css and in
    global.css; Toolkit uses Larry for netError.css, which is sslWarning.png
    now in modern (I've made that change here, thus requesting ui-review too).
    Only Toolkit's Linux theme has an Authentication.png defined, hence I took
    the Pastmodern sslWarning.png and renamed it to authentication-48.png here.
    The other Toolkit themes are using question-48.png for this instead.

 ** Naming is a bit inconsistent, there is a warning-large.png rather than
    warning-48.png, but the same applies to Windows/Mac Toolkit themes, thus
    I've kept that as is (the same applies to the capitalized 32px versions).

Explicit pixel sizes have been adjusted where they were specified (where they weren't, I assume that may have been done on purpose and thus didn't add them).
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #8337401 - Flags: ui-review?(neil)
Attachment #8337401 - Flags: review?(philip.chee)
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
(In reply to rsx11m from comment #4)
>  ** Naming is a bit inconsistent, there is a warning-large.png rather than
>     warning-48.png, but the same applies to Windows/Mac Toolkit themes, thus
>     I've kept that as is (the same applies to the capitalized 32px versions).

Changed those to warning-{32,48}.png as suggested by Ratty on IRC this morning.
Attachment #8337401 - Attachment is obsolete: true
Attachment #8337401 - Flags: ui-review?(neil)
Attachment #8337401 - Flags: review?(philip.chee)
Attachment #8338482 - Flags: ui-review?(neil)
Attachment #8338482 - Flags: review?(philip.chee)
A friendly ping for the reviews now that the holidays are over?
Sorry, I was sort of expecting the holidays to be

    do ALL the reviews!

But it turned out to be

    watch ALL the network premières!

http://knowyourmeme.com/memes/x-all-the-y
Comment on attachment 8338482 [details] [diff] [review]
Proposed patch (v2)

r=me on the mechanical changes.

> +++ b/suite/themes/modern/global/console/console.css

> -  list-style-image: url("chrome://global/skin/icons/alert-error.gif");
> +  list-style-image: url("chrome://global/skin/icons/error-48.png");
I don't know why we are using 48px GIFs but Toolkit is using 16x16pxpx. Please change these to error-16.png etc. Thanks.

+++ b/suite/themes/modern/global/global.css

 /* ::::: alert icons :::::*/
 
 .message-icon {
-  width: 46px;
-  height: 39px;
-  list-style-image: url("chrome://global/skin/icons/alert-message.gif");
+  width: 48px;
+  height: 48px;
+  list-style-image: url("chrome://global/skin/icons/information-48.png");
Toolkit uses 32x32px for their alert icons.

> +++ b/suite/themes/modern/global/netError.css
>  #errorPageContainer.certerror {
> -  background-image: url("chrome://global/skin/icons/alert-security.gif");
> +  background-image: url("chrome://global/skin/icons/sslWarning.png");
Check with Neil if he prefers authentication-48.png here.

I did not check if the PNGs had been optimized or not because we now have a volunteer who is currently re-doing our Modern theme images so these PNGs will probably be superseded eventually. I do think we should check this patch in since we don't know when we will be getting the new images.
Attachment #8338482 - Flags: review?(philip.chee) → review+
(In reply to Philip Chee from comment #8)
> > -  list-style-image: url("chrome://global/skin/icons/alert-error.gif");
> > +  list-style-image: url("chrome://global/skin/icons/error-48.png");
> I don't know why we are using 48px GIFs but Toolkit is using 16x16pxpx.
Originally both themes used their alert images, but in bug 353673 Toolkit switched to 16px for some inexplicable reason. Please don't change them.


> >  .message-icon {
> > -  width: 46px;
> > -  height: 39px;
> > -  list-style-image: url("chrome://global/skin/icons/alert-message.gif");
> > +  width: 48px;
> > +  height: 48px;
> > +  list-style-image: url("chrome://global/skin/icons/information-48.png");
> Toolkit uses 32x32px for their alert icons.
Why are we even changing these?

> > +++ b/suite/themes/modern/global/netError.css
> >  #errorPageContainer.certerror {
> > -  background-image: url("chrome://global/skin/icons/alert-security.gif");
> > +  background-image: url("chrome://global/skin/icons/sslWarning.png");
> Check with Neil if he prefers authentication-48.png here.
alert-security.gif is correct for netError.css; if the certError.xhtml override is in effect then it uses sslWarning.png automatically.
(In reply to rsx11m from comment #4)
> This patch adds all sizes (16,24,32,48,64) for error, information, question,
> and warning icons to the modern theme, thus providing parity with the
> Toolkit themes (and probably also making add-on authors happy who want to
> use them).
The bit about adding additional sizes for icons for consistency with toolkit themes I understand. But why are we removing the old images?
(In reply to neil@parkwaycc.co.uk from comment #9)
> > I don't know why we are using 48px GIFs but Toolkit is using 16x16pxpx.
> Originally both themes used their alert images, but in bug 353673 Toolkit
> switched to 16px for some inexplicable reason. Please don't change them.

Ok, so no change here.

> > > -  list-style-image: url("chrome://global/skin/icons/alert-message.gif");
> > > +  list-style-image: url("chrome://global/skin/icons/information-48.png");
> > Toolkit uses 32x32px for their alert icons.
> Why are we even changing these?

Since the GIF images are going away, I'll have to change that to its PNG counterpart. Either way, for the same reason as you stated above, those are supposed to remain 48px or do you want to change them to 32px as used by Toolkit?

> > > +++ b/suite/themes/modern/global/netError.css
> > >  #errorPageContainer.certerror {
> > > -  background-image: url("chrome://global/skin/icons/alert-security.gif");
> > > +  background-image: url("chrome://global/skin/icons/sslWarning.png");
> > Check with Neil if he prefers authentication-48.png here.
> alert-security.gif is correct for netError.css; if the certError.xhtml

Again the GIF will go, thus need the PNG replacement.

> override is in effect then it uses sslWarning.png automatically.

So, what are you suggesting here?

(In reply to neil@parkwaycc.co.uk from comment #10)
> The bit about adding additional sizes for icons for consistency with toolkit
> themes I understand. But why are we removing the old images?

The GIF images have inconsistent (and odd) sizes whereas the replacement PNGs are consistently 48x48px in size. What would be the benefit/need to retain the GIF versions once their proper PNG counterparts have been established?
OK, let me see whether I prefer 32px or 48px icons.
Comment on attachment 8338482 [details] [diff] [review]
Proposed patch (v2)

> #errorPageContainer.certerror {
>-  background-image: url("chrome://global/skin/icons/alert-security.gif");
>+  background-image: url("chrome://global/skin/icons/sslWarning.png");
I'd prefer authentication-48.png here thanks.
Attachment #8338482 - Flags: ui-review?(neil) → ui-review+
(In reply to rsx11m from comment #11)
> > > > -  list-style-image: url("chrome://global/skin/icons/alert-message.gif");
> > > > +  list-style-image: url("chrome://global/skin/icons/information-48.png");
> > > Toolkit uses 32x32px for their alert icons.
> > Why are we even changing these?
> 
> Since the GIF images are going away, I'll have to change that to its PNG
> counterpart. Either way, for the same reason as you stated above, those are
> supposed to remain 48px or do you want to change them to 32px as used by
> Toolkit?

Since you didn't explicitly comment on this in your approval, do I keep the 48px versions here or change them to 32px per comment #8? Also, Toolkit has inconsistent sizes here anyway (32px for Windows, 64px for Mac OSX, "size=dialog" for Linux except authentication.png with 40px), also there is no authentication-32.png available. Thus, I tend to keep those at 48px for modern to preserve its original design.
Yes, I meant for you to stick with the 48px versions throughout, thanks.
So, comment #13 remains as the only change relative to attachment 8338482 [details] [diff] [review].

Thanks for the reviews and push for comm-central, please.
Attachment #8338482 - Attachment is obsolete: true
Attachment #8365718 - Flags: ui-review+
Attachment #8365718 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a52afd9180c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.26
You need to log in before you can comment on or make changes to this bug.