Last Comment Bug 59314 - JavaScript alerts should be content-modal, not window-modal
: JavaScript alerts should be content-modal, not window-modal
Status: RESOLVED FIXED
[parity-opera] [needs-a11y-qa] **File...
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: P3 enhancement with 142 votes (vote)
: mozilla2.0b8
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
http://stuwww.kub.nl/~sanderb/deny.html
: 117561 123007 123913 138585 222125 237317 291474 297404 355904 361292 399822 408469 417894 418814 429906 431032 436671 436750 439416 450873 455466 456509 471940 520019 520020 536741 561138 562258 571683 576932 583718 608629 (view as bug list)
Depends on: 398103 613751 613754 613918 615026 615931 619134 621308 622949 622953 626442 626963 637264 642567 644322 647086 652226 652256 655308 670666 697446 727801 1061258 564337 575485 596371 598470 598786 598824 611166 611566 613403 613444 613710 613714 613734 613742 613748 613749 613752 613753 613757 613760 613763 613768 613800 613842 614094 614379 614738 615186 616832 617507 618856 619644 619645 619646 619765 620145 620195 620209 620213 620300 620614 620615 621672 622326 622331 622340 622495 622939 623481 625187 637160 638352 647727 648924 671820 762830 977957 978777
Blocks: eviltraps 551861 cuts-focus 613785 625056 655181 663515 useragent 123908 140346 327310 332195 402401 569993 603901 631485
  Show dependency treegraph
 
Reported: 2000-11-06 17:11 PST by Cesar Eduardo Barros
Modified: 2016-07-17 05:52 PDT (History)
164 users (show)
jst: blocking1.9.2-
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Add a menu item to the context menu in alert dialogs to stop the script that caused it. (3.94 KB, patch)
2009-09-13 22:16 PDT, Trent Waddington
no flags Details | Diff | Splinter Review
alert box showing legend to be displayed after 5 seconds (21.97 KB, image/png)
2010-01-17 17:36 PST, Jonathan
no flags Details
Patch v.1 (WIP) (21.18 KB, patch)
2010-09-14 14:07 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (23.51 KB, patch)
2010-09-27 17:56 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Part 1 (main patch), v.3 (28.79 KB, patch)
2010-10-13 19:39 PDT, Justin Dolske [:Dolske]
gavin.sharp: review-
Details | Diff | Splinter Review
Part 2 (opt-in tabmodal), v.1 (9.04 KB, patch)
2010-10-13 19:46 PDT, Justin Dolske [:Dolske]
jst: review+
gavin.sharp: feedback+
Details | Diff | Splinter Review
Part 1 (main patch), v.4 (27.48 KB, patch)
2010-11-03 18:37 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Part 1 (main patch), v.5 (31.82 KB, patch)
2010-11-11 20:44 PST, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Part 2 (opt-in tabmodal), v.2 (9.13 KB, patch)
2010-11-16 22:54 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Part 1 (main patch), v.6 (32.25 KB, patch)
2010-11-19 14:34 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.7 (31.40 KB, patch)
2010-11-19 20:10 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review

Description Cesar Eduardo Barros 2000-11-06 17:11:04 PST
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.
Comment 1 John Unruh 2000-11-07 08:10:32 PST
Confirmed.
Comment 2 Mitchell Stoltz (not reading bugmail) 2000-11-07 09:12:01 PST
DoS attacks are low on my priority list right now. Marking Future.
Comment 3 Mitchell Stoltz (not reading bugmail) 2000-11-13 18:35:00 PST


*** This bug has been marked as a duplicate of 29346 ***
Comment 4 Jesse Ruderman 2000-11-13 19:40:38 PST
Actually a dup of some part of bug 22049 (which currently needs to be split 
into several bugs).
Comment 5 John Unruh 2000-11-14 09:06:59 PST
Verified dupe.
Comment 6 Jesse Ruderman 2001-01-13 23:17:35 PST
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.
Comment 7 Matthew Paul Thomas 2001-01-14 07:15:11 PST
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().
Comment 8 Jesse Ruderman 2001-01-14 15:44:11 PST
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?
Comment 9 Matthew Paul Thomas 2001-01-15 17:08:00 PST
The button would produce an additional alert asking if you wanted to stop all 
scripts in the current page.
Comment 10 John Unruh 2001-01-16 08:56:11 PST
Qa > CKRITZER
Comment 11 Boris Zbarsky [:bz] 2002-01-01 00:32:52 PST
*** Bug 117561 has been marked as a duplicate of this bug. ***
Comment 12 Jesse Ruderman 2002-02-01 16:30:57 PST
*** Bug 123007 has been marked as a duplicate of this bug. ***
Comment 13 Boris Zbarsky [:bz] 2002-02-06 15:23:53 PST
*** Bug 123913 has been marked as a duplicate of this bug. ***
Comment 14 m_mozilla 2002-02-06 15:46:44 PST
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
Comment 15 John Levon 2002-02-15 19:05:52 PST
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 ?
Comment 16 John Levon 2002-02-15 19:12:05 PST
Aaagh, ignore me I missed bug 123913, and I duped it to that.
Comment 17 Andrew Hagen 2002-02-15 19:15:14 PST
Proposing to fix this bug for Mozilla 1.01. 
Comment 18 Mitchell Stoltz (not reading bugmail) 2002-03-07 20:10:36 PST
ANdrew, are you offering to fix this? If so, assign it to yourself.
Comment 19 Arthur 2002-04-09 01:00:59 PDT
*** Bug 136324 has been marked as a duplicate of this bug. ***
Comment 20 John Levon 2002-04-19 21:28:29 PDT
*** Bug 138585 has been marked as a duplicate of this bug. ***
Comment 21 petertw 2002-06-11 19:33:09 PDT
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
Comment 22 rgpublic 2002-07-30 00:11:51 PDT
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.
Comment 23 Matthew Paul Thomas 2002-08-06 07:27:49 PDT
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.
Comment 24 Jonas Jørgensen 2002-08-06 08:48:40 PDT
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.
Comment 25 Malx 2002-12-05 13:45:39 PST
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 :(
Comment 26 Brian 'netdragon' Bober 2003-02-09 16:10:02 PST
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.
Comment 27 Mitchell Stoltz (not reading bugmail) 2003-06-09 18:00:18 PDT
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.
Comment 28 Emmanuel Bourg 2003-10-14 17:15:18 PDT
*** Bug 222125 has been marked as a duplicate of this bug. ***
Comment 29 David Perry 2004-08-30 11:26:04 PDT
(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.
Comment 30 Jesse Ruderman 2004-10-01 14:31:42 PDT
*** Bug 237317 has been marked as a duplicate of this bug. ***
Comment 31 Joona Palaste 2005-01-15 14:36:33 PST
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.
Comment 32 Daniel Veditz [:dveditz] 2005-04-22 10:13:45 PDT
*** Bug 291474 has been marked as a duplicate of this bug. ***
Comment 33 Alex 2005-05-16 05:21:55 PDT
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)
Comment 34 Matthias Versen [:Matti] 2005-06-11 05:29:04 PDT
*** Bug 297404 has been marked as a duplicate of this bug. ***
Comment 35 Nicos Gollan 2006-05-04 14:27:39 PDT
The Web Developer Toolbar has a "Disable JavaScript" functionality. Would it be that hard to duplicate that for a modal dialog's originating context?
Comment 36 Ryan Flint [:rflint] (ping via IRC for reviews) 2006-10-07 23:01:07 PDT
*** Bug 355904 has been marked as a duplicate of this bug. ***
Comment 37 Jesse Ruderman 2006-11-21 13:31:41 PST
*** Bug 361292 has been marked as a duplicate of this bug. ***
Comment 38 Magnus Melin 2007-01-29 09:45:41 PST
*** Bug 368543 has been marked as a duplicate of this bug. ***
Comment 39 Jason Keirstead 2007-09-13 05:33:48 PDT
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.
Comment 40 Nicos Gollan 2007-10-08 05:44:22 PDT
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.
Comment 41 Bob Clary [:bc:] 2007-10-14 20:05:20 PDT
*** Bug 399822 has been marked as a duplicate of this bug. ***
Comment 42 Joseph Kopena 2007-10-23 06:18:13 PDT
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
Comment 43 Gábor Stefanik 2007-11-23 08:23:54 PST
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.
Comment 44 Gábor Stefanik 2007-11-23 08:25:40 PST
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).
Comment 45 Jason Keirstead 2007-11-23 08:32:22 PST
(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. 
Comment 46 Gábor Stefanik 2007-11-23 10:06:07 PST
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.)
Comment 47 Dave Townsend [:mossop] 2007-12-15 04:31:34 PST
*** Bug 408469 has been marked as a duplicate of this bug. ***
Comment 48 Tim McCormack 2008-02-15 09:04:06 PST
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.
Comment 49 Jesse Ruderman 2008-02-15 16:09:21 PST
fwiw, there are no more non-resizable content windows.  See bug 177838.
Comment 50 Jesse Ruderman 2008-02-16 14:45:16 PST
*** Bug 417894 has been marked as a duplicate of this bug. ***
Comment 51 Gábor Stefanik 2008-02-16 15:32:43 PST
(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).
Comment 52 Tim McCormack 2008-02-16 16:06:15 PST
(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.
Comment 53 Gábor Stefanik 2008-02-17 12:24:26 PST
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.
Comment 54 :Aryeh Gregor (working until September 2) 2008-02-17 12:32:36 PST
(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.
Comment 55 Gábor Stefanik 2008-02-18 06:29:36 PST
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).
Comment 56 Nickolay_Ponomarev 2008-02-21 12:08:34 PST
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.
Comment 57 Elmar Ludwig 2008-02-21 12:32:09 PST
*** Bug 418814 has been marked as a duplicate of this bug. ***
Comment 58 Gábor Stefanik 2008-02-23 16:10:57 PST
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.
Comment 59 Matthias Versen [:Matti] 2008-04-20 06:45:47 PDT
*** Bug 429906 has been marked as a duplicate of this bug. ***
Comment 60 Dave Townsend [:mossop] 2008-04-27 05:01:49 PDT
*** Bug 431032 has been marked as a duplicate of this bug. ***
Comment 61 Stephen 2008-06-01 02:49:35 PDT
*** Bug 436750 has been marked as a duplicate of this bug. ***
Comment 62 Jesse Ruderman 2008-06-01 19:20:26 PDT
*** Bug 436671 has been marked as a duplicate of this bug. ***
Comment 63 Derrick 2008-06-15 19:39:11 PDT
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/

Comment 64 Jo Hermans 2008-06-16 08:45:34 PDT
*** Bug 439416 has been marked as a duplicate of this bug. ***
Comment 65 Pasi Keinänen 2008-08-04 03:43:29 PDT
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).

Comment 66 John 2008-08-04 16:54:41 PDT
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.
Comment 67 [:Aleksej] 2008-09-16 07:45:22 PDT
*** Bug 455466 has been marked as a duplicate of this bug. ***
Comment 68 Jesse Ruderman 2008-09-23 00:45:17 PDT
*** Bug 456509 has been marked as a duplicate of this bug. ***
Comment 69 bugzilla.mozilla.org 2008-10-30 03:24:03 PDT
another example for malware exploiting this bug: http://scan.scannerantispyware.com/338/3/
Comment 70 Steven Sulley 2008-12-04 09:40:43 PST
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.
Comment 71 Mardeg 2009-01-02 19:52:13 PST
*** Bug 471940 has been marked as a duplicate of this bug. ***
Comment 72 Ralph 2009-02-17 15:32:21 PST
[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.
Comment 73 Jens Gyldenkærne Clausen 2009-04-04 17:10:34 PDT
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.
Comment 74 James May [:fowl] 2009-04-04 17:32:31 PDT
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'.
Comment 75 Nicos Gollan 2009-04-05 05:17:20 PDT
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!
Comment 76 James May [:fowl] 2009-04-05 06:03:34 PDT
By all means create a spin-off bug for the stopgap.
Comment 77 Nicos Gollan 2009-04-08 01:47:47 PDT
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.
Comment 78 Candid Dauth 2009-06-01 11:09:47 PDT
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.
Comment 79 Johnny Stenback (:jst, jst@mozilla.com) 2009-07-22 15:56:50 PDT
Not blocking 1.9.2 on this.
Comment 80 Ralph 2009-07-22 18:05:12 PDT
(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.
Comment 81 Neil Harris 2009-07-28 09:37:47 PDT
(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.
Comment 82 Trent Waddington 2009-09-13 18:04:01 PDT
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.
Comment 83 Trent Waddington 2009-09-13 22:16:57 PDT
Created attachment 400436 [details] [diff] [review]
Add a menu item to the context menu in alert dialogs to stop the script that caused it.

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 84 Nickolay_Ponomarev 2009-09-14 14:50:45 PDT
*** Bug 450873 has been marked as a duplicate of this bug. ***
Comment 85 Trent Waddington 2009-09-15 15:06:15 PDT
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.
Comment 86 Jesse Ruderman 2009-09-15 20:09:43 PDT
That patch belongs on bug 61098, not here.
Comment 87 Jesse Ruderman 2009-09-15 20:14:43 PDT
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 88 Igor Bukanov 2009-09-16 03:38:24 PDT
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.
Comment 89 Alex Vincent [:WeirdAl] 2009-09-16 12:05:04 PDT
+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.
Comment 90 [:Aleksej] 2009-11-25 02:31:54 PST
*** Bug 520020 has been marked as a duplicate of this bug. ***
Comment 91 Adam Alton 2009-12-09 03:45:00 PST
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>
Comment 92 Felix Lee 2009-12-09 11:52:14 PST
(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.
Comment 93 Mardeg 2009-12-25 06:47:19 PST
*** Bug 536741 has been marked as a duplicate of this bug. ***
Comment 94 Jonathan 2010-01-17 06:45:00 PST
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.
Comment 95 Jakub 'Livio' Rusinek 2010-01-17 12:32:38 PST
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.
Comment 96 Shad Sterling 2010-01-17 14:32:53 PST
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.)
Comment 97 Jonathan 2010-01-17 17:34:22 PST
> 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.)
Comment 98 Jonathan 2010-01-17 17:36:02 PST
Created attachment 422139 [details]
alert box showing legend to be displayed after 5 seconds
Comment 99 Dão Gottwald [:dao] 2010-04-23 05:07:56 PDT
*** Bug 123913 has been marked as a duplicate of this bug. ***
Comment 100 Dão Gottwald [:dao] 2010-04-23 05:08:28 PDT
*** Bug 561138 has been marked as a duplicate of this bug. ***
Comment 101 krishtal 2010-05-14 07:18:31 PDT
пожайлуста, пишите по русски)
Comment 102 Danny 2010-05-20 02:06:22 PDT
krishtal, учи английский или убирайся!
Comment 103 Dmitriy 2010-05-20 03:37:27 PDT
Danny, повежливей уж давай.

На самом деле баг - стыдобища: открыт в 2000-м году, критикал, позволяет заблокировать работу броузера и до сих пор не пофиксен.

Мне периодически сыпятся на мейл уведомления о том, что в этом треде что-то написали, на кого-то баг переассигновали. К сожалению, дальше балабольства дело не трогается.

Посему имхо без разницы на каком языке будет идти беседа - хоть на китайском: результата-то всё равно нет.
Comment 104 Michael Lefevre 2010-05-20 03:58:32 PDT
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
Comment 105 Daniel Cater 2010-05-25 17:28:16 PDT
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.
Comment 106 Cork 2010-06-12 08:04:35 PDT
*** Bug 571683 has been marked as a duplicate of this bug. ***
Comment 107 Brett Zamir 2010-06-22 15:42:47 PDT
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.
Comment 108 Mardeg 2010-07-04 23:53:49 PDT
*** Bug 576932 has been marked as a duplicate of this bug. ***
Comment 109 Jo Hermans 2010-08-02 06:01:40 PDT
*** Bug 583718 has been marked as a duplicate of this bug. ***
Comment 110 Justin Dolske [:Dolske] 2010-09-14 13:17:39 PDT
*** Bug 562258 has been marked as a duplicate of this bug. ***
Comment 111 Justin Dolske [:Dolske] 2010-09-14 13:19:53 PDT
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.]
Comment 112 Justin Dolske [:Dolske] 2010-09-14 14:07:14 PDT
Created attachment 475216 [details] [diff] [review]
Patch v.1 (WIP)

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.
Comment 113 Kamil Sartys 2010-09-14 14:10:46 PDT
(In reply to comment #111)
> Patches coming up.

oh no! i totally had started party planning for its 10th birthday ...
Comment 114 Justin Dolske [:Dolske] 2010-09-22 15:14:58 PDT
Marking blocking betaN per beltzner.
Comment 115 Justin Dolske [:Dolske] 2010-09-27 17:56:43 PDT
Created attachment 478933 [details] [diff] [review]
Patch v.2
Comment 116 Matt McCutchen 2010-09-27 19:05:26 PDT
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 117 Nochum Sossonko [:Natch] 2010-09-28 10:29:33 PDT
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...
Comment 118 Justin Dolske [:Dolske] 2010-09-28 12:58:21 PDT
(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.
Comment 119 Nochum Sossonko [:Natch] 2010-09-28 20:00:53 PDT
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).
Comment 120 Nochum Sossonko [:Natch] 2010-09-28 21:39:12 PDT
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...
Comment 121 Matt McCutchen 2010-09-28 21:47:28 PDT
(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.
Comment 122 Nochum Sossonko [:Natch] 2010-09-29 00:05:42 PDT
(In reply to comment 120)
s/Ex/Check/g - got confused by the impl.
Comment 123 Shad Sterling 2010-09-29 07:55:25 PDT
(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.
Comment 124 Justin Dolske [:Dolske] 2010-10-13 19:39:37 PDT
Created attachment 483059 [details] [diff] [review]
Part 1 (main patch), v.3

Small update mostly around how getTabModalPromptBox() is implemented/used.
Comment 125 Justin Dolske [:Dolske] 2010-10-13 19:46:39 PDT
Created attachment 483061 [details] [diff] [review]
Part 2 (opt-in tabmodal), v.1

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.
Comment 126 Nochum Sossonko [:Natch] 2010-10-14 11:29:25 PDT
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.
Comment 127 Justin Dolske [:Dolske] 2010-10-14 12:43:49 PDT
(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 128 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-27 18:26:36 PDT
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).
Comment 129 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-10-28 20:26:37 PDT
Would you call window.notifyDefaultButtonLoaded(defaultButton) when the dialog becomes visible? It supports "snap to default button" on Windows. See bug 76053.
Comment 130 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-31 14:59:17 PDT
*** Bug 608629 has been marked as a duplicate of this bug. ***
Comment 131 Eric Kolotyluk 2010-10-31 15:06:40 PDT
OK, so this problem has been around for 10 years now and no-one is going to fix it?
Comment 132 Dave Garrett 2010-10-31 15:48:27 PDT
(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
Comment 133 Eric Kolotyluk 2010-10-31 16:25:40 PDT
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 134 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-01 12:27:32 PDT
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 135 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-02 15:45:42 PDT
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 :(
Comment 136 Justin Dolske [:Dolske] 2010-11-03 18:20:19 PDT
(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.
Comment 137 Justin Dolske [:Dolske] 2010-11-03 18:37:21 PDT
Created attachment 488112 [details] [diff] [review]
Part 1 (main patch), v.4

Review comments addressed, except for issue noted in comment 134. Checkpointing changes so far...
Comment 138 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-04 02:01:19 PDT
(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).
Comment 139 Please Ignore This Troll (Account Disabled) 2010-11-07 05:11:14 PST
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?
Comment 140 Dave Townsend [:mossop] 2010-11-07 08:34:44 PST
(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.
Comment 141 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-08 03:50:29 PST
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++...
Comment 142 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-11-10 17:51:40 PST
Justin, would you check my previous comment (comment 129)?
Comment 143 Justin Dolske [:Dolske] 2010-11-11 20:44:11 PST
Created attachment 490029 [details] [diff] [review]
Part 1 (main patch), v.5

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.
Comment 144 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-11 21:47:21 PST
(In reply to comment #143)
> Ah. So, talking with mrbkap and jst, they think that a closed prompt should
> still throw.

Why?
Comment 145 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-11-11 23:05:58 PST
(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.
Comment 146 Justin Dolske [:Dolske] 2010-11-12 15:25:10 PST
(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.
Comment 147 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-15 14:10:40 PST
(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?
Comment 148 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-15 14:13:44 PST
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();.
Comment 149 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-16 15:48:34 PST
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.
Comment 150 Justin Dolske [:Dolske] 2010-11-16 22:54:16 PST
Created attachment 491107 [details] [diff] [review]
Part 2 (opt-in tabmodal), v.2

Updated Part 2 patch, no changes just resolving conflicts from bug 611566.
Comment 151 Justin Dolske [:Dolske] 2010-11-17 19:04:40 PST
(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.
Comment 152 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-18 19:39:58 PST
(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 153 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-18 19:40:50 PST
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.
Comment 154 Justin Dolske [:Dolske] 2010-11-19 14:34:17 PST
Created attachment 491955 [details] [diff] [review]
Part 1 (main patch), v.6

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.
Comment 155 Justin Dolske [:Dolske] 2010-11-19 19:39:35 PST
(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.
Comment 156 Justin Dolske [:Dolske] 2010-11-19 19:57:20 PST
(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.
Comment 157 Justin Dolske [:Dolske] 2010-11-19 20:10:22 PST
Created attachment 492034 [details] [diff] [review]
Patch v.7

Updated with review comments.
Comment 158 Justin Dolske [:Dolske] 2010-11-19 21:43:43 PST
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.
Comment 159 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-19 21:45:44 PST
(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...
Comment 160 Justin Dolske [:Dolske] 2010-11-19 21:46:28 PST
(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)
Comment 161 keegkey 2010-11-20 08:50:09 PST
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.
Comment 162 Dão Gottwald [:dao] 2010-11-21 01:39:49 PST
(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.
Comment 164 Tim McCormack 2010-11-21 06:50:41 PST
(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.)
Comment 165 Truth 2010-11-21 19:30:37 PST
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.
Comment 166 Truth 2010-11-21 19:33:27 PST
Ack, sorry - missed the note regarding: https://bugzilla.mozilla.org/show_bug.cgi?id=598824. Will follow there.
Comment 167 Yann Brelière 2010-11-22 03:28:16 PST
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?
Comment 168 gadjo 2010-11-22 03:35:54 PST
@Yann - bug 613763
Comment 169 Oleg 2010-11-22 13:21:53 PST
I've created Bug 613937 , but I'm think Matthias don't understood what I meant.
Comment 170 Marat Tanalin | tanalin.com 2010-11-23 06:42:42 PST
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?
Comment 171 gadjo 2010-11-23 06:51:13 PST
There are already filled bugs regarding these issues
Comment 172 Dirkjan Ochtman (:djc) 2010-11-23 07:11:43 PST
You want to look at bug 613749 (1) and bug 613742 (2).
Comment 173 Justin Dolske [:Dolske] 2010-11-23 15:32:44 PST
(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. ;)
Comment 174 Marat Tanalin | tanalin.com 2010-11-23 16:09:01 PST
(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.
Comment 175 Alex Faaborg [:faaborg] (Firefox UX) 2010-11-23 17:26:43 PST
>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 :)
Comment 176 Matt McCutchen 2010-11-23 17:44:27 PST
(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.
Comment 177 Dave Garrett 2010-11-23 19:31:05 PST
(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
Comment 178 Nikita Popov 2010-11-24 10:51:02 PST
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!
Comment 179 keegkey 2010-11-25 06:53:53 PST
I've created performance related bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=614810
Comment 180 Biju 2010-12-05 08:58:31 PST
window.showModalDialog() should be tab modal (Bug 616839)
Comment 181 Ed Morley [:emorley] 2010-12-16 15:35:16 PST
Believed to have caused regression bug 619644.
Comment 182 Adam Katz 2011-01-19 13:38:46 PST
Adding deps from duplicate bug 616843
Comment 183 Adam Katz 2011-01-19 13:40:06 PST
*** Bug 616843 has been marked as a duplicate of this bug. ***
Comment 184 Frank Yan (:fryn) 2011-01-19 18:05:52 PST
*** Bug 520019 has been marked as a duplicate of this bug. ***
Comment 185 G.S.ALex 2011-01-28 05:47:55 PST
The http access authentication windows are still window-modal.
Comment 186 Matt McCutchen 2011-01-28 06:05:19 PST
As per bug 562258 comment 31, this bug is specific to JavaScript alerts.  Bug 616843 is the tracker for all other dialogs.  I'm reverting the addition of the dependencies from bug 616843.
Comment 187 Eric Shepherd [:sheppy] 2011-02-01 11:54:05 PST
I've added this page talking about tab-modal prompts and how to use them from chrome (the code sample is in progress and will be added within the next hour or so).

In addition, the relevant XUL changes have been documented, I believe. If someone could review this and make sure it's accurate, I would appreciate it:

Updated (to include the getTabModalPromptBox method and tabmodalPromptShowing attribute):

https://developer.mozilla.org/en/XUL/tabbrowser

Updated (to cover the pref to return to the old style alerts):

https://developer.mozilla.org/en/Preferences/Mozilla_preferences_for_uber-geeks

New pages:

https://developer.mozilla.org/en/Using_tab-modal_prompts
https://developer.mozilla.org/en/XUL/Method/getTabModalPromptBox
https://developer.mozilla.org/en/XUL/Attribute/tabmodalPromptShowing
https://developer.mozilla.org/en/XUL/promptBox
Comment 188 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-21 13:29:44 PST
Just noticed this very old in-litmus request. Canceling the request. Please re-nom if a test is still needed.
Comment 189 Marat Tanalin | tanalin.com 2016-06-29 11:42:13 PDT
Thoughts aloud after two latest "changes": it would probably make sense to send notifications with a delay (instead of immediately) after the change, so that people were not disturbed with two consecutive notifications the second of which cancels the first one, so there is effectively NOTHING to notify about.

Note You need to log in before you can comment on or make changes to this bug.