95649 - nsPromptService doesn't know how to produce error alerts

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Matthew T (active 1999-2002)]

Build: 2001081504, Mac OS 9.1

To reproduce:
1.  Enter "http://foo.bar.invalid/" (without quotes) into Navigator's address
    field, and press Enter.
2.  Look at the resulting alert.

What you should see:
*   An error alert, with the red circle/stop-sign icon (depending on the theme).

What you actually see:
*   A dialog that thinks it's a confirmation alert (with the yellow exclamation
    icon), which of course it can't be because it's only got one button.

This appears to be due to the rather major flaw that error alerts cannot be
created in Mozilla -- even though the icons for them are present in the themes.
nsPromptService.cpp contains the following lines:
|
| static const char *kQuestionIconClass ="question-icon";
| static const char *kAlertIconClass ="alert-icon";
| static const char *kWarningIconClass ="message-icon";

Instead, it should have:
|
| static const char *kErrorIconClass = "error-icon";
| static const char *kNoteIconClass = "message-icon";
| static const char *kConfirmationIconClass = "alert-icon";
| static const char *kPasswordIconClass = "password-icon";

For this bug, I'd like error alerts implemented, and the unknown server alert
(or any other single easily reproducable alert) converted to be an error alert
as proof of concept.

Comment 1

[Håkan Waara]

You don't need anything implemented. Just switch the icon on the appropriate alerts.

Comment 2

[Adam Lock]

Conrad, what are your thoughts on this?

Comment 3

[Conrad Carlen (not reading bugmail)]

mpt: "A dialog that thinks it's a confirmation alert..."
What makes our current alert dialog seem like a confirmation alert? The yellow
triangle? An error alert should have only one button because there's no choice
to be made. Is the same true of a warning alert? Shouldn't that also have only
one button if no choice is offered?

If that's true, I'd like to add an 'alertType' param to alert() on nsIPrompt and
nsIPromptService. The values for this param could be:
1. stop (error)
2. note
3. warning

Or, should this be generalized and an 'iconType' param added to most
nsIPrompt/nsIPromptService methods? Then it would be possible to have a 2 button
confirm dialog with a warning icon. Currently, the icon for confirm() is always
a question mark. 

Comment 4

[Matthew T (active 1999-2002)]

For a brief introduction to the UI of alerts, see my 2001-05-30 comment in bug 
31573.

> What makes our current alert dialog seem like a confirmation alert? The
> yellow triangle?

Yes. An alert with only one button is either an error alert, in which case it 
should have the (X) icon (e.g. `The mail server returned an error'); or a note 
alert, in which case it should have the (i) icon (e.g. `The text you entered 
was not found'). Currently we're not distinguishing between the two (we're 
using the incorrect /!\ icon for both), which is why it's unfortunately not a 
case of `just switch the icon'.

> Or, should this be generalized and an 'iconType' param added to most
> nsIPrompt/nsIPromptService methods?

No thanks. :-) All that would do would be to make a lot of work for Håkan, 
fixing up cases (and it would be the majority of cases) where programmers 
picked the wrong icon for the type of alert they were using. Instead, 
nsPromptService should pick the icon automatically based on the alert type -- 
error, note, confirmation, or authentication. The bug here is that 
nsPromptService::Error and nsPromptService::Note aren't separate functions.

> Currently, the icon for confirm() is always a question mark. 

That's bug 59820.

Comment 5

[Matthew T (active 1999-2002)]

Attachment: A utopian redesign of the nsPromptService API, which would fix this bug and many others

Comment 6

[Conrad Carlen (not reading bugmail)]

For the most part, I like it. A few things though:
1) Too many params for Note() and Error().
  a) It's rare that we can offer advice for an error. I think the call site,
which knows the context of the error and is best able to offer advice, should
format the whole message, including advice, into one string.
  b) What are shortcut1Label and shortcut2Label used for?
2) Your first Confirm() method does not have the buttonFlags param like
ConfirmEx() which gives the ability to specify standard button text with
constants as well as arbitrary text.
3) Along with UsernameAndPassword() and Password(), we should have Username().
This is the same as the current Prompt() method though the implementation would
then know to use the same kind of icon as the other methods which are used for
signing onto something.
4) The equivalent of the current Prompt() is gone. We still need that.

As for Select(), it is used (or should be - I have to check) by single-signon
when the user has more than one name with which they have logged into a domain.
It needs to put up a list of these names so the user can pick one.

Comment 7

[Matthew T (active 1999-2002)]

> a) It's rare that we can offer advice for an error.

If that's true, we have bugs in error handling which need to be fixed. Windows 
UI guidelines (emphasis added): `State the problem, its probable cause (if 
known), *and what the user can do about it*, no matter how obvious the 
solution'. Macintosh UI guidelines (emphasis added): `A good alert box message 
says what went wrong, why it went wrong, *and what the user can do about it*'.

> I think the call site ... should format the whole message, including advice,
> into one string.

Not only are they separate strings, but they're displayed using different fonts 
<http://developer.apple.com/techpubs/mac/HIGOS8Guide/graphics/HIG_CG-082.gif>, 
so you can't even fake their separateness by using line break characters.

> b) What are shortcut1Label and shortcut2Label used for?

In very rare cases, an error or note alert may contain one or more buttons 
other than `OK'. These buttons can only be shortcuts to functions which are 
easily available elsewhere (hence the param names); otherwise you're actually 
asking the user to make a decision, in which case you should be using a 
confirmation alert instead. For example, take the error alert where 
errorMessage=`The mail filter "Bugzilla Spam" cannot be used, because the 
destination folder "Bugmail" cannot be found.' and errorAdvice=`Until you fix 
or delete the filter, matching messages will be left in your Inbox.'. In that 
alert, ideally there should be not only an `OK' button, but also `Fix 
Filter...' and `Delete Filter' buttons which provide convenient access to 
likely responses (remember that these commands are also accessible outside the 
alert).

> 2) Your first Confirm() method does not have the buttonFlags param like
> ConfirmEx()

That's deliberate; not only does it encourage the use of informative action 
button labels for *all* confirmation alerts (rather than just those which the 
special ConfirmEx was used for), it also removes tacit approval for horrible 
`Yes' and `No' labels.

> 3) Along with UsernameAndPassword() and Password(), we should have
> Username().

What do you think of making Username() and Password() the same function, with a 
boolean flag to determine whether input is shown as cleartext or as bullets?

> The equivalent of the current Prompt() is gone.

I think I called Prompt() Confirm() by mistake ... I'll check when I wake up.

> As for Select(),

How do I invoke this so I can see it in action? It sounds like it's probably a 
dialog, rather than an alert.

Comment 8

[Matthew T (active 1999-2002)]

Yes, my mistake. The last `nsPromptService::Confirm' in my proposed API should 
be `nsPromptService::String' (since the name `nsPromptService::Prompt' is 
confusingly tautological).

Note that I'm assuming Confirm and ConfirmEx can be merged, such that if
*askWheneverLabel is null then the checkbox isn't shown.

Comment 9

[Conrad Carlen (not reading bugmail)]

> The last `nsPromptService::Confirm' in my proposed API should be
`nsPromptService::String'

Sounds good.

> Note that I'm assuming Confirm and ConfirmEx can be merged, such that if
*askWheneverLabel is null then the checkbox isn't shown.

Yes. That's the behavior of ConfirmEx right now anyway. That there are currently
3 variations on Confirm needs to change. Merging them is the answer.

I still think we need the buttonFlags param on Confirm. There are too many cases
in which the confirmLabel is "OK" and the cancelLabel is "Cancel" Forcing all
call sites to produce an actual string from a bundle for these will be a mess.
Having them default to "OK" and "Cancel" if NULL is passed isn't good either.
That would force strings to be fetched for the common Save/Don't Save strings.
The behavior of confirmEx WRT button titles is completely flexible, and saves
resource and code bloat in the case where common labels are used.

I now agree that having both message and advice params on Error(), Note(), and
Alert() is good - if the implementation makes the message bold and the advice
less visible. It will take work at call sites to make the message which is bold
short and descriptive enough. What some call sites are using now as the window
title is actually a message summary. Many others will need change.

I'll post a screen shot of a *good* confirm dialog for the sake of discussion.

Comment 10

[Conrad Carlen (not reading bugmail)]

Attachment: a good confirm dialog

Comment 11

[Conrad Carlen (not reading bugmail)]

The confirm dialog pictured is from CW7. It has
(1) no title
(2) a short description of the situation in bold
(3) more text the you might read but don't have to
(3) descriptive button titles
(4) identification of the app that is posing it (the large icon) For this item
on Windows, the impl could use the application name as the dialog title.

Comment 12

[David Epstein]

changing qa contact to Dharma.

Comment 13

[Conrad Carlen (not reading bugmail)]

taking for 0.9.6

Comment 14

[Matthew T (active 1999-2002)]

Thanks for taking this, Conrad! It will make Mozilla's UI so much nicer. (And 
it will pave the way for use of native alerts/sheets on Mac OS, which will give 
us OS notification of alerts opening in the background, alert text which is 
spoken aloud, and cool stuff like that.)

Yes, there are two platform-specific implementation details (I don't agree with 
either of them, but I think it's better to be consistent with the platform than 
to do our own thing):
*   on Windows, an alert should be titled with &brandShortName; rather than
    having an empty title bar;
*   on Mac OS X (but not Mac OS Classic), the application icon itself should be
    used no matter what the alert type. (In confirmation alerts the application
    icon should ideally be overlaid with a miniature /!\ icon, but that can
    probably wait until later.)

Comment 15

[Conrad Carlen (not reading bugmail)]

Thought I took it for 0.9.6... Well, then 0.9.7

Comment 16

[Judson Valeski]

going out on a limb here and calling this an enhancement.

Comment 17

[Matthew T (active 1999-2002)]

No, it's a major bug. Every single alert in Mozilla has the wrong icon
currently, except for the ones where people have rolled their own XUL so as to
avoid nsIPrompt altogether. This really needs to be fixed.

Comment 18

[Conrad Carlen (not reading bugmail)]

mass move to 0.9.8

Comment 19

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.9

Comment 20

[Conrad Carlen (not reading bugmail)]

CC'ing timeless.

Comment 21

[Mike Pinkerton (not reading bugmail)]

i still don't quite see how this bug will allow us to replace bad error messages
coming from w/in gecko with a more appropriate version in an embedding app (like
we can with security dialogs). This just adds more functionality to the
Confirm/Prompt API, not any context about the source of the dialog.

For instance, we'd like to be able to replace the cookie dialogs with more
appropriate messages and/or better worded text.

Comment 22

[Conrad Carlen (not reading bugmail)]

If the messages are not ideal for embedding, why shouldn't the wording be
improved in all cases? It sounds painful to have each text parameter in the API
be a key to an actual string for which the embeddor could supply their own value.

The reason I thought this was relevant is that the format of the dialogs for
nsIPromptService is wrong:
1) We rely in many cases on the title of the window to indicate the reason for
the dialog which is wrong for sheets.
2) Lack of methods which make a distinction between confirmations, alerts, and
errors.

If that was cleaned up by this bug, we would have a clearer dialog framework in
which the text of the messages could be improved.

Comment 23

[Simon Fraser [no longer active]]

> If the messages are not ideal for embedding, why shouldn't the wording be
> improved in all cases? It sounds painful to have each text parameter in the API
> be a key to an actual string for which the embeddor could supply their own value.

First off, I think the embedder should supply *all* UI strings. This way,
embedding apps can doing all of their localization in the platform-specific way,
and not have to mess with gecko's weird and wonderful localization schemes.

There are other good reasons for allowing embedders to provide UI strings:
1. They might want to include the product name in a string.
2. They might use different terminology than gecko
3. They might want to direct the user to UI that is specific to/different in
their application.

Comment 24

[Alec Flett]

what about making the embeddor implement their own nsIStringBundle, and allow
them to somehow selectively override specific bundles of strings with this
implementation?

That way their code plays nice with existing code in mozilla, we don't have to
tweak many (any?) APIs, and they can retrieve the actaul strings from wherever
they want. The other nice thing is that the implementation of nsIStringBundle is
pretty straight forward, requiring only a bit of extra code for the embeddor -
you mostly just implement getStringFromName/Id() and translate that call into
your own localization system.

on a side note, this means that we need to freeze
nsIStringBundleService/nsIStringBundle - please include me in that discussion as
well :)

Comment 25

[Håkan Waara]

There's constructive discussion going on here - I'll just remind you that the
main point of this bug is to change the way we currently misuse the icons for
all the wrong reasons as per comment 0.

Comment 26

[Conrad Carlen (not reading bugmail)]

Setting to milestone that's not passed.

Comment 27

[Simon Paquet [:sipaq]]

This bug is targeted at a Mac classic platform/OS, which is no longer supported
by mozilla.org. Please re-target it to another platform/OS if this bug applies
there as well or resolve this bug.

I will resolve this bug as WONTFIX in four weeks if no action has been taken.
To filter this and similar messages out, please filter for "mac_cla_reorg".

Comment 28

[Matthew T (active 1999-2002)]

Five years later, Microsoft are making this explicit in the Vista User Experience Guidelines: "Never use the warning icon for errors to make it feel less severe. Errors aren't warnings." <http://msdn.microsoft.com/library/default.asp?url=/library/en-us/UxGuide/UXGuide/Principles/TopViolations/TopViolations.asp>

Comment 29

[Alex Vincent [:WeirdAl]]

per comment 27, WONTFIX

Comment 30

[Michiel van Leeuwen (email: mvl+moz@)]

reopening. The bug is in os:all. Please read before wontfixing bug. (Especially when they are api bugs. It's unlikely for an api to be platform specific)

Comment 31

[Alex Vincent [:WeirdAl]]

mvl: I've just re-read the bug, and I'm not sure this bug is valid any more.  Seriously, no one's put any effort into the bug.  Firefox doesn't throw a dialog up for 404 pages, and has replaced SeaMonkey as the primary product.

This bug needs an active owner and active work if anything's going to happen... how many milestones have gone by in the last five years with not a single patch attached?  If anyone cares about this bug, they need to submit a patch for review and take this bug.

Comment 32

[Michiel van Leeuwen (email: mvl+moz@)]

I think this is still valid. It's about an API. For that, it doesn't matter what the main application is, and if it calls that api in one specific case, or not. It's still not possible to show a proper error dialog.
The fact that there is no owner also doesn't make the bug itself less valid. It just indicates that it is unlikely that it will ever get fixed.

Comment 33

[Matthew T (active 1999-2002)]

Updated steps to reproduce (these apply to Windows or Linux, not Mac OS X):

1. In Firefox 2.0, type "foo://bar.hum/" (without quotes) into Firefox's address field, and press Enter.
2. Look at the resulting alert.

Or:
1. In Firefox 2.0, try to save a Web page into a folder that is read-only.
2. Look at the resulting alert.

Or:
1. In Thunderbird 2.0, try to create a message filter with no name.
2. Look at the resulting alert.

Applications can and should reduce their use of error alerts, and Firefox's use of error pages for my original steps to reproduce is an example of this. But when applications do still use error alerts, they should be presented correctly. And with the current API, Firefox's and Thunderbird's error alerts always violate the UI guidelines of both Windows Vista and Gnome.

Comment 34

[Alex Vincent [:WeirdAl]]

I now see the error of my ways and repent.  :)  The bug still needs an active owner, though.

Comment 35

[Boris Zbarsky [:bzbarsky]]

So... this is asking for a change to the prompt service API, right?  With an errorAlert() function or some such on nsIPromptService2 ?

ccing some UI folks for input too.

Comment 36

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.