From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.17 i686; en-US; m18) Gecko/20001106 BuildID: 2000110608 The page in the URL (don't open it unless you know what you're doing!) "locks" the user in an endless stream of JavaScript alerts. There is no way out; closing the popup just opens a new one; UI is unresponsive in *any* place except the popup; you can't cancel the loading of the page (or do something like ESC to stop the script) since the UI is blocked by the popup. The perfect anti-Mozilla DoS. Reproducible: Always Steps to Reproduce: 1. Make sure there's nothing important open in Mozilla 2. Open the page 3. Try to escape the popup. You can't, you are stuck. Get ready to Ctrl+C Mozilla. Actual Results: An annoying popup which won't go away. All UI activity blocked by the popup. Expected Results: Allowed one to cancel the script, or at least close/stop/back the offending page. Not blocked UI in other windows. I found that URL while searching for 'bugscape' in google. I don't know the significance of that.

Confirmed.

DoS attacks are low on my priority list right now. Marking Future.

*** This bug has been marked as a duplicate of 29346 ***

Actually a dup of some part of bug 22049 (which currently needs to be split into several bugs).

Verified dupe.

Bug 22049 ended up covering too many different issues, so I'm reopening this bug. I'm turning this one into "Alerts should be content-modal, not window- modal"; see bug 61098 for another proposal that also might solve this problem.

I suggest placing a Stop button (no text, just a stop icon, so as to be unobtrusive) in the bottom left corner of any alert().

What would clicking the stop button do? Would it stop all network activity in the window and all scripts on currently loaded pages? (That's more than I think the stop button on the navigation toolbar should do.) Just prevent more modal windows from coming up?

The button would produce an additional alert asking if you wanted to stop all scripts in the current page.

Qa > CKRITZER

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

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

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

Boris, I don't think bug 123913 is a straight dup of bug 59314. Specifically, bug 123913 is an RFE for a crossplatform implementation of "sheets" (a la Mac OS X) to implement the content-modality. Also, bug 123913 presuposes a fix in tabbed browsing which may well be addressed (or not addressed) regardless of what happens in bug 59314. For example, bug 59314 might be fixed by providing a magic keycommand which aborts all scripts. I think... at least the bug 59314 comments indicate such... actually, bug 59314 seems to have a bit of a split personality, and might be best off *closed* in favor of two clear bugs: bug 123913 (which depends on bug 123908) and bug 61098) From the other direction, bug 123913 could be fixed, and bug 59314 would still hold: once you clicked into the offending tab, the tab would give you a sheet with the alert and you might still have an application totally locked if the stream of alerts never ended. So, since each bug can be fixed independantly, I don't think they're dups, and I'm de-duping... -matt

m_mozilla, I am in a quandary: look at bug 125829. It is clearly a dupe. but of this bug or of bug 123908 ? I would propose: 1) kill this bug. The DoS attack is covered by bug 61098 2) leave bug 123908 as-is (mac specific, tabs don't have sheets) 3) turn bug 125829 into an xp RFE for sheet-behaviour 4) make bug 123908 depend on bug 125829 Please people comment on this - good idea ?

Aaagh, ignore me I missed bug 123913, and I duped it to that.

Proposing to fix this bug for Mozilla 1.01.

ANdrew, are you offering to fix this? If so, assign it to yourself.

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

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

How simple would it be to add a user preference option under Advanced::Scripts and Windows to disable Javascript popup alert()'s in the mean time? In the history of my web browsing, javascript pop-up alerts have played a very small role. -Peter

one important aspect of this bug hasn't been mentioned yet (as far as i can see): i know *lots* of web developers who use "alert" as a quick hack to check the contents of some javascript or php variable Like <? $x="up"."down"; //<< Some calculations go here ?><script language="javascript">alert('<?=$x?>');<? //This is used to show $x ?> Now, if you accidentally put this in a large/endless loop you are stuck. This problem is far more common than some vicious website doing that so this might raise the importance of this bug.

This bug is invalid. Most window managers know how to make alerts window-modal (on the Mac, app-modal) or system-modal, but the number of window managers which know how to make an alert `content-modal' -- modal to some parts of the parent window, but not others -- is approximately zero. Even if there are one or two such window managers in existence, their lack of ubiquity makes it impossible to prevent the DoS that way. That can only be done by approaches such as the one in bug 61098.

Reopening. While the window manager might not know how to make a content-modal dialog, we could just make the dialog modeless (to the window-manager) but cause any click in the content area to focus it. Then the dialog would effectively be content-modal.

Yet another 'joke' - for russian speaking users :( http://fogi2000.narod.ru/photo.htm A botton inside alert dialog, wich will redefining "alert() = void()" whould be just fine :(

Bug 60150 is "Modal dialogs (alert, confirm, prompt) do not halt code execution", which is not what the web page developer would expect. Not a dupe, but related.

over to XP Apps to see if they have any ideas. This is a denial-of-service issue and therefore not that high priority, but I'd be interested to hear about possible solutions.

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

(In reply to comment #27) > over to XP Apps to see if they have any ideas. This is a denial-of-service issue > and therefore not that high priority, but I'd be interested to hear about > possible solutions. I'm still hopping through bugzilla breadcrumbs, so this may have been mentioned elsewhere already. But I came across a situation in Safari 1.2.3 (v125.9) where a non-selected tab suddenly got yellow "!" icon in it. When I selected the tab, a modal dialog popped up. (I wasn't able to recreate the behaviour with an alert in a non-selected tab, contrary to what I reported in bug 123908.) What if you did something like that for content-modal dialogs? It doesn't directly address the problem of freezing up the window when the currently selected tab has an alert, but it does ensure the only modal dialogs the user will see are for the tab they're currently viewing. Confusing situations like the one described in bug 123908 comment #2 would be avoided.

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

I want to add my wish to get this fixed too. I just visited some web page whose author had an image map, and due to a rather spectacularly bad mistake in his HTML code, he had made no less than *nineteen* rectangular areas, all overlapping each other perfectly, over the image map, each with the "onclick" attribute set to a Javascript alert. Guess what this meant when I accidentally clicked on part of the image that wasn't a link? Yes, you guessed it - I got nineteen consecutive Javascript alerts, unable to do anything in between to stop them. Even closing the browser window didn't help. If the author would have been brain-dead enough to make a perpetual loop of Javascript alerts I would have had to kill the entire browser process.

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

There *is* an easy way to get out, all you have to do is press Ctrl + W and click the OK button. There will probably be one more alert, and then the tab will be closed. However, I can't believe it's *that* hard to enable the user to disable JavaScript while alert windows are open... just un-block the browser window (I'd do it, but I'm too lazy to dig my way through the whole Firefox source code)

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

The Web Developer Toolbar has a "Disable JavaScript" functionality. Would it be that hard to duplicate that for a modal dialog's originating context?

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

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

I'm going to copy my comment from bug 61098 into here as to why this is a security issue. I could have 8 tabs open, be on the 8th tab, and the page in the first tab causes an alert. What happens? - An alert box appears totally interrupting whatever I am doing, even though I am not on the first tab - The alert box causes the tabs to switch to the first one, without asking me at all. It's a huge issue. It's a security issue as well, because the alert box steals focus! If I was entering a password that contained a space in it in the page I was on at the time, and my eyes were not on the screen, then large portions of that password could be captured by the offending page, since the space will close the alert. It would be simple to do a proof of concept of this hijack - site has a link that opens secure site in a new window, starts a timer, timer fires and steals back focus, hopefully grabbing part of the password. Password is then sent to rogue server via a post or AJAX.

I wonder why Firefox goes the heavyweight way of opening a separate window for stuff that's specific to an open page anyway. Why doesn't it use an overlay à la Flickr for that stuff or otherwise attaches the alerts, missing protocol handler notifications, and other stuff to the displayed page until the user has a look? Maybe tint the tab in red or something like that. Maybe attach the dialogs to the tab. However, all such dialogs need at least two additional options (maybe normally hidden in an "advanced" area): - Stop all scripts in the corresponding context: This one should be pretty obvious. As mentioned in bug 61098, duplicate the script slowdown functionality. - Close the corresponding context: self-explanatory. No warnings, no questions asked. Just throw it away.

I wanted to note that this is a more serious problem than has been implied several times on this list. For example, my impetus for commenting was losing a good bit of time this morning when a magazine submission form through an alert up about a form input error, and then somehow got stuck in an infinite loop putting up alert boxes. That was an error on the form's part. Forcing me to kill my browser from the console and lose the contents of all of my other, unrelated, tabs in order to deal with it was an error on Firefox's part. I find that alerts are pretty common in many large "legacy" sites (i.e. not Web 2.0, whatever that means). That includes document submission sites, coursework managers, some mail clients, etc, so the exposure here is much more than the very small probability of people doing this as a DoS attack. Long story short, window-modal dialogs for a tab-driven browser make zero sense. Of the remedies listed above, turning alerts into an overlay like many more hip sites do sounds the most appealing, followed by a button to kill that tab or script. Changing preferences, etc, are all just silly---they don't help people get out of the infinite loop, they just keep it from happening again, largely by crippling their usual experience. For the record, the problem I had this morning was using ScholarOne's Manuscript Central system handling IEEE Internet Computing's article submissions. I'm using Firefox 2.0.0.6 on this machine. Thx

My proposal: turn js:alerts into information bars. When there are multiple alerts following each other, display multiple infobars, descending downwards, but never display more than 5 js:alert infobars at once, instead pause the script until the user dismisses some js:alerts. This way, the user gets the alerts, but in an unobtrusive, non-blocking form.

Oh, and I almost forget: don't hardcode 5 as the most js:alert infobars allowed, make it follow a pref instead (preferably not hidden, but exposed in the Javascript/Advanced dialog).

(In reply to comment #43) > My proposal: turn js:alerts into information bars. When there are multiple > alerts following each other, display multiple infobars, descending downwards, > but never display more than 5 js:alert infobars at once, instead pause the > script until the user dismisses some js:alerts. You can't do this because a JS alert *has* to block the script until cleared.

Modified proposal: show the alerts one-at-a-time as infobars, blocking script execution. That would solve the window-modality problem. (Possibly we need to implement modal infobars as a new feature for this.)

Who says that alert() boxes have to conform to the OS theme? We don't have to do infobars per se, but it couldn't be hard to do some sort of overlay. I do see a potential problem, however: When a window is both small (less than a few square inches) and non-resizable, how would the overlay be displayed? Maybe this isn't a problem, but it is an edge-case.

fwiw, there are no more non-resizable content windows. See bug 177838.

(In reply to comment #48) > Who says that alert() boxes have to conform to the OS theme? We don't have to > do infobars per se, but it couldn't be hard to do some sort of overlay. What's wrong with an infobar? (Apart from having to implement a sort of infobar that blocks the execution of the script that called it, of course. But that wouldn't be so hard, I think.) An overlay is good on Mac, where it looks native, but not really on Windows or Linux. And it still covers part of the page, maybe immovably. We also need a "Stop script" button on the alert. If we choose the infobar, we might display a "Stop script" button on the infobar, while interpreting the "x" of the bar as the OK button. Or maybe a "Settings" button instead of a "Stop script" one, with extra choices like "Don't show more alerts on this page" for auto-clearing alerts, maybe printing them on the error console - more power to the user! Possibly with a "Disable scripts on this page" option as well. Fixes both the "modal storytelling" case (where there are 100 alerts telling a story or any other text line-by-line - the user sees the page content simultaneously with the "story", and can close the page without going through the alerts; or maybe choose "auto-dismiss" and read the "story" in a single block on the error console in case of an advanced user) and the "You must click Yes to get access" one (infinite loop throwing alerts in every iteration - "Stop script" or "Disable scripts permanently on this page" can be used to easily kill it, if we implement it).

(In reply to comment #51) > What's wrong with an infobar? (Apart from having to implement a sort of infobar > that blocks the execution of the script that called it, of course. But that > wouldn't be so hard, I think.) From an interaction design standpoint, infobars are more conducive to parallel interaction. They are non-blocking in both behavior and appearance. Indeed, that's what users expect of infobars. If you create a new kind of infobar that has blocking capability, users will not realize what has happened. ("Why is the page frozen?") Remember, infobars are not highly visible (I know this from watching users), and I think that is part of their design. > An overlay is good on Mac, where it looks > native, but not really on Windows or Linux. And it still covers part of the > page, maybe immovably. You raise a good point: The dialog interface should *not* block visual access to the page. Users may need to look at the page before deciding on a course of action. > We also need a "Stop script" button on the alert. If we > choose the infobar, we might display a "Stop script" button on the infobar, > while interpreting the "x" of the bar as the OK button. That feels too much like using a screwdriver as a hammer. Firefox already suffers from clunky and unintuitive interfaces in a number of places, and I think using an infobar as a modal component would not help. > Or maybe a "Settings" > button instead of a "Stop script" one, with extra choices like "Don't show more > alerts on this page" for auto-clearing alerts, maybe printing them on the error > console - more power to the user! Possibly with a "Disable scripts on this > page" option as well. Fixes both the "modal storytelling" case (where there are > 100 alerts telling a story or any other text line-by-line - the user sees the > page content simultaneously with the "story", and can close the page without > going through the alerts; or maybe choose "auto-dismiss" and read the "story" > in a single block on the error console in case of an advanced user) and the > "You must click Yes to get access" one (infinite loop throwing alerts in every > iteration - "Stop script" or "Disable scripts permanently on this page" can be > used to easily kill it, if we implement it). I'm not sure I understand these use-cases. Probably should keep it simple and implement it the way Opera does: Add a checkbox and text under the button area in the existing dialog box.

I have not said that it should block access to the page. It should block the script that called it, however. That is, the page should NOT freeze. Only the script. Maybe make this infobar red, for discoverability. About the use cases: It is becoming a common prank to link to pages with interesting link texts (like "Mozilla Firefox 4 released!" or "Windows 7 beta leaked!"), where the actual page contains a long text ("story") that's printed in alert boxes. A specific case (all lines are in separate popups that open after the previous box has been OK'd out): "You have been trolled! No way to escape! Maybe you are too curious... You shouldn't have clicked on an unknown link! You won't leave for hours! I'm sure! Kinda like Zero wing, isn't it? All your base are belong to me! You know, 'What happen? Somebody set up us the bomb! We get signal! What you say? Main screen turn on...." and so on. In this case, choose Auto-dismiss. The alert texts get dumped out on the error console, while the page is shown as if the user has clicked through the annoying dialogs. This is not to be confused with the second case, as here the popup stream is finite, and non-repetitive. (Another case when such an annoying popup line is used is to represent something like the "Angel.pps" files sent in e-mails as attachments. Each slide comes in a separate alert.) The other, worse case is where the popups are produced by an infinite loop, printing the same single line over and over again. This is different because in this case, the user definitely doesn't want to read the text that is presented in the popups (because it's simply a repetition, not a long text, possibly an interesting, but finite story as in the first case). Also, here auto-dismiss just doesn't work, as it produces an infinite loop, and thus, a freeze. Here, the solution is stopping the script AND disabling scripts on that page, to prevent any further abuse of the page. Improves Mozilla's reputation as a porn browser (do we really want that?! I don't know... :-), while fixing a DoS vulnerability.

(In reply to comment #53) > I have not said that it should block access to the page. It should block the > script that called it, however. That is, the page should NOT freeze. Only the > script. Correct me if I'm wrong, but if the script freezes, then arbitrary content on the page may freeze (e.g., ignore user input), depending on how the scripts are written. That suggests, as Tim McCormack says, that some kind of notification should be used that's blocking in appearance. alert() is typically used when the user is intended to see the alert before doing anything else on the page. Yes/no popup dialogs (forgive me, I forget the function call for those in JS) even more so, and those fall under the same category in terms of abuse. The issue is that such pop-up dialogs should not block *other* tabs, or other browser functionality such as the menus or "close tab" button. Alerts still need to block the tab that opened them.

I don't think so. Only interaction with the tab, at most. It should still remain scrollable. Blocking access to the page is bad. But as you mentioned, a similar fix is needed for confirmation boxes and info entry dialogs (I also don't know the name of the function call).

Stefanik: your suggestion either breaks the "always runs to completion" property of in-browser JS or leaves the user with a partially broken page. In other words: what happens if JS needs to run in response to user's action when the alert() infobar is up? Either you don't run it until the infobar is dismissed (in this case the page looks broken - no events are handled) or you run it anyway, possibly breaking the expectations the script, that called alert(), has about the browser state. Anyway, this discussion doesn't belong here. File a separate bug about your infobar idea or take it to the newsgroups. Bugs with lots of discussion are unnecessarily hard to work with.

Nickolay: you misread my post. I only want the page to be scrollable while the alert is visible. Links or buttons not working is a different case. Currently, one can sometimes view the page that thrown the alert while the alert is still up (by dragging the alert window away), but because the alert box is window-modal, the user can't scroll the page to view its invisible regions. If access to the tab is completely blocked, then the user again can only see the portion of the page that was in the viewport when the alert was thrown. So, the page content shouldn't be interactive, but the non-content area *and the scrollbar* should still work.

I found another example of this... even with all the javascript tweaks in place trying to close the tab is an exercise in frustration http://www.bringvictory.com/

Another page found and it spreads malware. This is getting out of hand. Can we please get a fix. This is a HCI issue (window modality) and also a security issue (javascript dialogs can 'user-force' a malware download through looping).

This bug will soon be celebrating its 8th birthday. It's even older than Firefox itself. I voted for it years ago and still run into it once in a while. I think it is getting pretty embarrassing that there is still no solution for this and duplicates keep being filed. Recently I ran into this bug again with a website that was missing some JavaScript files and was spawning alerts and stealing focus for each page opened. Firefox 3 on Mac OS X uses Growl for unobtrusive notifications about updates and downloads. I would like it very much if JavaScript alerts would work the same way, appear and disappear without halting execution, stealing focus, and requiring user interaction. But really, anything that does not steal focus would be nice.

another example for malware exploiting this bug: http://scan.scannerantispyware.com/338/3/

Wouldn't it be easier to count how many alerts have been called on a page and if it gets over a certain limit provide another dialog to allow the user to get out of there, or go back a page.

[Coming from bug 450873, is it a duplicate of this one?] I agree on this behavior can get really bothersome, and it's a major issue of the UI. A special case is when resuming a previous session. If you have more than one tab open for the same authenticated site, then you will get as many requests as tabs you have, even if it's the same and you did authenticate, so that's another flaw (IMHO). Overall, I believe these alerts being constrained to the container tab (and not taking focus) is the best way, but also helping the user notice the tabs with an alert in it, maybe with a (theme-configurable) flashing and/or styling. —I think this is/was Opera's behavior with popup windows: they remain "inside" its parent tab.

The Chrome solution to this problem is very elegant - the second (and every following) popup, has a checkbox option to prevent more popups from the same page (see description on http://blog.spathare.com/2009/02/chrome-prevents-javascript-alert-loop/ and http://www.nczonline.net/blog/2008/09/06/chrome-tames-wild-dialogs-and-popups/) Hope to see Firefox improve here.

IMHO that isn't elegant - It's a hack. It might prevent the very worst of the problems from this bug from happening, but it's can break webpages and isn't completely understandable or explainable to the user. Nothing from the content should be anything stronger than content modal, no matter what hacks are put in place to make it 'safe'.

It may be a hack, but it is better than anything any Mozilla product has to offer, and as such it should be at least considered. Presentation is lacking, I'll give you that. The checkbox title is not informative to a user, it's rather babble in the good old "snotweasel omegaforce" sense. However, I would suggest something like it, maybe with a text link in the modal box that a user can click for details. In any case it should be something that the caller can never fake, and which is unobtrusive enough to not distract the user's attention from the contents, i.e. a good old blue link. The details should include: * the title of the parent context, * the URL of the parent context, * a clearly labeled way to disable all JavaScript execution for that context for the time being, and * a way to do that permanently for the calling context. If a user decides to disable scripts in a context, there should be a clear notification of that, stating that behavior in that context may be different than expected. Furthermore, the user must be given an opportunity to re-enable scripting (which should probably also raise a warning that site behavior may be severely changed by the interruption, and suggest to start a new session). The handling of modal messages in Firefox is generally changing for the worse, giving this bug an ever rising importance. Right now, I'd go so far and say that modal messages on all levels are completely broken, with the browser happily presenting n different cookie confirmation dialogs for n cookies from the same site (this is a severe regression from FF2), wildly changing tabs when an HTTP connection calls for authentication, and the "small" issue covered by this bug. I am also very tempted to become very impolite at this point, seeing as this bug is STILL NEW!

By all means create a spin-off bug for the stopgap.

I am not going to open new bugs here, since I quite frankly don't think anyone even bothers anymore. I'm on CC for a few bugs that are about 8 years old, and that frequently get stuck in useless discussions before nobody cares about them for two more years. However, I invite you to use whatever I write and create spin-off bugs yourself. Just keep me off the CC.

Until recently, the alert boxes were not stealing the focus on Linux, so it was possible to just close the tab in the main window. The current behaviour of preventing the user from doing anything in the main window should be changed back.

Not blocking 1.9.2 on this.

(In reply to comment #79) > Not blocking 1.9.2 on this. I/We? know, UI and Usability / User Experience doesn't seem to block anything... which, in turn means that this could stay open for (more) years. Too bad, but it's a great application nevertheless.

(In reply to comment #78) > Until recently, the alert boxes were not stealing the focus on Linux, so it was > possible to just close the tab in the main window. The current behaviour of > preventing the user from doing anything in the main window should be changed > back. Fixing this would sort the single biggest problem with modal alerts on Linux, which is that a simple loop invoking alert() repeatedly can render the entire browser unusable, without any way to navigate away from the page or tab that is causing the problem.

Inexperienced users (aka, my parents) get trapped by this all the time. Why is it still an issue? Please supply an interim fix while you're working on the all-singing all-dancing one. For example, shift-click on the close button of the alert should stop the script that spawned it. At least then I can give some over-the-phone advice on how to get out of an alert loop.

This is a quick (mere hours of work) and dirty (oh so dirty) hack to support a "Stop Script" menu option on the context menu of javascript Alert dialogs. Selecting the menu option and then pressing OK on the dialog will stop the script that started it. It aint pretty but it does the job.

Comment on attachment 400436 [details] [diff] [review] Add a menu item to the context menu in alert dialogs to stop the script that caused it. Igor, if you're not the right person to review this, please pass it on to someone who is. Thanks.

That patch belongs on bug 61098, not here.

The interwebs have invented a great appearance for page-modal dialogs, called a "lightbox", where the page is dimmed except for the active bits. I think we should adopt it for alert(). This would replace the ugliness that is window-modality and "The page at https://bugzilla.mozilla.org/ says:".

Comment on attachment 400436 [details] [diff] [review] Add a menu item to the context menu in alert dialogs to stop the script that caused it. I am not the right person for this. For the right person see http://www.mozilla.org/about/owners.html . Alternatively, a hg log for the files that the patch touches will show comments and persons who reviewed the previous changes.

+1 on lightbox. However, I do wonder on how to suspend JS within the tab and not within the whole window. Show me how to do that, and this may be something I implement in a hurry.

I don't like the idea of a 'stop script' button. If a site is giving you endless alert dialogues then all the user needs is the ability to close the tab/window. No one wants to stay on a site that's got abusive Javascript stuff on it. The solution to this is to stop the alert from blocking the whole UI. It should let the user close the tab. You can also screw things up with this: <script> document.onblur = function(){ alert('this will screw it up'); }; document.onblur = function(){ alert('when combined with this'); }; </script>

(In reply to comment #91) > I don't like the idea of a 'stop script' button. If a site is giving you > endless alert dialogues then all the user needs is the ability to close the > tab/window. No one wants to stay on a site that's got abusive Javascript stuff > on it. I usually do want to stay on a site that has runaway javascript. accidental runaway and poor design are far more common than deliberately malicious. other browsers have a "stop script" button on alert boxes, the UI is simple and seems to work well.

I have noticed a bug that is closely-enough related to this one that I think that a new bug-report would be considered a duplicate. OS: Mac OS 10.4.11 Steps to reproduce: 1. go to https://bugzilla.mozilla.org/attachment.cgi?id=192603 2. Open the Activity Monitor utility (Finder ▸ Applications ▸ Utilities ▸ Activity Monitor) 3. If the running-processes window doesn't open up automatically, go to Window ▸ Activity Monitor (keyboard shortcut ⌘-1) 4. Find the Firefox process, and select it. 5. Click the "Quit Process" icon in the upper-left of the window. 6. Click on "Quit" in the dialog box. Actual behavior: Nothing happens. Expected behavior: Firefox quits gracefully. This is a low priority issue because you can still quit Firefox the normal way (Firefox ▸ Quit Firefox or ⌘-Q), or if you prefer you can choose "Force Quit" in the dialog box that appears in step (6) above, but still I think that it should be addressed.

What is the relation? Force Quit dialog is platform related, it's not called by Firefox using its technologies... Because Firefox doesn't respond. I'd close your bug as INVALID if I could.

re Comment 95, Comment 94 He's not talking about the Force Quit dialog, he's talking about the Force Quit option in the Activity Monitor, which has distinct options "Quit" and "Force Quit". I expect the "Force Quit" option does the same as the Force Quit dialog; a quick google search failed to find anything specific about what "Quit" does, but it clearly gives the application a chance to clean up or even not exit. When you open the link in Comment 94 (WARNING, that will force you to kill your browser!), Firefox will not quit with the "Quit" option, only with the "Force Quit" option. (At the startup page, Firefox will quit with either option.)

> Steps to reproduce: > 1. go to https://bugzilla.mozilla.org/attachment.cgi?id=...... > 2. Open the Activity Monitor utility (Finder ▸ Applications ▸ Utilities ▸ > Activity Monitor) .... Sorry, I forgot to mention that *THE URL MENTIONED IN STEP 1 OF MY COMMENT 94 ABOVE WILL LOCK YOUR BROWSER* It's the same as the first attachment in bug #61098. Basically it produces an endless loop of JavaScript alerts. Anyway, here is what I would prefer Firefox to do: * when a Javascript alert box appears, allow user to switch windows, but not change to a different tab in the same window. That's because, cosmetically, the alert box appears as part of the window, so from a UI standpoint, this would seem to be the most logical thing to do. In this sense, the dialog should be window-modal. * Likewise, when the alert box appears, pause execution of all running scripts, plugins, or other third-party executables in any tab of the current window until the user makes the box go away, and pause any scripts, plugins, or third-party executables in other windows until the user either makes the box go away or brings the other window to the foreground. * Respond normally to the "quit" (⌘-q in Macintosh) function in the file menu – this is what Firefox already does – but (on the Macintosh) also respond the same way to any "quit process" signal that is sent from Activity Monitor. * Do not disable the "close window" (alt-F4 in Windows or ⇧-⌘-W on Macintosh) or "close tab" (Ctrl-w in Windows or ⌘-w on Macintosh) functions in the File menu. There is no reason the user should be unable to kill the current tab. (I think that's the heart of what this request is all about.) * Add a new keyboard shortcut (for example, ctrl-break in Windows or ⌘-. on Macintosh) that, when activated, will halt the execution of all scripts in all Firefox windows. * Currently, the alert box says at the top "The page at .... says": which is good, because it identifies that it's the web page and not the OS that is generating the box. But I'd also like to add, at the bottom of the alert box in fine print, something to the effect "To close the above web page, press ⌘-w" and "To exit all running scripts, press ⌘-." (See new attachment. The fine print should appear after the script has been running or the box displayed longer than 5 seconds, or if a single page generates more than one alert box.)

пожайлуста, пишите по русски)

krishtal, учи английский или убирайся!

Danny, повежливей уж давай. На самом деле баг - стыдобища: открыт в 2000-м году, критикал, позволяет заблокировать работу броузера и до сих пор не пофиксен. Мне периодически сыпятся на мейл уведомления о том, что в этом треде что-то написали, на кого-то баг переассигновали. К сожалению, дальше балабольства дело не трогается. Посему имхо без разницы на каком языке будет идти беседа - хоть на китайском: результата-то всё равно нет.

Dmitriy: Maybe Danny could have said it more politely, but he is correct. You are in some way right that it doesn't matter what language these recent comments are in - because none of them are useful. If you are not adding useful information that helps to fix the bug ("this bug is old and is not fixed" is not useful), then please don't waste everyone's time. See https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

In the interests of transparency, I'd like to point out that this is now being looked at in bug 562258. Interested parties can CC themselves there. Please don't comment without an extremely good reason.

More than DoS, this bug adds a challenge in debugging, e.g., when using the otherwise convenient live evaluation of JavaScript in the XUL Editor or JS Environment of the Extension Developer's Extension.

Picking up this bug, now that the initial prototyping from bug 562258 is complete. Patches coming up. [And in a futile attempt to head off the inevitable: please keep comments on this old-and-slightly-crufty on topic and constructive.]

Mostly complete patch; needs a bit of cleanup still, and needs some more events/observers to handle closing a window/app, since TabClose events are not fired then.

(In reply to comment #111) > Patches coming up. oh no! i totally had started party planning for its 10th birthday ...

Marking blocking betaN per beltzner.

Comment on attachment 478933 [details] [diff] [review] Patch v.2 >+function getTabModalPromptBox(aWindow) { >+ var foundBrowser = gBrowser.getBrowserForDocument(aWindow.document); >+ if (foundBrowser) >+ return gBrowser.getTabModalPromptBox(foundBrowser) Is it just me or is there a missing semicolon?

Comment on attachment 478933 [details] [diff] [review] Patch v.2 [ ...snip... ] >diff --git a/toolkit/components/prompts/content/tabprompts.xml b/toolkit/components/prompts/content/tabprompts.xml >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/prompts/content/tabprompts.xml [ ...snip... ] >+ <method name="abortPrompt"> >+ <body> >+ <![CDATA[ >+ this.linkedTab.removeEventListener("TabClose", this, false); >+ window.removeEventListener("unload", this, false); >+ this.Dialog.abortPrompt(); >+ this.parentNode.removeChild(this); >+ ]]> >+ </body> >+ </method> What happens when you "undo close tab" on this tab? I don't think the state will be properly restored. This was never really an issue before since you couldn't dismiss a modal dialog by just closing the tab... [ ...snip... ] >+ // XXX oops, this doesn't yet deal with having the tab/windows closed >+ // without answering the prompt, you'll hang on shutdown. :) Lol, this kind of answers the question (in that it hasn't been dealt with fully yet...) :) This patch looks cool, can we get a try build to play around with? If you don't have time, I might just fire one up myself...

(In reply to comment #116) > Is it just me or is there a missing semicolon? Oops! (In reply to comment #117) > What happens when you "undo close tab" on this tab? I don't think the state > will be properly restored. This was never really an issue before since you > couldn't dismiss a modal dialog by just closing the tab... JS state has never been restored after closing a tab, this is no different. > >+ // XXX oops, this doesn't yet deal with having the tab/windows closed > >+ // without answering the prompt, you'll hang on shutdown. :) > > Lol, this kind of answers the question (in that it hasn't been dealt with fully > yet...) Actually, that comment is a lie now. This version of the patch handles those cases.

Pushed this patch, along with the series in bug 575485 to try, builds should show up later tonight here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/?C=M;O=D Only ran the build machines though, no tests at all (and only desktop, not mobile).

Builds becoming available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/highmind63@gmail.com-afa401d6ab23/ Two things I noticed from some quick playing around with this: 1) The styling makes the alert seem like it's deeper than the background as opposed to beveled (styling isn't a big deal at this point though, I take it). 2) Once you loop through alerts more than the MAX_CONCURRENT_DIALOG_LIMIT amount, you run into bug 61098, which throws up the [...]Ex versions of the dialogs, e.g. alertEx, confirmEx, promptEx. This need to be implemented in this patch as well, as of now the old app-modal dialog comes up...

(In reply to comment #120) > 2) Once you loop through alerts more than the MAX_CONCURRENT_DIALOG_LIMIT > amount, you run into bug 61098, which throws up the [...]Ex versions of the > dialogs, e.g. alertEx, confirmEx, promptEx. This need to be implemented in this > patch as well, as of now the old app-modal dialog comes up... ISTM that the changes for bug 61098 are not needed and could be disabled when content-modal alerts are used because the user can escape by closing the tab.

(In reply to comment 120) s/Ex/Check/g - got confused by the impl.

(In reply to comment #121) > ISTM that the changes for bug 61098 are not needed and could be disabled when > content-modal alerts are used because the user can escape by closing the tab. They could be disabled, but I think it would be better to keep them - keeping them will ensure that the user can always view whatever is already rendered.

Small update mostly around how getTabModalPromptBox() is implemented/used.

For risk reduction, this makes tab-modal prompts opt-in for nsIPrompt callers, and adds a pref for globally disabling tab-modal prompts. Implements the opt-in for the JS alert()/confirm()/prompt(). [To be clear, it's not giving the page control over the prompt style, but rather giving our implementation of the prompts control.] Flagging jst for the nsGlobalWindow change, gavin can do the rest.

In nsDocumentViewer::PermitUnload ConfirmEx is used which ends up calling through to the old modal prompt service. E.g. in Gmail, start creating a message then close the tab.

(In reply to comment #126) > In nsDocumentViewer::PermitUnload ConfirmEx is used which ends up calling > through to the old modal prompt service. E.g. in Gmail, start creating a > message then close the tab. The opt-in stuff in the Part 2 patch means that prompt will continue to be window-modal. It, and other prompts, can be addressed in followup bugs after this lands.

Comment on attachment 483059 [details] [diff] [review] Part 1 (main patch), v.3 >diff --git a/toolkit/components/prompts/content/commonDialog.js b/toolkit/components/prompts/content/commonDialog.js >+// TODO: how to call Dialog.onButton1 (cancel)? What's this about? >diff --git a/toolkit/components/prompts/content/tabprompts.xml b/toolkit/components/prompts/content/tabprompts.xml >+ <binding id="tabmodalprompt"> >+ <resources> >+ <stylesheet src="chrome://global/skin/tabprompts.css"/> >+ </resources> >+ >+ <implementation implements="nsIDOMEventListener"> Can you put <content> first? Matches what we do in most places. Also the indentation for this <implementation> block is messed. >+ <constructor> >+ binding.ui.button0.addEventListener("command", function() { binding.onButtonClick(0); }, false); >+ binding.ui.button1.addEventListener("command", function() { binding.onButtonClick(1); }, false); >+ binding.ui.button2.addEventListener("command", function() { binding.onButtonClick(2); }, false); >+ binding.ui.button3.addEventListener("command", function() { binding.onButtonClick(3); }, false); I think putting these inline on the <content> (as this.Dialog.onButtonClick?) would be better, per comments in the other bug. >+ <field name="ui"/> >+ <field name="args"/> >+ <field name="linkedTab"/> >+ <field name="Dialog"/> >+ >+ <property name="defaultButtonNum"> Don't really understand the purpose of this. No one seems to set it, and the only getter (onKeyAction) seems like it could just use this.args.defaultButtonNum instead. CommonDialog.jsm already handles setting the "default" attribute on the button. >+ <method name="init"> >+ // TODO: on windows, unhide buttonSpacer iff there are 3+ buttons Seems like instead we should remove the unused button 3 in a followup. >+ let tmp = {}; >+ Components.utils.import("resource://gre/modules/CommonDialog.jsm", tmp); >+ this.Dialog = new tmp.CommonDialog(args, this.ui); >+ this.Dialog.earlyInit(); >+ this.Dialog.onLoad(null); Need to update this for the CommonDialog.jsm changes. >+ <method name="handleEvent"> >+ case "TabClose": >+ case "unload": >+ case "pagehide": >+ this.Dialog.abortPrompt(); Shouldn't this call this.abortPrompt(), to also remove the listeners? >+ <method name="abortPrompt"> >+ this.linkedTab.removeEventListener("TabClose", this, false); >+ window.removeEventListener("unload", this, false); Don't you need to remove the pagehide handler as well? >+ <method name="onButtonClick"> >+ this.linkedTab.removeEventListener("TabClose", this, false); >+ window.removeEventListener("unload", this, false); I guess a "removeHandlers()" would be handy. >+ <handler event="focus" phase="capturing"> >+ // Tab changes the default button. >+ // TODO: I don't get the dialog.xml code, what's the "default" >+ // attribute do? The other code there seems to use >+ // "defaultButton". It's just used for styling: http://mxr.mozilla.org/mozilla-central/search?string=button%5Bdefault It gets set in dialog.xml via _setDefaultButton. >diff --git a/toolkit/components/prompts/src/CommonDialog.jsm b/toolkit/components/prompts/src/CommonDialog.jsm >+ // XXX if this opened in a background tab, do we still want to focus? >+ button.focus(); I think so - shouldn't have any effect on other tabs, but will mean the button is focused when you switch back to this tab. >diff --git a/toolkit/components/prompts/src/nsPrompter.js b/toolkit/components/prompts/src/nsPrompter.js >+ getTabModalPrompt : function (domWin) { >+ // If any errors happen, just assume no notification box. "no modal prompter" >+function openTabPrompt(domWin, tabPrompt, args) { >+ if (args.promptAborted) >+ throw Components.Exception("prompt aborted by user", Cr.NS_ERROR_NOT_AVAILABLE); Hmm, why treat aborts differently from cancel? Seems analoguous to just clicking the "X" on the dialog, which we already treat as cancel. > function ModalPrompter(domWin) { > this.domWin = domWin; > } > ModalPrompter.prototype = { > domWin : null, >+ allowTabModal : true, Wasn't the idea to have a PrompterImpl() base class, that TabModal and Modal inherit from, and then do the picking in pickPrompter? getPrompt would need to be changed to use pickPrompt too, and the nsIWritablePropertyBag/allowTabModal stuff can be on Prompter itself (used in pickPrompter).

Would you call window.notifyDefaultButtonLoaded(defaultButton) when the dialog becomes visible? It supports "snap to default button" on Windows. See bug 76053.

OK, so this problem has been around for 10 years now and no-one is going to fix it?

(In reply to comment #131) > OK, so this problem has been around for 10 years now and no-one is going to fix > it? Ok, so this bug is 10 years old but no-one actually reads it before posting? Sarcasm to plain English translation: This bug is being worked on in recent comments here and in other related bugs. This bug is blocking2.0:betaN+ (the "2.0" is the Gecko version underlying Firefox 4.0) which means a fix will be in one of the future Firefox 4.0 betas once ready. It is not a trivial fix, hence the 10 year wait, but yes, it's going to be fixed eventually. Just because a bug is up into the hundred plus comment range is not an excuse to not read them all before posting. Please just wait patiently for the fix like the rest of us. https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

OK, that was a great summary - thanks! Sarcasm noted and appreciated :-) I honestly tried to read all the comments, but just got lost in the verbiage. It is hard to be patient when you can't understand the verbiage. By the way, a concise summary is excellent etiquette. Thanks again.

Comment on attachment 483059 [details] [diff] [review] Part 1 (main patch), v.3 There's a problem with this that I discussed with dolske on IRC: dialogs need to not rely on their event loop terminating to be dismissed, otherwise new dialogs block closing old dialogs.

Comment on attachment 483061 [details] [diff] [review] Part 2 (opt-in tabmodal), v.1 This looks good, but would need to get the nsIWritablePropertyBag2 from "promptFac" rather than "prompt" if we go with the approach from the end of comment 128. Though now that I think that through, that wouldn't really work, since that object is a singleton and you don't want to change the behavior for subsequent callers :(

(In reply to comment #128) > >+// TODO: how to call Dialog.onButton1 (cancel)? > > What's this about? Oops, old comment that no longer applies, removed. > Also the > indentation for this <implementation> block is messed. Ugh, not sure what happened there. Fixed. > >+ <constructor> > >+ binding.ui.button0.addEventListener("command", function() { binding.onButtonClick(0); }, false); ... > > I think putting these inline on the <content> (as this.Dialog.onButtonClick?) > would be better, per comments in the other bug. Well... The problem is that |this| in the oncommand is the XUL element, so it needs some ugly code to get up to the actual binding (where |Dialog| is at). And there are two sets of buttons (#ifdef'd, since the order is different on OS X). So I've just stuck with adding event listeners in the constructor. The pluginProblem binding does basically the same thing for the same reason... > >+ <property name="defaultButtonNum"> > > Don't really understand the purpose of this. Hmm, yeah, looks like I overthought this. Simplified as suggested. > >+ // TODO: on windows, unhide buttonSpacer iff there are 3+ buttons > > Seems like instead we should remove the unused button 3 in a followup. Yeah. Filed bug 609510. > >+ <method name="handleEvent"> > > >+ case "TabClose": > >+ case "unload": > >+ case "pagehide": > >+ this.Dialog.abortPrompt(); > > Shouldn't this call this.abortPrompt(), to also remove the listeners? Oops, yeah. Although this is the only callsite for this.abortPrompt, so I just inlined that. I think that's a leftover before I was routing these events through handleEvent. > toolkit/components/prompts/src/nsPrompter.js ... > > >+function openTabPrompt(domWin, tabPrompt, args) { > > >+ if (args.promptAborted) > >+ throw Components.Exception("prompt aborted by user", Cr.NS_ERROR_NOT_AVAILABLE); > > Hmm, why treat aborts differently from cancel? Seems analoguous to just > clicking the "X" on the dialog, which we already treat as cancel. I'm basically just copying the behavior from how the "don't let this page open more prompts" checkbox works (bug 61098). Although I guess bug 61098 comment 209 implies this was done because only "broken code" should trigger dialog loops, and not specifically as a way to indicate something other that ok/cancel happened. But, would it even actually matter here? The tab is being closed when this happens, so it's basically Game Over for the page no matter if we return or throw. > Wasn't the idea to have a PrompterImpl() base class, that TabModal and Modal > inherit from, and then do the picking in pickPrompter? getPrompt would need to > be changed to use pickPrompt too, and the nsIWritablePropertyBag/allowTabModal > stuff can be on Prompter itself (used in pickPrompter). Sorta. I started down the route I had originally intended in the earlier versions of this patch (see bug 562258, now duped to here). The problem was that the TabPrompter implementation basically duplicated everything in ModalPrompter. This also allows for more opportunity to delay deciding between window/tab modal until a prompt is actually shown, lest something change between the time something calls GetPrompt and when it decides to show a prompt. Since there was so much overlap, doing inheritance didn't really seem like it was worth it.

Review comments addressed, except for issue noted in comment 134. Checkpointing changes so far...

(In reply to comment #136) > But, would it even actually matter here? The tab is being closed when this > happens, so it's basically Game Over for the page no matter if we return or > throw. Right, my point was mostly that there's no need for a separate flag (and the code that watches for it and throws in openTabPrompt()). > Since there was so much overlap, doing inheritance didn't really seem like it > was worth it. They could both "inherit" from the same base class to avoid the duplication, which would have made this rather elegant IMO. Comment 135 kind of makes that a nonstarter, though, so I guess we should go the other way and get rid of pickPrompter() (though not necessarily here).

Oh no, I've missed the bug's anniversary! It's now 10 years old! Ten, damn, years old! Happy birthday! Is it a record, that such a bug managed to stay alive for so long?

(In reply to comment #139) > Oh no, I've missed the bug's anniversary! It's now 10 years old! Ten, damn, > years old! Happy birthday! > Is it a record, that such a bug managed to stay alive for so long? No and your comments really only serve to distract people who might want to work on the bug so please only comment if you have something useful to say.

I just remembered something else - nsXULWindow::ShowModal pushes a null entry onto the JS context stack while the event loop spins (presumably because we have code that checks that stack for determining privilege? mrbkap/jst know more). Do we need to do something like that here too? That would involve keeping the event loop spinning in C++...

Justin, would you check my previous comment (comment 129)?

Addresses comment 134 (modulo bug 611566), and cleans up a few todo/xxx/dumps. (In reply to comment #138) > Right, my point was mostly that there's no need for a separate flag (and the > code that watches for it and throws in openTabPrompt()). Ah. So, talking with mrbkap and jst, they think that a closed prompt should still throw. So I've left this as-is. (In reply to comment #141) > I just remembered something else - nsXULWindow::ShowModal [...] Yeah, the one TODO left is to look at the crufy code in nsWindowWatcher::OpenWindowJSInternal (which calls this). I don't believe any of this stuff matters, but I'll distill it down and check with bkap/jst. Beyond that, the only remaining work here is to add some more tests, and fix any existing tests that might not be expecting a tab modal dialog. (In reply to comment #142) > Justin, would you check my previous comment (comment 129)? Sorry, missed it. I don't think we want to do that, since the plan is to make these prompts less like OS dialogs, and not even immediately pop up when triggered in a background tab (unlike the current dialogs). Bug 598824 would be the place to discuss this if you disagree.

(In reply to comment #143) > Ah. So, talking with mrbkap and jst, they think that a closed prompt should > still throw. Why?

(In reply to comment #143) > (In reply to comment #142) > > Justin, would you check my previous comment (comment 129)? > > Sorry, missed it. I don't think we want to do that, since the plan is to make > these prompts less like OS dialogs, and not even immediately pop up when > triggered in a background tab (unlike the current dialogs). Bug 598824 would be > the place to discuss this if you disagree. Hmm, okay. But note that if the window (the API called window) isn't active in OS level, it does nothing.

(In reply to comment #144) > (In reply to comment #143) > > Ah. So, talking with mrbkap and jst, they think that a closed prompt should > > still throw. > > Why? Basically, AIUI, since it's abnormal condition. JST mentioned that it probably should even be an uncatchable exception (ala OOM?), but code currently can't do that unless it's in the JS engine. Ceasing execution of the page JS (such that what the prompt returns becomes moot) was also an idea, but not sure if that's possible at all.

(In reply to comment #146) > Basically, AIUI, since it's abnormal condition. I don't see how it's an abnormal condition, though. Why should it be treated differently than clicking the window X on our current modal dialogs, which we already treat as "cancel"? Why would anyone ever want to tell these cases apart? Why should alert() now sometimes throw, when it couldn't ever really before?

I can see how it doesn't matter much in practice if the alert()/prompt() call in question is on the window being closed (though it probably reports an exception unnecessarily?), but if it doesn't matter then doing the simplest thing seems best. It seems like it could matter if you're doing win.open().alert();.

What's different between what happens on trunk today and with this patch wrt the caller of modal dialog functions in JS is that today the window can't be closed while the dialog is open, and with this patch the window and/or tab can be closed. In that case, I'd rather stop the script than having it continue to run in a closed window, which could lead to more modal dialogs, and who knows what else. That's why I would ideally like to see an uncatchable exception in this case, but until then, a catchable one would be better than nothing.

Updated Part 2 patch, no changes just resolving conflicts from bug 611566.

(In reply to comment #141) > I just remembered something else - nsXULWindow::ShowModal pushes a null entry > onto the JS context stack while the event loop spins FTR: Talked with mrbkap today, confirmed this is actually redundant code... nsXPConnect::OnProcessNextEvent() also pushes a null context, so we'll still get it when spinning the event loop outselves.

(In reply to comment #149) > I'd rather stop the script than having it continue to run in a closed window Ah, I see. Throwing a catchable exception doesn't actually guarantee that it stops though, and it causes us to report exceptions for what will be common actions, like closing a tab with a prompt open. I guess we should probably deal with that in a followup by coming up with a way to throw an uncatchable/unreported exception from JS...

Comment on attachment 490029 [details] [diff] [review] Part 1 (main patch), v.5 >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml >+ <method name="getTabModalPromptBox"> >+ let promptBox = { >+ appendPrompt : function(args, onCloseCallback) { >+ let tab = self._getTabForContentWindow(browser.contentDocument.defaultView); browser.contentWindow >diff --git a/toolkit/components/prompts/content/tabprompts.xml b/toolkit/components/prompts/content/tabprompts.xml >+ <binding id="tabmodalprompt"> >+ <description class="info.title" hidden="true" style="padding-bottom: 1em"/> Padding should probably be in the theme? Though actually this element seems to be unused, since nothing ever unhides it... so we never show dialog titles. I guess it should just be removed then, and can be re-added if we do want to support showing titles? >+ <hbox class="buttonContainer"> >+#ifdef XP_WIN Flip this around and keep it #ifdef XP_UNIX as in dialog.xml to avoid changing OS/2 behavior :) >+ <button class="button0" label="&okButton.label;" default="true"/> >+ <button class="button0" label="&okButton.label;" default="true"/> I think you should omit the default="true" here, otherwise you can end up with two buttons set as default when CommonDialog.jsm sets the attribute on a non-button0 button (without going through the _setDefaultButton logic in dialog.xml). >+ <constructor> >+ function getElement(class) { >+ return document.getAnonymousElementByAttribute(binding, "class", class); These should probably use anonid instead of class (I'm assuming most don't actually need a class for styling, but even if they do, they can have both specified). >+ this.ui.button0.addEventListener("command", function() { binding.onButtonClick(0); }, false); You could use this.ui.button0.addEventListener("command", this.onButtonClick.bind(this, 0), false); >+ <method name="init"> >+ linkedTab.addEventListener("TabClose", this, false); >+ window.addEventListener("unload", this, false); Can you use an unload handler on this.args.domWindow instead of these two? Would avoid the need for linkedTab which simplifies things a bit. >+ <method name="onKeyAction"> >+ if (action == "default") { >+ let button = 0; >+ if ("defaultButtonNum" in this.args) >+ button = this.args.defaultButtonNum; >+ if (!button.hasAttribute("default")) >+ return; This looks broken - defaultButtonNum.hasAttribute() throws (and indeed enter-to-accept seems broken). It looks like you can just remove it. >+#ifdef XP_MACOSX >+ <handler event="keypress" key="." modifiers="meta" phase="capturing" >+ action="this.onKeyAction('cancel', event);"/> I can't get this handler to do anything, not sure why. >+#else >+ <handler event="focus" phase="capturing"> >+ // Focus shift clears the default button. >+ let bnum = 0; >+ if ("defaultButtonNum" in this.args) >+ bnum = this.args.defaultButtonNum; How about: let defaultButtonNum = this.args.defaultButtonNum || 0; >diff --git a/toolkit/components/prompts/src/CommonDialog.jsm b/toolkit/components/prompts/src/CommonDialog.jsm > if (this.args.defaultButtonNum) > b = this.args.defaultButtonNum; > let button = this.ui["button" + b]; > if (xulDialog) { > xulDialog.defaultButton = ['accept', 'cancel', 'extra1', 'extra2'][b]; > let isOSX = ("nsILocalFileMac" in Components.interfaces); > if (!isOSX) > button.focus(); >+ } else { >+ button.setAttribute("default", "true"); >+ // XXX if this opened in a background tab, do we still want to focus? >+ button.focus(); > } It might be nice in the future if tabprompts.xml shared more of the dialog API - for example, it could have the same defaultButton setter, and this code could always just take the first branch. Worth a followup bug, perhaps? > if (this.args.enableDelay) { >+ if (!xulDialog) >+ throw "BUTTON_DELAY_ENABLE not yet supported for tab-modal prompts"; I think this this should be in tabprompts.xml::init(), since that's the code that actually doesn't support the button delay. Someone doing their own prompt impl. could theoretically use this module and also support BUTTON_DELAY_ENABLE... >+ let topic = "common-dialog-loaded"; >+ if (!xulDialog) >+ topic = "tabmodal-dialog-loaded"; ...and so I wonder whether this shouldn't be something more generic - "dialog-loaded"? That might be too generic. Or maybe we shouldn't fire anything at all in the tabmodal case for now, and we can revisit when a need arises? >diff --git a/toolkit/components/prompts/src/nsPrompter.js b/toolkit/components/prompts/src/nsPrompter.js >+ getTabModalPrompt : function (domWin) { >+ // Given a content DOM window, returns the chrome window it's in. >+ function getChromeWindow(aWindow) { This will actually fail if aWindow *is* a chrome window (chromeEventHandler is null for chrome windows). We don't care about supporting tabmodal prompts parented to chrome windows, but perhaps that should be more explicit (by handling a null chromeEventHandler and returning null), rather than having it throw and be caught in the try/catch below. This applies equally to the same code in nsLoginManagerPrompter.js, so maybe just file a followup? >+ try { >+ // Get topmost window, in case we're in a frame. >+ var promptWin = domWin.top The fact that getChromeWindow() uses chromeEventHandler makes this unnecessary (also missing semi-colon). >+ // Get the chrome window for the content window we're using. >+ // .wrappedJSObject needed here -- see bug 422974 comment 5. Hmm, I kind of wish we'd stop copy/pasting this comment, since it's not a very useful reference (the comment in question applied to an old iteration of the patch on that bug that never landed). "Need to unwrap since we're getting a non-IDL-defined property off the window" is a better comment, if it's still true (this is chrome->chrome so we could conceivably get an unwrapped object here some day). >+function openTabPrompt(domWin, tabPrompt, args) { >+ let canceled = PromptUtils.fireEvent(domWin, "DOMWillOpenModalDialog"); >+ if (canceled) >+ return; The nsAutoWindowStateHelper code doesn't actually let you cancel the dialog opening by canceling the event, so this probably shouldn't either (particularly if we want to fix bug 611553 eventually). (It *does* seem to let listeners control whether they receive DOMModalDialogClosed, which is rather useless/broken...) But actually I think we should just not fire these for tabmodal prompts, since their only purpose was to focus the tab opening the dialog, and that doesn't really serve a useful purpose for tabmodal prompts. >diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css >+/*********** filefield ************/ copy/pasto >diff --git a/toolkit/themes/pinstripe/global/tabprompts.css b/toolkit/themes/pinstripe/global/tabprompts.css >+/* Tab Modal Prompt boxes */ >+tabmodalprompt { >+ width: 100%; >+ height: 100%; Do these actually have any effect? r=me with all of these addressed.

This is a small update to v.5, no review comments addressed yet. * Adds a promptBox.listPrompts() method for tests * Twiddles onPromptClose() and .promptActive -- my text fixes in bug 613403 exposed the interesting case of closing the prompt via the tabmodal-dialog-loaded notification... We were initializing .promptActive to true after the test closed the prompt, and onPromptClose() couldn't close the prompt because tabPrompt.appendPrompt() hadn't returned |newPrompt| yet! * Fixed two little typos exposed by the tests in bug 613444.

(In reply to comment #153) > >+ <description class="info.title" hidden="true" style="padding-bottom: 1em"/> > > Padding should probably be in the theme? Though actually this element seems to > be unused, since nothing ever unhides it... so we never show dialog titles. I > guess it should just be removed then, and can be re-added if we do want to > support showing titles? Removed the padding, I was thinking this could replace the <spacer/> that's in the sameish spot in commonDialog.xul. But I think that actually looks odd in commonDialog... Will follow a followup about tweaking these styles, already had some ideas. I want to leave the always-hidden element in, though, so it's in the DOM as in commonDialog.xul (ie, I don't want to have to bother null checking it). > You could use this.ui.button0.addEventListener("command", > this.onButtonClick.bind(this, 0), false); Yes! \o/ > >+ <method name="init"> > > >+ linkedTab.addEventListener("TabClose", this, false); > >+ window.addEventListener("unload", this, false); > > Can you use an unload handler on this.args.domWindow instead of these two? > Would avoid the need for linkedTab which simplifies things a bit. Doesn't seem to work; when closing a tab with a prompt I get an error about "component has no method named handleEvent", and although the tab goes away the extra event loop remains. So, leaving asis. > >+ <method name="onKeyAction"> ... > This looks broken - defaultButtonNum.hasAttribute() throws (and indeed > enter-to-accept seems broken). It looks like you can just remove it. Oops. Must have broken this as some point. :/ I fixed it to work -- iirc the intention from dialog.xml was that the default keyboard action should only trigger the button when initial default button == current default button (default can shift on focus). > >+#ifdef XP_MACOSX > >+ <handler event="keypress" key="." modifiers="meta" phase="capturing" > >+ action="this.onKeyAction('cancel', event);"/> > > I can't get this handler to do anything, not sure why. Hmm. Doesn't seem to work with normal modal prompts anyway -- nor does it do anything in Safari. Just going to remove this. > It might be nice in the future if tabprompts.xml shared more of the dialog API > - for example, it could have the same defaultButton setter, and this code could > always just take the first branch. Worth a followup bug, perhaps? Eh, maybe, I don't feel strongly either way. I'm actually finding (at least in tests, so far) that using Dialog.ui.* is a very convenient way to get at things. > >+ let topic = "common-dialog-loaded"; > >+ if (!xulDialog) > >+ topic = "tabmodal-dialog-loaded"; > > ...and so I wonder whether this shouldn't be something more generic - > "dialog-loaded"? That might be too generic. Or maybe we shouldn't fire anything > at all in the tabmodal case for now, and we can revisit when a need arises? No, definitely want to fire something here -- it's the primary way for tests (and perhaps addons) to know when a new dialog is up and running, so they can twiddle/dismiss it. I thought about making it a single notification, but wanted to avoid breaking current code (at least, common-dialog-loaded code that assumed the subject was a window). Adding 2 observers, as I've done in some tests, doesn't seem too bad. > >+ getTabModalPrompt : function (domWin) { > > >+ // Given a content DOM window, returns the chrome window it's in. > >+ function getChromeWindow(aWindow) { > > This will actually fail if aWindow *is* a chrome window (chromeEventHandler is > null for chrome windows). [...] This applies equally to the same > code in nsLoginManagerPrompter.js, so maybe just file a followup? Filed bug 613695. > >+function openTabPrompt(domWin, tabPrompt, args) { > >+ let canceled = PromptUtils.fireEvent(domWin, "DOMWillOpenModalDialog"); > >+ if (canceled) > >+ return; > > The nsAutoWindowStateHelper code doesn't actually let you cancel the dialog > opening by canceling the event I think it does; the last thing nsWindowWatcher::OpenWindowJSInternal() does is check windowStateHelper.DefaultEnabled(), and doesn't open the window if it was canceled. But anyway, I agree we should just remove this for tab-modal prompts. I think I might have ended up using it for bug 598824, but for new code the notification would be better anyway. > /toolkit/themes/pinstripe/global/tabprompts.css > > >+/* Tab Modal Prompt boxes */ > >+tabmodalprompt { > >+ width: 100%; > >+ height: 100%; > > Do these actually have any effect? Not sure, but bug 598786 replaces them (this was basically placeholder style in the original patch). I'm just going to leave this, since it's harmless and avoids a bit of work to update the bug 598786 patch.

(In reply to comment #153) > >+ try { > >+ // Get topmost window, in case we're in a frame. > >+ var promptWin = domWin.top > > The fact that getChromeWindow() uses chromeEventHandler makes this unnecessary Oops, it might not be needed for getChromeWindow, we end up calling getBrowserForDocument(), and that needs to know the topmost doc.

Updated with review comments.

Pushed to mozilla-central: Part 1: http://hg.mozilla.org/mozilla-central/rev/b48b84055237 Part 2: http://hg.mozilla.org/mozilla-central/rev/1fc05b5edd02 Note that bug 598824 doesn't have a patch yet, so prompts triggered in background tabs won't do anything (behaviorally or visually) until you switch to that tab. We'll get that fixed ASAP. Anyway -- Yay! Big thanks to the 165 users CC'd on this bug: Old bugs with lots of CC's are infamous for being hard to work in, because there can be lots of noise and off-topic debate. You've all been great -- very much appreciated! I <3 our community. And sorry for all the bugmail you've gotten over the past few weeks as this has neared completion. ;-) Finally, just to answer a few likely-common questions: 1) This has landed in mozilla-central. If all goes well, it will first appear in tomorrow's nightly test build of Firefox 4 (http://nightly.mozilla.org). Firefox Beta 8 will be the first official release with this fix. This will not be backported to Firefox 3.6. 2) The only prompts this affects are the window.alert() / .confirm() / .prompt() calls triggered by pages. Things like HTTP Authentication, onbeforeunload messages, quit dialogs, etc are unchanged (ie, window-modal prompts). We'll look at fixing those later, probably after Firefox 4 to minimize risk. 3) This bug is closed now. If you find problems with the implementation, or have suggestions for more improvements, please file a NEW BUG, and mark it as blocking this one. CC me if you wish.

(In reply to comment #155) > I fixed it to work -- iirc the intention from dialog.xml was that the default > keyboard action should only trigger the button when initial default button == > current default button (default can shift on focus). Dialog doesn't have any such special case, AFAICT. Enter always triggers the default action. > > I can't get this handler to do anything, not sure why. > > Hmm. Doesn't seem to work with normal modal prompts anyway -- nor does it do > anything in Safari. Just going to remove this. It worked fine for me in normal modal prompts...

(In reply to comment #158) > [...] tl;dr: Fixed for Firefox 4. Not Firefox 3.6. File a NEW BUG for any issues or concerns. Did I mention filing a NEW BUG? Great, thanks! :) (file a new bug)

It fells much less responsive than in ff3.6. On my machine P4 2.8GHz it take approximately 1-2sec to show up modal dialog. Whereas in 3.6 is almost instantaneously. Any plans to improve this? Otherwise great work, I like it very much.

(In reply to comment #161) > It fells much less responsive than in ff3.6. > On my machine P4 2.8GHz it take approximately 1-2sec to show up modal dialog. Please file a bug. I suspect the blur effect is to blame.

http://img257.imageshack.us/img257/8378/acid3test.png looks ugly

(In reply to comment #163) > http://img257.imageshack.us/img257/8378/acid3test.png looks ugly File a new bug, as the comments above yours instruct. (Check if one has already been filed for this first.)

Just got hit by this "solution", testing the nightly build (2010-11-21). How are we to determine that a tab needs our attention this now that .alerts() and .confirms() no longer pop up over our current page? Usage Scenario: Control system, accessed via Web Page, using Java Script to pop up critical notifications. As far as I'm concerned, without some sort of notification system this bug is not Resolved Fixed.

Ack, sorry - missed the note regarding: https://bugzilla.mozilla.org/show_bug.cgi?id=598824. Will follow there.

I've noticed something, but I'm not sure if it is a bug: When a tab-modal alert is displayed, we can still interact with the content of the page by clicking on the url bar (for example), and the tabbing to a selectable item in the page. Is it intentional behaviour?

@Yann - bug 613763

I've created Bug 613937 , but I'm think Matthias don't understood what I meant.

There are two bugs compared with previous (window-modal) implementation (as for Windows version of Firefox 4): 1. only five lines are visible at once (unlimited in previous implementation). It makes sense to display more lines without need to scroll. Probably it makes sense to display all lines that fits into document client height (or only a bit fewer): for example, alert window would have height equal to 90% of client height or so); 2. alert text is not copyable (previous implementation has text selectable and therefore available to copy which is very useful and usable). Are these need to be filed as a separate bugs?

There are already filled bugs regarding these issues

You want to look at bug 613749 (1) and bug 613742 (2).

(In reply to comment #170) > Are these need to be filed as a separate bugs? I probably should have mentioned this back in comment 158 or comment 160... If you have comments, issues, problems, etc, file a NEW BUG. That's right! FILE A NEW BUG. Thanks. (file a new bug) [1] You too, preed. new. bug. ;)

(In reply to comment #173) Maybe it makes sense to introduce into Bugzilla some kind of "sticky" messages that would be easily noticeable without need to read hundreds of comments. Anyway, appropriate bugs 613749 and 613742 already exist as stated in comment 172. Thanks for your attention.

>Maybe it makes sense to introduce into Bugzilla some kind of "sticky" messages >that would be easily noticeable without need to read hundreds of comments. that sounds like a new bug :)

(In reply to comment #174) > Maybe it makes sense to introduce into Bugzilla some kind of "sticky" messages > that would be easily noticeable without need to read hundreds of comments. That's what the status whiteboard is for. I have set it.

(In reply to comment #175) > >Maybe it makes sense to introduce into Bugzilla some kind of "sticky" messages > >that would be easily noticeable without need to read hundreds of comments. > > that sounds like a new bug :) Actually, that sounds like a very old one -> bug 283695

At that point I want to very much thank Justin Dolske for his work! I very much appreciate this, window-modal dialogs for alert()s always were a great source of annoyance. Thanks!

I've created performance related bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=614810

window.showModalDialog() should be tab modal (Bug 616839)

Believed to have caused regression bug 619644.