Bug 33962

Created attachment 47227 [details] Testcase using window.alert

Created attachment 47229 [details] Does not fire focus/blur events while a modal dialog is up This patch suppresses firing all focus events while a modal dialog is up (using Page::defersLoading() to detect this case). This is consistent with Safari 4.0.4 behavior, although it is a change compared to ToT WebKit. Here is a summary of what other browsers do when an <input> has a mouseup handler that opens a modal dialog (window.alert/window.confirm) and a blur handler that sets a timer (window.setTimeout): Firefox 3.5.7 executes the 'onblur' handler when the modal dialog appears. The timer is set, but it does not fire until the time has elapsed _and_ the dialog is dismissed. While the dialog is up, the input area does not appear to be focused (there is no blinking carrot or focus ring on OS X). When the dialog is dismissed, the input area is focused and looks focused (blinking carrot, focus ring on OS X). IE8 executes the 'onblur' handler when the modal dialog appears. The timer is set, but it does not start 'counting' until the dialog is dismissed (so on this test page it does not fire until 5 seconds after the dialog is dismissed). While the dialog is up, the input area doesn't visibly appear focused. Safari 4.0.4 on Windows does not execute the 'onblur' handler at all. The input area does not appear visually focused while the modal dialog is up. Once the dialog is dismissed, the input area still does not look visually focused and does not receive text input, but when the user clicks anywhere else on the page the 'blur' event still fires at the <input>. This seems really buggy. Safari 4.0.4 on OS X does not execute the 'onblur' handler at all. The input area does not appear visually focused while the modal dialog is up. Once the dialog is dismissed, the input area does look visually focused and does receive text input (unlike on Windows). WebKit ToT executes the 'onblur' handler while the dialog is up and fires the timer while the dialog is up. It also crashes with an assertion failure in debug mode if the dialog is dismissed while the timer is still pending. I do not think that website authors typically expect their code to execute while another piece of code is blocked on a modal dialog. This patch brings behavior back to roughly the Safari 4.0.4 behavior. Alternatively, we could still fire focus/blur events while modal dialogs are up but ensure that all timers created are created in the suspended state. Then they would be resumed by the PageGroupLoadDeferrer when the dialog is dismissed. This would be closer to the Firefox/IE8 behavior, which is still pretty odd IMHO.

I think we have a blur of death bug on file as well. I remember running into it while writing test cases for wave bugs.

Yes, bug 18315.

Bug 18315 looks like exactly the same issue.

Comment on attachment 47229 [details] Does not fire focus/blur events while a modal dialog is up Makes sense to me. Seems the code should have a comment next to it explaining why the if check is made. And can we test this? If we can't, we should explain why not in the ChagneLog.

I wonder if we can leverage showModalDialog somehow to test this. window.alert() just logs text in the layout tests, but showModalDialog actually functions.

showModalDialog() support in DRT and test_shell seems really bad. It's off by default in test_shell and can be enabled with a command-line flag, but the nested loop for the opened windows does not spin down when window.close() is invoked inside the popup so it's not very useful. I couldn't get the dialog to show at all using DRT - I suspect that this behavior is also behind some flag or setting and may not be fully baked. I'll upload another patch with explanatory comments in the code and ChangeLog that Eric suggested, but I don't think a new test is entirely feasible here, as sad as that is.

Created attachment 47567 [details] patch

Comment on attachment 47567 [details] patch Is it OK to never fire these events? Do we need to fire them after the modal dialog goes back down?

The focused state is the same when the dialog is dismissed as it was when the dialog went up, so it doesn't make sense to fire any events at the page. In particular, with the following snippet: <input id='i'></input> <script> var inElem = document.getElementById('i'); inElem.focus(); window.alert('hi'); </script> Once the alert is dismissed, the input area is still focused.

FYI, html5 dictates the following: window.alert(), window.pause(), and window.prompt() all require the user agent to 'pause' which is defined at http://dev.w3.org/html5/spec/Overview.html#pause as: Some of the algorithms in this specification, for historical reasons, require the user agent to pause while running a task until some condition has been met. While a user agent has a paused task, the corresponding event loop must not run further tasks, and any script in the currently running task must block. User agents should remain responsive to user input while paused, however, albeit in a reduced capacity since the event loop will not be doing anything. window.showModalDialog() has a slightly different behavior in the spec: Disable the user interface for all the browsing contexts in the list of background browsing contexts. This should prevent the user from navigating those browsing contexts, causing events to be sent to those browsing context, or editing any content in those browsing contexts. However, it does not prevent those browsing contexts from receiving events from sources other than the user, from running scripts, from running animations, and so forth. http://dev.w3.org/html5/spec/Overview.html#dom-showmodaldialog Both seem to prohibit firing any events at the page opening the dialog.

It seems the question here is "why only focus events need special treatment?" I was easily able to reproduce the ASSERT in timers by creating a timer from Worker's onmessage callback while showing alert() from the main page. So either we should define what happens when a timer is created while modal UI is up (fire it after modal UI is gone?), or we should 'pause' the page showing modal UI in a more generic way.

I agree in principle, Dmitry. Ideally we would not fire any events at a page behind a modal dialog. However, it makes sense to change this in smaller, isolated changes just in case there's a web compat issue. In particular special casing focus/blur makes sense in the short term for the following reasons: - This case is causing ASSERT failures on a very popular webpage (gmail). I don't know of any popular sites that trigger ASSERT failures on other events (like onmessage). - The blur event always fires when a modal dialog is opened up since the modal dialog will gain focus. This makes it much more likely to trigger bugs than things like onmessage which are racy. - The behavior in this patch matches a released browser (Safari 4.0.4), which reduces the change of introducing new compat bugs. Also this patch is quite tiny and doesn't touch anything as sensitive as the core event handling logic.

> Ideally we would not fire any events at a page behind a modal dialog. Note that Firefox will fire an image onload event behind a modal dialog when the image load finishes. I'm not sure it's a bad thing - the concept of modality is about user input, not about something that happens programmatically.

After some looking around I think the ASSERT in timer code is a red herring here. Firing events on a page that is already in a modal UI means more JS reentrancy. Very few JS developers are prepared to handle reentrancy, that's a good reason to 'freeze' the page behind the modal UI. It seems the good analogy is Cocoa NSApplication's runModalForWindow - it stops all events (including timers) on other windows. One reason why innocently-looking JS can easily shoot itself in the foot in presence of JS reentrancy is bug 18315 which pop ups alerts on top of each other and prevents the page from being closed (in ToT). That bug will be fixed with the proposed patch too. If FF loads images and fires load events behind the modal UI, then they probably have some code in place to deal with an HTTP Auth or "bad cert" dialogs that may pop up while loading images, or perhaps there is some way to 'defer' another alert() that JS code in the event handler may want to pop up. This can be done but it adds complexity and additional code and queues, etc. For example, the network layer of the embedder may continue to pull the bits off the network while events may be deferred (or blocked on the attempt to nest another modal UI, which seems to require more support). This behavior likely needs case-by-case consideration. Since in modal UI case the focus remains on the same element after modal UI is dismissed, I can see a reason not to fire focus events at all in this scenario (no 'blur' and no 'focus'). As James noted, the risk of compat issue is mitigated by the fact that shipping Safari does not fire those. It might be deemed necessary to prevent/defer other sources of events (worker.onmessage for example) from being dispatched while modal UI is up, or deal with effects of those in some other way, but it likely would require separate patches anyways. So I'm negating my previous remark, and think the patch is reasonable.

Comment on attachment 47229 [details] Does not fire focus/blur events while a modal dialog is up Cleared Eric Seidel's review+ from obsolete attachment 47229 [details] so that this bug does not appear in http://webkit.org/pending-commit.

(In reply to comment #15) > > Ideally we would not fire any events at a page behind a modal dialog. > > Note that Firefox will fire an image onload event behind a modal dialog when > the image load finishes. I'm not sure it's a bad thing - the concept of > modality is about user input, not about something that happens > programmatically. I believe that's a bug in Firefox. JavaScript is supposed to run to completion without any interleaving threads. If events fire during a JavaScript call, then that single threaded, run to completion rule is violated.

It can be a good idea to try to find what changed this behavior, since by fixing it we might 'revert' some effects of that change. Also, this really needs a test. This 'regressed' relative to Safari 4.0.4 because there is no tests for the modal UI. Without a test, it can be broken again the next day. Currently DRT does not implement [UIDelegate webViewRunModal]. Interestingly, custom property getter for DOMWindow even hides the showModalDialog() if that method is not implemented... Perhaps simply by implementing this method we could test modal behavior using showModalDialog()? Even one port's DRT is way better then nothing, since this is platform-independent code. It can be a separate patch - or perhaps combined, considering this is a 1-line fix.

Created attachment 49249 [details] patch with manual tests This patch adds manual tests to WebCore/manual-tests/

The behavior changed from Safari 4.0.4 at http://trac.webkit.org/changeset/48257 which was a fix to bug https://bugs.webkit.org/show_bug.cgi?id=27105 "Blur and focus events are not fired when window is blurred and focused". This makes sense, the modal dialog was causing the window to get blurred/focused. However the blurred/focused window still shouldn't fire blur/focus events if it is hidden behind a modal dialog.

Hey everyone, Is there consensus on this patch? I think most of the concerns have been addressed (there is a test, we know when the behavior changed, the new behavior is in line with the spec and Safari 4). The current behavior is pretty unfortunate. A coworker reported that he couldn't use a WebKit browser with debug assertions enabled without having the check fail extremely rapidly on gmail or google calendar.

Comment on attachment 49249 [details] patch with manual tests Looking at the patch that regressed this, it looks very reasonable. r=me. It's ok to add manual test for now but please create a bug for DRT to support testing modal dialogs and consider to investigate implementation!

Filed https://bugs.webkit.org/show_bug.cgi?id=35350 to improve DRT.

Committed r55205: <http://trac.webkit.org/changeset/55205>