Closed Bug 31573 Opened 22 years ago Closed 20 years ago
The alert boxes come from the browser, not the engine. It's the code in nsWebShellWindow that provides the default title, I think. i don't know if the DOM level had a chance to sepcify something different.
Assignee: rogerl → trudelle
QA Contact: rginda → jrgm
Assignee: trudelle → rogerl
QA Contact: jrgm → rginda
Umm, why is this back to the engine? There's nothing I can do to fix this bug, the engine has no control over what the embedding calls are doing. Is XP Toolkit/Widgets not the right component? I'm assigning it to DOM0 then.
Assignee: rogerl → jst
QA Contact: rginda → desale
Nothing I can do about this in the DOM code, it's all in nsWebShellWindow AFAIK. Over to Don Melton (XPApps module owner).
Assignee: jst → don
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reassigning for Don
Assignee: don → ben
Target Milestone: --- → M20
email@example.com - did you mean to assign this bug to Ben Goodger? Changing severity - this isn't an enhancement IMO. It's 4.xP. Gerv
Severity: enhancement → normal
Move to M21 target milestone.
Target Milestone: M20 → M21
are these titles localized even?
You can see this dialog eliminating a bookmark in the Bookmark manager (right click + Delete). As Joseph points out it's not localized, and currently there's not a way to localize this title. (so I added rchen also to the CC list..)
bug 38523 deals with localizing the title.
Assignee: rogerl → valeski
I added a dialog title parameter which should be used for this.
Assignee: valeski → adamlock
Target Milestone: M21 → mozilla1.0
See also bug 64676. Currently, the page can specify any title for the alert that it wants.
OS: Windows 98 → All
Hardware: PC → All
So we have two suggestions: (1) to alter the title, and (2) to alter the icon. First, the title. Alerts shouldn't have titles at all. The exception is on Windows, where alerts should be titled with the name of the application (i.e. `Mozilla'), but that should be handled at the XP Toolkit level rather than by the calling function. There are two main reasons for this. Firstly, giving an alert a title is not useful for window-switching purposes (since alerts are modal to their parent window), and an alert title would practically never give the user enough information to allow them to avoid reading the contents of the alert at all. So the only thing giving an alert a title would do is slow the user down unnecessarily. Secondly, some platforms don't show title bars for alerts at all; the twm window manager didn't when I used Solaris, for example, and on Mac OS X the preferred method of displaying window-specific alerts is as title-less sheets. For those same two reasons, relying on the alert title to indicate the provenance of the alert is probably not a good idea -- it might not be visible at all, and people won't read it even if it is. Second, the icon. There are four types of alert, and these are distinguished by their icons as follows. (1) Error alert: (X) on Windows, hand in a stop sign on Mac OS. (2) Note alert: `i' in a bubble on Windows, talking face on Mac OS. (3) Confirmation alert: /!\ on both Windows and Mac OS. (4) Login/connection alert: bunch of keys on Windows, planet Earth on Mac OS. If you mix up these alert types and icons, you tend to confuse people. Unfortunately, Mozilla is currently hopeless in this regard: we have a bunch of the deprecated Windows-95-style `?' icons hanging around (bug 59820), we use the confirmation-alert icon for window.alert() when (as Henri noted) we should be using the note-alert icon instead (bug 75713); there are a bunch of places where we're using an alert icon for a type of window other than an alert (e.g. bug 81213); and we have at least one place where we're using a custom icon where we shouldn't (the drag-to-the-Home-button alert, for which I am to blame). Fixing the window.alert() icon bug would help, but we shouldn't do anything special to make icons for script alerts inconsistent with the icons for other alerts of the same type. We're left, then, with changing the content of the alert itself. This could be done in a similar fashion to JVMs, which put `Warning: Applet window' or similar at the bottom of a window opened by an applet. Brendan says that any such modifications can be erased, but I don't see how that could be possible if all the Web page can do is specify unformatted text for the alert. An additional nice touch would be to color the alert using the background and foreground colors of the page, rather than with the normal chrome colors (after doing a sanity check that the background and foreground colors of the page weren't unreadably similar). This would make it fairly obvious that the alert was emanating from the page, rather than from Mozilla itself.
*** Bug 93930 has been marked as a duplicate of this bug. ***
Small change to fix a significant security problem. nsbeta1.
Jesse or Mitch, can you point me to some sample code that does this checking? Or should we require chrome to use the prompt service? It seems most of the chrome already does with a few places that call the public JS methods. Obviously the latter entails more work.
Adam, I'll help with the checking that Jesse suggested. I need to think through how to do it.
If this doesn't get fixed first, it definitely needs a relnote.
I'm happy to submit an updated version of my original patch and forget about doing any clever checks to see what kind of JS is running. My opinion is that chrome is wrong if it calls these methods and should be fixed to use the prompt service instead. New bugs should be opened to cover those cases but generally the chrome is already doing the right thing.
dan, you have background on who's throwing what. any opinion on adam's last comment?
I'm inclined to not impose the restriction of chrome authors -- window.alert is easy and convenient, and it's being used. I see what looks like 28 .js and .xul files in Mozilla chrome that use window.alert, sometimes several times. It's a matter of documentation and expectation -- we haven't been warning off doing it the easy way in chrome. We'd have to start. I'd prefer to look up the call stack. It's not new code; we're already doing this in several places. nsScriptSecurityManager::IsCapabilityEnabled is doing something very similar. And it doesn't seem like a performance hit, since it'll happen fairly rarely and the task of opening an alert is already pretty large.
Dan can you point me to an existing example? Thanks
I and other xpfe super reviewers have been requiring use of the prompt service for a little while now, primarily because it's embarrassing to have chrome errors with titles of "Alert". If window.alert() is changed to have the app name as the title when called from chrome, then that's fine. Actually, it'd have the added benefit of not allowing our chrome authors to specify titles, which is good, since our titles are currently horribly inconsistent.
Well yeah, I guess that's what you gotta do. Looks generally good. A couple of comments: Just for posterity, I feel like mentioning I have vague uneasiness about referencing commonDialogs.properties inside nsGlobalWindow. But I asked around and no one seemed to mind, so that's not a real objection. A couple of points on efficiency: there's a fair bit of extra work done in nsGlobalWindow's constructor. It's not as if a typical web page contains so many DOM windows that this sounds like a big issue, but I do wonder if it'll affect page load times. And there is that new fetch of of the Script Security Manager Service for each alert or prompt posted. I suggest considering caching the SSM service (so alert/prompt doesn't have to fetch it from scratch) and delaying the construction of mScriptDlgTitle until the first time it's required (often never, I'd think), (outside the constructor). One thing, though. Is the DOM window the right place for it? Won't alerts and prompts posed directly through the Prompt Service miss all this good stuff? I gather from previous comments that window.alert and promptservice.alert are used interchangeably throughout our chrome. Since nsGlobalWindow goes through the prompt service, I'm curious why you didn't put this patch there. (That would of course add a requirement for all implementors of the prompt service that we'd have to document...)
New patch changes the following: 1. New method MakeScriptDialogTitle makes the title instead. Replaces the bundle code I had put in the constructor. 2. MakeScriptDialogTitle uses nsIStringBundle::FormatStringFromName for localisation purposes. 3. NS_WARN_IF_FALSE assertions for chrome callers to these methods.
Attachment #73716 - Attachment is obsolete: true
Comment on attachment 73870 [details] [diff] [review] New patch oh right, duh -- this is only for non-chrome alerts and prompts. This patch could still cache ScriptDlgTitle like in the previous patch, which kept mScriptDlgTitle rather than fetching it from the service each time. But r=me.
Attachment #73870 - Flags: review+
Code can no long cache the title because it's now a localized string containing a %S, so it has to be remade each time.
Comment on attachment 73870 [details] [diff] [review] New patch nice. sr=blake
Attachment #73870 - Flags: superreview+
Comment on attachment 73870 [details] [diff] [review] New patch a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73870 - Flags: approval+
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Opened bug 132420 to deal with do_GetService mess in nsGlobalWindow.cpp. Opened bug 132279 to deal with OS X not showing titles for any alert/prompt/confirm dialog.
Verified in Windows
*** Bug 64676 has been marked as a duplicate of this bug. ***
re-opening. please read my comments above & use testcase I attached.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Perfect. Got it. Sorry Adam I re-opened it for wrong reasons. Marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.