Closed Bug 494657 Opened 11 years ago Closed 11 years ago

Refresh generic notification bar icons on OS X

Categories

(Toolkit :: Themes, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: faaborg, Assigned: ehsan)

Details

(Keywords: verified1.9.1, Whiteboard: [icon-3.5] [icon-complete])

Attachments

(3 files, 1 obsolete file)

Pinstripe uses very dark or saturated notification bars that don't visually mesh with the standard 16x16 dialog icons (error, warning, question, information) like we use on Windows and Linux.  This bug has two components:

1) Drop the new notification bar dialog icons into /source/toolkit/themes/pinstripe/global/icons/

2) Update various types of notification bars to use these new default icons (error, warning, question, information)
Whiteboard: [icon-3.5] [icon-complete]
These icons needs to be dropped in to:

/source/toolkit/themes/pinstripe/global/icons/
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #379413 - Flags: review?(gavin.sharp)
Attachment #379413 - Flags: approval1.9.1?
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes
Attachment #379413 - Flags: review?(gavin.sharp) → review?(dao)
Comment on attachment 379413 [details] [diff] [review]
Patch (v1)

This can land on trunk to bake once reviewed.
Comment on attachment 379413 [details] [diff] [review]
Patch (v1)

The icons go to toolkit/themes/pinstripe/global/notification/, and notification-foo.png shoud be foo-icon.png instead.
Attachment #379413 - Flags: review?(dao) → review-
Attached patch Patch (v2)Splinter Review
Updated according to review comments.
Attachment #379413 - Attachment is obsolete: true
Attachment #380184 - Flags: review?(dao)
Attachment #379413 - Flags: approval1.9.1?
Are there plans to actually use the question icon?
I think the idea is that generic infobars asking the user for more information should use the question icon, yes.
(these have my pre-approval to land on mozilla-central, please ask for a191 once they do)
Attachment #380184 - Flags: review?(dao) → review+
Comment on attachment 380184 [details] [diff] [review]
Patch (v2)

r=me with question-icon.png not added to jar.mn.
>Are there plans to actually use the question icon?

As far as I know question and error are just part of the platform we provide, and extensions might use them when they invoke that type of bar, but we don't currently use either.  Although I am not entirely sure.
(In reply to comment #9)
> (From update of attachment 380184 [details] [diff] [review])
> r=me with question-icon.png not added to jar.mn.

Landed without question-icon.png added to jar.mn:

http://hg.mozilla.org/mozilla-central/rev/591ad6f4cc76
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attached patch 1.9.1 PatchSplinter Review
Attachment #380372 - Flags: approval1.9.1?
(In reply to comment #10)
> >Are there plans to actually use the question icon?
> 
> As far as I know question and error are just part of the platform we provide,
> and extensions might use them when they invoke that type of bar, but we don't
> currently use either.

Extensions can't really do that in a sane way, since the notification API asks for images paths rather than class names or something like that, and question-icon.png won't exist in themes other than pinstripe.
Attachment #380372 - Flags: approval1.9.1? → approval1.9.1+
So, did anybody even look at this before landing it?  This patch replaced 16x16 icons with 24x24 icons, and likely caused bug 495903.  Verifying before marking proper dependency...
(In reply to comment #16)
> This patch replaced 16x16 icons with 24x24 icons

information-16.png, warning-16.png and error-16.png aren't 24x24 icons.
(In reply to comment #17)
> information-16.png, warning-16.png and error-16.png aren't 24x24 icons.
right, and the new icons are 24x24.
Indeed, and that's wrong regardless of bug 495903. They contain bogus transparent pixels that need to be removed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: fixed1.9.1
http://hg.mozilla.org/mozilla-central/rev/71cdad13b9c7
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2c9e498c400c
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
(In reply to comment #16)
> So, did anybody even look at this before landing it?

Yeah, I did. I loaded the zipped icons in tabs and didn't realize that they were bigger than 16x16, as their visual size is actually 16x16. I also tested a build with this patch, but I don't actually know a notification where we use the default icons, so I only verified that the style rules applied and pointed to the right icons.
Btw, the notification icons are always displayed at 16*16px. So the 24px icons would have been scaled and appeared smaller than they should. Nothing dramatic (esp. as the icons are barely used), but quite wrong nevertheless.
I should have caught this as well, I just uploaded the icons after quickly viewing their visible size.
Looks better now with the follow-up fix. Marking verified fixed on trunk and 1.9.1 with

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090603 Minefield/3.6a1pre ID:20090603031250

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090603 Shiretoko/3.5pre ID:20090603031333
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.