72530 - Common dialogs CSS is messed up in Modern

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect, P2)

Description

[Simon Fraser [no longer active]]

The common dialogs CSS seems to be messed up, and common dialogs are missing 
images in some cases.

For example, open a new composer window, type, then hit the close box. You'll see 
a "Save/don't save" dialog. Notice that the dialog is missing its image, and has 
the wrong style buttons.

The image is missing because callers specify the image URI (stupid idea in the 
first place), and composer uses "chrome://global/skin/question-icon.gif".

To find all callers of this dialog, LXR for "UniversalDialog".

Note that if you build with files, not jars, in chrome, then this dialog does not 
come up at all, the window is left in an unresponsive state, and you have to 
force-quit the app, so this is fairly serious.

Comment 1

[Simon Fraser [no longer active]]

This at least makes the dialog come up in non-jar builds:
Index: mozilla/xpfe/global/resources/content/commonDialog.xul
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/commonDialog.xul,v
retrieving revision 1.29
diff -b -u -2 -r1.29 commonDialog.xul
--- commonDialog.xul	2001/03/11 12:17:29	1.29
+++ commonDialog.xul	2001/03/19 21:29:22
@@ -18,5 +18,5 @@
   <box flex="1">
     <box autostretch="never" valign="top">
-      <image id="info.icon" class="spaced" src="chrome://global/skin/message-
icon.gif" />
+      <image id="info.icon" class="spaced" src="chrome://global/skin/icons/
alert-message.gif" />
     </box>
     <separator orient="vertical" class="thin"/>

and this fixes editor's URL

     {value:0},
-    "chrome://global/skin/question-icon.gif",
+    "chrome://global/skin/icons/alert-question.gif",
     {value:"false"},

Hewitt, it seems that you need to grep for everyone using the old icon urls, and 
fix them.

These patches do *not* fix the bad button CSS that I still see.

Comment 2

[Simon Fraser [no longer active]]

The page title dialog also needs some fixing.

Comment 3

[Simon Fraser [no longer active]]

There seem to be a bunch of places that use "global/skin/icon/" rather than 
'global/skin/icons/" too:

/themes/modern/global/console.css, line 97 -- list-style-image: url("chrome://
global/skin/icons/alert-error.gif");
/themes/modern/global/console.css, line 101 -- list-style-image: url("chrome://
global/skin/icons/alert-exclam.gif");
/themes/modern/global/console.css, line 105 -- list-style-image: url("chrome://
global/skin/icons/alert-message.gif");
/themes/modern/global/global.css, line 88 -- list-style-image: url("chrome://
global/skin/icon/alert-message.gif");
/themes/modern/global/global.css, line 92 -- list-style-image: url("chrome://
global/skin/icon/alert-exclam.gif");
/themes/modern/global/global.css, line 96 -- list-style-image: url("chrome://
global/skin/icon/alert-error.gif");
/themes/modern/global/global.css, line 100 -- list-style-image: url("chrome://
global/skin/icon/alert-question.gif");
/themes/modern/messenger/messengercompose/messengercompose.css, line 270 -- list-
style-image: url("chrome://global/skin/icon/alert-question.gif");
/themes/modern/messenger/messengercompose/messengercompose.css, line 274 -- list-
style-image: url("chrome://global/skin/icon/alert-question.gif");
/themes/modern/messenger/messengercompose/messengercompose.css, line 278 -- list-
style-image: url("chrome://global/skin/icon/alert-exclam.gif");
/themes/modern/messenger/messengercompose/messengercompose.css, line 283 -- list-
style-image: url("chrome://global/skin/icon/alert-question.gif");

Comment 4

[Oliver Klee]

Adding modern keyword.

Comment 5

[Simon Fraser [no longer active]]

It seems that the Classic skin still has the old layout, so we can't currently 
write CSS that works in both skins, for alert icons and such.

This needs fixing fast. Lots of dialogs in Modern are missing icons because of 
it, and in some cases it causes dialogs to not come up at all.

Comment 6

[Simon Fraser [no longer active]]

This improves things a little:

Index: global/global.css
===================================================================
RCS file: /cvsroot/mozilla/themes/modern/global/global.css,v
retrieving revision 1.30
diff -b -u -2 -r1.30 global.css
--- global.css	2001/03/17 01:53:05	1.30
+++ global.css	2001/03/30 23:28:36
@@ -86,17 +86,17 @@
 
 .message-icon {
-  list-style-image: url("chrome://global/skin/icon/alert-message.gif");
+  list-style-image: url("chrome://global/skin/icons/alert-message.gif");
 }
 
 .alert-icon {
-  list-style-image: url("chrome://global/skin/icon/alert-exclam.gif");
+  list-style-image: url("chrome://global/skin/icons/alert-exclam.gif");
 }
 
 .error-icon {
-  list-style-image: url("chrome://global/skin/icon/alert-error.gif");
+  list-style-image: url("chrome://global/skin/icons/alert-error.gif");
 }
 
 .question-icon {
-  list-style-image: url("chrome://global/skin/icon/alert-question.gif");
+  list-style-image: url("chrome://global/skin/icons/alert-question.gif");
 }
 
Note that there are lots of places in both C++ and JS that will need fixing too:

<http://lxr.mozilla.org/seamonkey/search?string=skin%2Fquestion>
<http://lxr.mozilla.org/seamonkey/search?string=skin%2Falert>
<http://lxr.mozilla.org/seamonkey/search?string=skin%2Ferror>
<http://lxr.mozilla.org/seamonkey/search?string=skin%2Fmessage>

Comment 7

[hangas]

Marking nsbeta1+ and 0.9

Comment 8

[Joe Hewitt (gone)]

Not a good idea to hard code these image URLs all over the place.  Class names
should be used instead. Forthcoming patch to fix this...  BTW, thanks for the
good lead, Simon.  I had no idea common dialogs were hard coding those URLs.

Comment 9

[Joe Hewitt (gone)]

*** Bug 73465 has been marked as a duplicate of this bug. ***

Comment 10

[Simon Fraser [no longer active]]

This bug is also cause the full-screen, content-less dialogs to show up on 
Windows. It's nasty.

Comment 11

[Joe Hewitt (gone)]

Attachment: patch to fix

Comment 12

[Blake Ross]

Mind changing those string names also (since you're no longer assigning urls to 
them)?

Comment 13

[Simon Fraser [no longer active]]

Presumably Classic will need to be fixed to use the new icon classes, no?

Comment 14

[Joe Hewitt (gone)]

Fortunately, classic (mac and win) already correctly uses these icon classes.

Comment 15

[Simon Fraser [no longer active]]

You should change the comments and the parameter name at:
http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/
nsNetSupportDialog.cpp#134
and
http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/
nsNetSupportDialog.cpp#134
and
http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/src/
nsWebShellWindow.cpp#2050

This also needs fixing:
http://lxr.mozilla.org/seamonkey/source/themes/classic/messenger/﷒0﷓

So I'd like to see the aIconUrl parameter renamed for 
nsICommonDialogs.UniversalDialog, in the interface file and all implementations. 
Maybe law or alecf can do this.

Comment 16

[Simon Fraser [no longer active]]

Classic already uses the icon classes? Not according to LXR:

/themes/classic/global/mac/console.css, line 102 -- list-style-image : 
url("chrome://global/skin/error-icon.gif");
/themes/classic/global/mac/global.css, line 105 -- list-style-image : 
url("chrome://global/skin/error-icon.gif");
/themes/classic/global/win/console.css, line 102 -- list-style-image : 
url("chrome://global/skin/error-icon.gif");
/themes/classic/global/win/global.css, line 97 -- list-style-image : 
url("chrome://global/skin/error-icon.gif");

Comment 17

[Simon Fraser [no longer active]]

Oops, ignore that last comment. Those are the class definitions, duh.

Comment 18

[Simon Fraser [no longer active]]

I see that http://lxr.mozilla.org/seamonkey/source/themes/classic/messenger/﷒0﷓ is actually fine.

so sr=sfraser on the fixes in this patch, but I encourage you to fix the comments 
as mentioned, and to get someone to change the parameter name in 
nsICommonDialogs.

Is Classic going to move to the same layout as modern at some point?

Comment 19

[Alec Flett]

Attachment: rename the icons & fix some idl

Comment 20

[Alec Flett]

as requested - along with some conversion from C++ only enums to script-friendly
const longs which I had in my tree
care to sr=?

Comment 21

[Simon Fraser [no longer active]]

alecf: looks good apart from this comment:
+      in wstring inIconClass, /* url of icon to be displayed in dialog */

Comment 22

[Alec Flett]

doh! fixed in my tree.
thanks

Comment 23

[Simon Fraser [no longer active]]

Can we get this in please? We have to make windows common dialogs not be full-
screen and empty.

Comment 24

[Simon Fraser [no longer active]]

sr=sfraser on alecf's parts too.

Comment 25

[Peter "jag" Annema]

r=jag

Comment 26

[Boris Zbarsky [:bzbarsky]]

*** Bug 75210 has been marked as a duplicate of this bug. ***

Comment 27

[Joe Hewitt (gone)]

Tried to land this last night and found that danm had hooked up nsIPromptService
all over the place, this invalidating parts of my patch.  Upcoming patch is
updated to work with nsIPromptService instead of nsICommonDialogs.

Comment 28

[Joe Hewitt (gone)]

Attachment: changes to patch for nsIPromptService

Comment 29

[Peter "jag" Annema]

Not your code, but since you're touching it:

+    nsAutoString url; url.AssignWithConversion( "question-icon" ); 
+    block->SetString( nsIPromptService::eIconClass, url.GetUnicode()); 

NS_NAMED_LITERAL_STRING(styleClass, "question-icon");
block->SetString(nsIPromptService::eIconClass, styleClass.get());


   nsString url;
-  url.AssignWithConversion(kAlertIconURL);
-  block->SetString(eIconURL, url.GetUnicode());
+  url.AssignWithConversion(kAlertIconClass);
+  block->SetString(eIconClass, url.GetUnicode());

NS_ConvertASCIItoUCS2 styleClass(kAlertIconClass);
block->SetString(eIconClass, styleClass.get());

(same pattern repeated a few times, make sure any other |url| is changed to
|styleClass|)

Or, since kAlertIconClass is only used at two places, either
#define ALERT_ICON_CLASS "alert-icon"
...
NS_NAMED_LITERAL_STRING(styleClass, ALERT_ICON_CLASS);

(though I vaguely recall this not working on all platforms)

or just specify the string explicitely:

NS_NAMED_LITERAL_STRING(styleClass, "alert-icon");

All instances of GetUnicode() can be converted to get() here, btw.

Comment 30

[Simon Fraser [no longer active]]

The nsCommonDialogs code is still there, so I think you should fix both.

Comment 31

[Joe Hewitt (gone)]

I am still fixing the common dialog code. The second patch is in addition to the
first patch.

Comment 32

[Joe Hewitt (gone)]

Attachment: patch again, this time with everything

Comment 33

[Peter "jag" Annema]

r=jag (with that nit discussed on irc fixed)

Comment 34

[Alec Flett]

sr=alecf under the assumption that the nits are with the lack of
NS_LITERAL_STRING()/too much *WithConversion()

Comment 35

[Peter "jag" Annema]

Yeah. The ones in nsCommonDialogs.cpp are moot because that file isn't part of
the build any longer, and he fixed the one in nsPermissions.cpp.

Comment 36

[Joe Hewitt (gone)]

fixed

Comment 37

[Conrad Carlen (not reading bugmail)]

What about this one:
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigatorDD.js#311.

Comment 38

[sairuh (rarely reading bugmail)]

*** Bug 74174 has been marked as a duplicate of this bug. ***

Comment 39

[Patty Mac]

Marking verified on Mac (2001-04-25-09-Mtrunk).