694786 - Remove hardcoded icon paths from notification.xml.

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

From bug 693844 comment 1:

There is one reference to chrome://mozapps/skin/xpinstall/xpinstallItemGeneric.png in suite/common/bindings/notification.xml

KaiRo already noticed this in https://bugzilla.mozilla.org/show_bug.cgi?id=572043#c22

> Don't care about modern, and I actually hate how notification.xml directly
> points to an URL without giving themes to point to a possibly
> differently-named file.

I think we should avoid hard coding image paths in themes (or anywhere else).

Looking at the toolkit PopupNotifications.jsm for inspiration:

In our notification.xml::addonInstallBlocked(), we can add at the end:

var notification = this.appendNotification(messageString, notificationName,
                                           iconURL, priority, buttons);
notification.setAttribute("class", notificationName);

Then in navigator.css near /* ::::: notification popups ::::: */ we can add:

.addon-install-blocked {
  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
}

or perhaps:
.addon-install-blocked > .notification-inner > hbox > .messageImage {
  list-style-image: url("chrome://mozapps/skin/extensions/extensionGeneric.png");
}

May need !important; to override the default icons in global/notifications.css.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Fixit.

Includes fix for Bug 511874: Notification bar should use 16x16 versions of icons.

> +.messageImage[type][value="plugin-crashed"] {
> +  list-style-image: url("chrome://mozapps/skin/plugins/pluginGeneric-16.png");
[...]
> +.messageImage[type][value="indexedDB-permissions-prompt"],
> +.messageImage[type][value="indexedDB-quota-prompt"] {
> +  list-style-image: url("chrome://global/skin/icons/question-16.png");
Fixes Bug 511874: Notification bar should use 16x16 versions of icons.

>  .popup-notification-icon[popupid="indexedDB-permissions-prompt"],
>  .popup-notification-icon[popupid="indexedDB-quota-prompt"] {
> -  list-style-image: url("chrome://global/skin/icons/question64.png");
> +  list-style-image: url("chrome://global/skin/icons/question-64.png");
Somewhere along the way toolkit renamed question64.png to question-64.png.

> +++ b/suite/themes/modern/global/notification.css
[....] 
> -.notification-inner[type="info"] > hbox > .messageImage {
> +.messageImage[type="info"] {
>    list-style-image: url("chrome://global/skin/icons/information-16.png");
>  }
>  
> -.notification-inner[type="warning"] > hbox > .messageImage {
> +.messageImage[type="warning"] {
>    list-style-image: url("chrome://global/skin/icons/warning-16.png");
>  }
>  
> -.notification-inner[type="critical"] > hbox > .messageImage {
> +.messageImage[type="critical"] {
>    list-style-image: url("chrome://global/skin/icons/error-16.png");
>  }
This ports my toolkit fix for Bug 736735 (notification.css: Use inheritance instead of non-performant css).

> +++ b/suite/themes/modern/jar.mn
[....] 
>    skin/modern/global/icons/pg-portrait-small.gif                   (global/icons/pg-portrait-small.gif)
> +  skin/modern/global/icons/Question.png                            (global/icons/Question.png)
In Bug 539066 I added Question.png but forgot to add it to jar.mn!!

>    skin/modern/global/icons/question-16.png                         (global/icons/question-16.png)
> +  skin/modern/global/icons/question-64.png                         (global/icons/question-64.png)
Add question-64.png

> +++ b/suite/themes/modern/navigator/navigator.css
[....] 
>  .popup-notification-icon[popupid="indexedDB-permissions-prompt"],
>  .popup-notification-icon[popupid="indexedDB-quota-prompt"] {
> -  list-style-image: url("chrome://global/skin/icons/alert-question.gif");
> -  width: 46px;
> -  height: 36px;
> +  list-style-image: url("chrome://global/skin/icons/question-64.png");
>  }
And get rid of the width/height hack.

Comment 2

[[e-mail]]

(In reply to Philip Chee from comment #1)
> >  .popup-notification-icon[popupid="indexedDB-permissions-prompt"],
> >  .popup-notification-icon[popupid="indexedDB-quota-prompt"] {
> > -  list-style-image: url("chrome://global/skin/icons/question64.png");
> > +  list-style-image: url("chrome://global/skin/icons/question-64.png");
> Somewhere along the way toolkit renamed question64.png to question-64.png.
Eek! Do we need to fix this on any branches?

Comment 3

[Philip Chee]

>> Somewhere along the way toolkit renamed question64.png to question-64.png.
Looking at Hg blame it seems to have always been question-64.png since it landed via Bug 429282 - "Land new windows icons (20080416)"

> Eek! Do we need to fix this on any branches?
I'd say probably maybe. Nobody seems to have noticed it missing in the MailNews MDN notification bar.

Comment 4

[Philip Chee]

> -  list-style-image: url("chrome://global/skin/icons/question64.png");
Blame says question64.png introduced by Neil via Bug 595810 Part 8c: implement indexedDB doorhanger

Comment 5

[[e-mail]]

Oops. Any chance of a separate bug so we can get it backported? Thanks :-)

Comment 6

[Philip Chee]

> Oops. Any chance of a separate bug so we can get it backported? Thanks :-)
Please file a bug and assign to me thanks! I probably won't be ablee to get around to this until after the weekend though.

Comment 7

[[e-mail]]

Comment on attachment 619098 [details] [diff] [review]
Patch v1.0 Fixit.

>diff --git a/suite/themes/classic/mac/navigator/navigator.css b/suite/themes/classic/mac/navigator/navigator.css
The best way of associating the styles with notifications would be a scoped stylesheet. Unfortunately the notification element is separate from the notificationbox so we would have to create a new derived binding for notification elements. Failing that, the styles belong in communicator.css rather than navigator.css (which is doorhangers only).

>+.messageImage[type][value="refresh-blocked"] {
.messageImage[value="foo"] should cascade after .messageImage[type="foo"]

>+.messageImage[type][value="indexedDB-permissions-prompt"],
>+.messageImage[type][value="indexedDB-quota-prompt"] {
>+  list-style-image: url("chrome://global/skin/icons/question-16.png");
Why this change?

> .popup-notification-icon[popupid="indexedDB-permissions-prompt"],
> .popup-notification-icon[popupid="indexedDB-quota-prompt"] {
>-  list-style-image: url("chrome://global/skin/icons/alert-question.gif");
>-  width: 46px;
>-  height: 36px;
>+  list-style-image: url("chrome://global/skin/icons/question-64.png");
Why this change?

Comment 8

[Philip Chee]

Attachment: Patch v1.1 fix issues.

>>diff --git a/suite/themes/classic/mac/navigator/navigator.css b/suite/themes/classic/mac/navigator/navigator.css
> The best way of associating the styles with notifications would be a scoped
> stylesheet. Unfortunately the notification element is separate from the
> notificationbox so we would have to create a new derived binding for
> notification elements. Failing that, the styles belong in communicator.css
> rather than navigator.css (which is doorhangers only).
Fixed.

>>+.messageImage[type][value="refresh-blocked"] {
> .messageImage[value="foo"] should cascade after .messageImage[type="foo"]
Fixed.

>>+.messageImage[type][value="indexedDB-permissions-prompt"],
>>+.messageImage[type][value="indexedDB-quota-prompt"] {
>>+  list-style-image: url("chrome://global/skin/icons/question-16.png");
> Why this change?

The doorhanger uses a question icon. I thought we should be consistent when using a notificationbox.

>> .popup-notification-icon[popupid="indexedDB-permissions-prompt"],
>> .popup-notification-icon[popupid="indexedDB-quota-prompt"] {
>>-  list-style-image: url("chrome://global/skin/icons/alert-question.gif");
>>-  width: 46px;
>>-  height: 36px;
>>+  list-style-image: url("chrome://global/skin/icons/question-64.png");
> Why this change?

alert-question.gif is 46x36px so needs a hack to prevent it from being stretched. Also it doesn't have a transparent background. I've replaced it with question-64.png from Kuden's Past Modern which is 64x64 and has an alpha transparent background.

Comment 9

[Philip Chee]

Comment on attachment 622045 [details] [diff] [review]
Patch v1.1 fix issues.

ping stefanh for Mac review. To test run the following in navigator chrome context:

gBrowser.getNotificationBox().appendNotification("refresh-blocked", "refresh-blocked", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);

gBrowser.getNotificationBox().appendNotification("blocked-plugins", "blocked-plugins", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("missing-plugins", "missing-plugins", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("outdated-plugins", "outdated-plugins", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("carbon-failure-plugins", "carbon-failure-plugins", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("plugin-crashed", "plugin-crashed", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("click-to-play-plugins", "click-to-play-plugins", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);

gBrowser.getNotificationBox().appendNotification("addon-install-blocked", "addon-install-blocked", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("addon-install-cancelled", "addon-install-cancelled", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("addon-install-complete", "addon-install-complete", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("addon-install-disabled", "addon-install-disabled", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("addon-install-failed", "addon-install-failed", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("addon-install-started", "addon-install-started", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("lwtheme-install-request", "lwtheme-install-request", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("lwtheme-install-notification", "lwtheme-install-notification", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);

gBrowser.getNotificationBox().appendNotification("popup-blocked", "popup-blocked", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);

gBrowser.getNotificationBox().appendNotification("indexedDB-permissions-prompt", "indexedDB-permissions-prompt", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);
gBrowser.getNotificationBox().appendNotification("indexedDB-quota-prompt", "indexedDB-quota-prompt", null,gBrowser.getNotificationBox().PRIORITY_INFO_MEDIUM, []);

Comment 10

[[e-mail]]

Comment on attachment 622045 [details] [diff] [review]
Patch v1.1 fix issues.

>-          <xul:image anonid="messageImage" class="messageImage" xbl:inherits="src=image"/>
>+          <xul:image anonid="messageImage" class="messageImage" xbl:inherits="src=image,type,value"/>
Good catch, but we actually have three notification overrides.
r=me with that and your line endings fixed.

Comment 11

[Philip Chee]

Attachment: Patch v1.1 fix line endings etc. r=Neil

>>-          <xul:image anonid="messageImage" class="messageImage" xbl:inherits="src=image"/>
>>+          <xul:image anonid="messageImage" class="messageImage" xbl:inherits="src=image,type,value"/>
> Good catch, but we actually have three notification overrides.
> r=me with that and your line endings fixed.
Fixed.

Over to Stefan for Mac review.

Comment 12

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/2e313a301e28
Includes fix for Bug 511874 Notification bar should use 16x16 versions of icons.
Also fixes DOS line endings in classic/mac/messenger/primaryToolbar.css.