Closed Bug 95649 Opened 19 years ago Closed 4 years ago
Prompt Service doesn't know how to produce error alerts
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.
You don't need anything implemented. Just switch the icon on the appropriate alerts.
Conrad, what are your thoughts on this?
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.
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.
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.
> 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.
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.
> 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.
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.
changing qa contact to Dharma.
QA Contact: depstein → dsirnapalli
taking for 0.9.6
Assignee: adamlock → ccarlen
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.)
Thought I took it for 0.9.6... Well, then 0.9.7
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
going out on a limb here and calling this an enhancement.
Severity: major → enhancement
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.
Severity: enhancement → major
mass move to 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Mass move to 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
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.
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.
> 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.
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 :)
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.
Setting to milestone that's not passed.
Target Milestone: mozilla1.0 → mozilla1.5alpha
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".
OS: Mac System 9.x → All
Hardware: Macintosh → All
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>
per comment 27, WONTFIX
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
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)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
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.
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.
I now see the error of my ways and repent. :) The bug still needs an active owner, though.
Assignee: ccarlen → nobody
Status: REOPENED → NEW
QA Contact: dsirnapalli → apis
Target Milestone: mozilla1.5alpha → ---
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.
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.
Status: NEW → RESOLVED
Closed: 13 years ago → 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.