512173 - .message-icon rule in toolkit/themes/winstripe/global/global.css points to non-existent icon

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[[e-mail]]

As far as I can tell it could be intended to be information-32.png? That image seems to be nearest to the old alert-message.gif image.

Comment 1

[Dão Gottwald [:dao]]

Are there cases where commonDialog.js hasn't been provided with an icon class?

Comment 2

[Dão Gottwald [:dao]]

To clarify: In mozilla-central, the message-icon class is only used as a fallback in commonDialog.js. I'd like to know whether that fallback makes sense in order to figure out if the class should be fixed or moved out of toolkit.

Comment 3

[[e-mail]]

Bah, now not only do I find that we almost but not quite used it in the HTML mail question dialog, but we do (try to) use it in our sort bookmark dialog :-(

Comment 4

[Magnus Melin [:mkmelin]]

Dão: were gonna use it for bug 507682.

Comment 5

[Dão Gottwald [:dao]]

message-icon seems to exist specifically for commonDialog.xul, so other uses wouldn't need to be supported. Then again, I wonder why it's not defined in commonDialog.css.

The problem with accepting the *-icon classes globally is that consumers may want different icon sizes depending on the context.

Comment 6

[[e-mail]]

Attachment: Option 1: rename the file to match Error, Warning etc.

Comment 7

[[e-mail]]

Attachment: Option 2: change the CSS

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 396427 [details] [diff] [review]
Option 2: change the CSS

>--- a/toolkit/themes/pinstripe/global/global.css	Mon Aug 24 15:56:43 2009 -0700
>+++ b/toolkit/themes/pinstripe/global/global.css	Tue Aug 25 14:33:21 2009 +0100

>-.message-icon {
>-  display: none;
>-}

Maybe this was done on purpose for commonDialog.xul?

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 396425 [details] [diff] [review]
Option 1: rename the file to match Error, Warning etc.

Option 2 seems more appealing, as the foobar-XX.png naming scheme is newer.

Comment 10

[[e-mail]]

(In reply to comment #8)
> >-.message-icon {
> >-  display: none;
> >-}
> Maybe this was done on purpose for commonDialog.xul?
But as you already pointed out, no Firefox code can ever trigger this anyway.

Comment 11

[Dão Gottwald [:dao]]

I haven't pointed that out. I've noticed that commonDialog.js falls back to message-icon, but I don't know if that fallback is actually triggered.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 396427 [details] [diff] [review]
Option 2: change the CSS

Somebody needs to figure this out:

> commonDialog.js falls back to
> message-icon, but I don't know if that fallback is actually triggered.

Comment 13

[[e-mail]]

In all of comm-central, there are only two hits for message-icon in code.
The first is in nsPromptService.cpp where it is used to initialise an unreferenced variable. The second is in commonDialog.js as you know, and the only caller in code is nsPromptService.cpp again.

I also searched mxr-test's addons snapshot. There are are number of extension scripts referring to the message-icon class, although they are just rolling their own dialog and it's not clear if they actually end up using it. Some extensions also explicitly open commonDialog.xul and one of them does not seem to set the icon class and would therefore fall back to the message icon. One extension has two windows that only specify the message-icon class in the XUL.

Comment 14

[Dão Gottwald [:dao]]

So I looked through nsPromptService.cpp, and it doesn't look like it will ever open commonDialog.xul in such a way that commonDialog.js will fall back on message-icon. So can we remove that fallback as well as kWarningIconClass from nsPromptService.cpp?

Comment 15

[[e-mail]]

(In reply to comment #14)
> So I looked through nsPromptService.cpp, and it doesn't look like it will ever
> open commonDialog.xul in such a way that commonDialog.js will fall back on
> message-icon. So can we remove that fallback as well as kWarningIconClass from
> nsPromptService.cpp?
Do you want to assume that people will always pass in a non-empty class?

Comment 16

[Dão Gottwald [:dao]]

Is using message-icon instead of no icon reasonable, given that we don't have a case for message-icon right now (neither explicit nor via the empty argument) and pinstripe has code for hiding it? Shouldn't people pass message-icon if they want that?

Comment 17

[[e-mail]]

(In reply to comment #16)
> Is using message-icon instead of no icon reasonable, given that we don't have a
> case for message-icon right now (neither explicit nor via the empty argument)
> and pinstripe has code for hiding it? Shouldn't people pass message-icon if
> they want that?
Sure, but should the icon class-mangling code run if they pass no icon?

Comment 18

[Dão Gottwald [:dao]]

I would just not touch info.icon's class if that parameter is empty. People passing an empty class would get no icon.

Comment 19

[[e-mail]]

Whoops, I accidentally duplicated bug 498893. Sorry.

I've just noticed that message-icon etc. are documented on MDC!

Comment 20

[[e-mail]]

Attachment: Remove bogus message-icon consumers

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 403198 [details] [diff] [review]
Remove bogus message-icon consumers

>+    iconElement.className += " " + iconClass;

iconElement.classList.add(iconClass);?

Comment 23

[Daniel Holbert [:dholbert]]

This landed 5 hours ago as:
  http://hg.mozilla.org/mozilla-central/rev/29e8f7716f6d
However, every single one of the 8 Linux mochitest-plain runs since then have had test failures in "test_prompt.html" and have timed out the mochitest harness in "test_prompt_async.html":

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254169763.1254173049.17329.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254172373.1254175527.12761.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254174039.1254177588.4052.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254178446.1254181861.19312.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254169523.1254172494.11086.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254171028.1254173685.24517.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254172936.1254175535.12805.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254178822.1254181904.19656.gz

It hasn't been as consistent on Mac & Windows, but there have been 3 windows tinderbox cycles with the same failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254176310.1254179363.24094.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254176781.1254179410.24491.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254179086.1254181393.13939.gz

and one Mac tinderbox cycle with the same failure:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254175153.1254178088.9812.gz

Given that this patch modifies prompt-related code, I think it's safe to assume this is related...  I'm backing this out.

Comment 24

[Daniel Holbert [:dholbert]]

(I'm not sure why this bug has Version field set to "1.9.1" right now -- the patch landed on mozilla-central, so I'm setting this bug's Version field to "Trunk".)

Comment 25

[Daniel Holbert [:dholbert]]

backout complete: http://hg.mozilla.org/mozilla-central/rev/58da7dbc028e

Comment 26

[Justin Dolske [:Dolske]]

Huh, weird. I don't see offhand why that should break the test. Maybe it's changing the timing of when the icon image loads, and that upsets the test's (buggy?) timing?

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #24)
> (I'm not sure why this bug has Version field set to "1.9.1" right now

Because the bug exists on 1.9.1 (which doesn't mean only 1.9.1). This is particularly relevant for Thunderbird.

> the patch landed on mozilla-central

That's the target milestone.

Comment 28

[Justin Dolske [:Dolske]]

I happened to be running mochitests on a build including this patch (ie, before it was backed out). Running test_prompt.html plops a few of these into the error console when it fails:

Error: uncaught exception: [Exception... "String contains an invalid character"  code: "5" nsresult: "0x80530005 (NS_ERROR_DOM_INVALID_CHARACTER_ERR)"  location: "chrome://global/content/commonDialog.js Line: 168"]

Not sure if these have been there all along, or if it's related to the test failure...

Comment 29

[[e-mail]]

Pushed changeset fbea0edb9622 to mozilla-central, but renaming the variable to iconClasses to remind us why comment #21 won't work.

Comment 30

[Boris Zbarsky [:bzbarsky]]

So this leaves this css pointing to nonexistent images (and commonDialog consumers can trigger it).  was that on purpose?

Comment 31

[[e-mail]]

Comment on attachment 396427 [details] [diff] [review]
Option 2: change the CSS

Pushed changeset 5c8ab5dd1792 to mozilla-central.

(In reply to comment #30)
> So this leaves this css pointing to nonexistent images.  was that on purpose?
No, it was an oversight.

> (and commonDialog consumers can trigger it)
Although documented on devmo I was only able to find comm-central uses.

Comment 32

[[e-mail]]

Comment on attachment 396427 [details] [diff] [review]
Option 2: change the CSS

Makes the message-icon class work as documented on all platforms. (Attachment 403198 [details] [diff] isn't a necessary part of the fix.)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Do we need to fix this on 1.9.2 as well, per the dup?

Comment 35

[Dão Gottwald [:dao]]

We could, I don't think we need to.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, a192=beltzner though

Comment 37

[[e-mail]]

Pushed changeset 1efddecb6ada to releases/mozilla-1.9.2