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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

1.9.1 Branch
mozilla1.9.3a1
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9.2 -

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

9 years ago
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.
Are there cases where commonDialog.js hasn't been provided with an icon class?
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.
(Assignee)

Comment 3

9 years ago
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 :-(
Dão: were gonna use it for bug 507682.
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.
(Assignee)

Comment 6

9 years ago
Created attachment 396425 [details] [diff] [review]
Option 1: rename the file to match Error, Warning etc.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #396425 - Flags: review?(dao)
(Assignee)

Comment 7

9 years ago
Created attachment 396427 [details] [diff] [review]
Option 2: change the CSS
Attachment #396427 - Flags: review?(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 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.
Attachment #396425 - Flags: review?(dao)
(Assignee)

Comment 10

9 years ago
(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.
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 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.
Attachment #396427 - Flags: review?(dao)
(Assignee)

Comment 13

9 years ago
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.
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?
(Assignee)

Comment 15

9 years ago
(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?
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?
(Assignee)

Comment 17

9 years ago
(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?
I would just not touch info.icon's class if that parameter is empty. People passing an empty class would get no icon.
(Assignee)

Comment 19

9 years ago
Whoops, I accidentally duplicated bug 498893. Sorry.

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

Comment 20

9 years ago
Created attachment 403198 [details] [diff] [review]
Remove bogus message-icon consumers
Attachment #403198 - Flags: review?(dao)
Comment on attachment 403198 [details] [diff] [review]
Remove bogus message-icon consumers

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

iconElement.classList.add(iconClass);?
Attachment #403198 - Flags: review?(dao) → review+

Updated

9 years ago
Attachment #396427 - Flags: review+

Updated

9 years ago
Duplicate of this bug: 498893
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.
(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".)
Version: 1.9.1 Branch → Trunk
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?
(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.
Target Milestone: --- → mozilla1.9.3a1
Version: Trunk → 1.9.1 Branch
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...
(Assignee)

Comment 29

9 years ago
Pushed changeset fbea0edb9622 to mozilla-central, but renaming the variable to iconClasses to remind us why comment #21 won't work.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
So this leaves this css pointing to nonexistent images (and commonDialog consumers can trigger it).  was that on purpose?
(Assignee)

Comment 31

9 years ago
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.
(Assignee)

Comment 32

9 years ago
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.)
Attachment #396427 - Flags: approval1.9.2?

Updated

9 years ago
Duplicate of this bug: 522449
Do we need to fix this on 1.9.2 as well, per the dup?
Flags: blocking1.9.2?
We could, I don't think we need to.
Attachment #396427 - Flags: approval1.9.2? → approval1.9.2+
Not blocking, a192=beltzner though
Flags: blocking1.9.2? → blocking1.9.2-
(Assignee)

Comment 37

9 years ago
Pushed changeset 1efddecb6ada to releases/mozilla-1.9.2
status1.9.2: --- → final-fixed
You need to log in before you can comment on or make changes to this bug.