126786 - JS debugger needs new window modality model

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Robert Ginda]

Mozilla's current implementation of modal windows varies by platform. The JS
debugger can be made to work with modal windows on some platforms like Windows,
but not on others like the Mac (see bug 100677.) The debugger would like to see
all platforms move to a model where modality is handled by enabling and
disabling individual windows (the modal window's parent on most platforms, and
obviously all windows but the modal one on Mac OS <X.)

This new modality model may also fix, or make easier to fix:
bug 55472 modal dialog parent has active min/max/close widgets [linux]
bug 53476 parent menus accessible from Keyboard while modal dialog is up
bug 32204 modal dialogs still allow user to click toolbar buttons in the parent
window
bug 22720 Account Wizard should be window-modal, not app-modal
bug 99728 Modal dialogs do not block keyboard events (only mouse events)
bug 78421 need generic modality service
bug 65521 [linux] modal dialogs should only freeze parent window (not all windows)
bug 39841 modal window lost focus, now mozilla essentially hung
bug 55977 File save/open dialogs should not be app modal.

Comment 1

[Dan M]

Attachment: use window enabling model on the Mac

We're already there on Windows. gtk will be there after nsIWidget::Enable is
properly hooked up. blizzard is working on that, but I can't find a bug number.
That leaves the Mac. This patch actually seems to work. I've gutted event
filtering in the ShowModal event loop and filtered out mouse messages to
disabled windows. (More messages should probably be filtered out, but I'd like
to err on the side of less filtering, and this seems sufficient for modal
window work.)

Comment 2

[Robert Ginda]

sweet.  blizzard, do you want a new bug for gtk window enabling, or are you
still planning to slip it in with bug 121011?

Few comments on the patch...

+#if defined(XP_MAC) && !defined(XP_MACOSX)
+   EnableOtherWindows(PR_FALSE);
+#endif

Could we do this as a pref, and set an appropriate default per platform?  I love
prefs :)

+NS_IMETHODIMP nsXULWindow::SetEnabled(PRBool aEnable)
+{
+  // we support multiple disabling so stacked modal dialogs will work
+  NS_ASSERTION(mDisabledLevel > 0 || !aEnable, "puzzling attempt to overenable
a xul window");
+  if (aEnable) {
+    if (--mDisabledLevel == 0 && mWindow)
+      mWindow->Enable(PR_TRUE);
+  } else {
+    if (++mDisabledLevel == 1)
+      mWindow->Enable(PR_FALSE);
+  }
+  return NS_OK;
+}

The else cause is ambiguous, mWindow might be null.  Also, the debugger will
need to enable itself regardless of mDisableDepth.  What would you say to having
Get/SetEnabled call straight through to mWindow->Enable, and add something like
Freeze and Thaw methods that honor mDisableLevel (and maybe even return it?)

The thought of something like
while (!window.enabled) { ++n; window.enabled = true; }
isn't very appealing :)

Comment 3

[Dan M]

about Robert's first comment:

+#if defined(XP_MAC) && !defined(XP_MACOSX)
+   EnableOtherWindows(PR_FALSE);
+#endif

doesn't want to be a preference. It's just the way either platform works. OS<X
expects modal windows to shut down the whole app; OSX doesn't. It would be
incorrect to disable all other windows on OSX and incorrect to not do it on OS9.
Also note it still isn't quite right. I've moved the #ifdef inside the method call:

+nsresult nsXULWindow::EnableOtherWindows(PRBool aEnable)
+{
+#if defined(XP_MAC) || defined(XP_MACOSX)
+  if (::OnMacOSX())
+    return NS_OK;
+
+  nsCOMPtr<nsIWindowMediator> mediator(do_GetService(kWindowMediatorCID));
+  if(!mediator)
+    return NS_ERROR_FAILURE;

second comment:

D'oh! Brace-challenged! I've heard of people who find

if () {
 if()
   x
} else

confusing, but I find it elegant and minimalist and no harder to read and it
makes me happy. But you're right, I do need to add an (&& mWindow) to the
++mDisabledLevel == 1 clause.

About mDisabledLevel: ooo, that's a problem. I need levels of disabling to
handle stacked modal windows. If window A poses modal window B which poses modal
window C, when C goes away and enables all other windows, I need window A to
remain disabled for B. Hoo. How about nsIXULWindow::ForceEnable(bool aEnable) ?
You'd have to be careful with it.

Comment 4

[Robert Ginda]

+nsresult nsXULWindow::EnableOtherWindows(PRBool aEnable)
+{
+#if defined(XP_MAC) || defined(XP_MACOSX)

I guess, but doesn't a method called EnableOtherWindows that only does what it
claims to do on one specific platform leave you cold?

>How about nsIXULWindow::ForceEnable(bool aEnable) ?  You'd have to be careful
> with it.

That's kinda where I was going with my comment, except ForceEnable would be
Get/SetEnable, and what we now call Get/SetEnable would become Freeze and Thaw.

For you're uses, where you need to maintain mDisableLevel, you'd call Freeze()
to add to mDisabledLevel, and Thaw() to subtract.  Freeze() and Thaw() might
even want to return mDisabledLevel.

The debugger would avoid the whole mDisableLevel thing, and call Get/SetEnabled,
which would leave mDisableLevel untouched.

Freeze and Thaw (and Pause and UnPause) are names I use for similar
functionality in the debugger, but if you prefer other names that's fine by me.

Comment 5

[Dan M]

Attachment: use window enabling model on the Mac, improved version

Improved version of the last patch. I got rid of
nsXULWindow::EnableOtherWindows completely, and nsIBaseWindow::SetEnabled no
longer has a counter on XUL windows; it just works flat out. I think this
solves all objections made so far to the previous patch. Robert?

Comment 6

[Robert Ginda]

Comment on attachment 70979 [details] [diff] [review]
use window enabling model on the Mac, improved version

Looks good to me, r=rginda

Comment 7

[David Hyatt]

sr=hyatt

Comment 8

[Robert Ginda]

For those of you considering whether or not to land this for 0.9.9...

Our current window modality code is implemented three different ways for the
three main platforms.  Windows disables the parent window, gtk uses the gtk
notion of modality (the cause for the mozilla1.0 bug 65521), and mac uses a
complicated event filter.  This patch makes all three platforms work like
Windows.  This brings everyone under a single modality paradigm, which is much
easer to understand and maintain, and arguably more robust.  It's exactly the
kind of change we *need* take for 0.9.9, which may be our last chance to clean
up interfaces like this.

The comment #1 lists nine other window modality bugs which will benefit from a
single, cross platform notion of how to make a modal window.  Some of these bugs
will be fixed by this patch, others will become *posible* to fix.

The debugger can *not* debug modal windows without this patch.  If we don't land
this, we won't be able to come up with a workaround later.

Comment 9

[Mike Pinkerton (not reading bugmail)]

+        // do the drag, but not if the window is disabled or if the command key
+        // is not down. (DragWindow will activate the window itself if
+        // the command key isn't down.)
+        nsCOMPtr<nsIWidget> topWidget;
+        PRBool              enabled;
+        nsToolkit::GetTopWidget(whichWindow, getter_AddRefs(topWidget));
+        if (!topWidget || NS_FAILED(topWidget->IsEnabled(&enabled)) || enabled ||
+             (anEvent.modifiers & cmdKey)) {

so this will disallow dragging of windows when the parent is "disabled"? problem
is, you are allowed to drag the parent of a sheet window. won't this break that
behavior?

cc'ing rjc, as there are some sheet/modality changes here

Comment 10

[Dan M]

Oh, for the love of Mike.
Thanks for pointing that out (Pink).
Alright, I'll change it to this:

+        /* Do the drag, but not if the window is disabled or if the command key
+           is not down. (DragWindow itself will activate the window if
+           the command key isn't down.) However, allow the drag on OSX.
+           On OSX we disable only the single parent of a window with a sheet,
+           so it's a safe-ish assumption that the disabled window can be
+           dragged and brought to the foreground. */
+        nsCOMPtr<nsIWidget> topWidget;
+        PRBool              enabled;
+        nsToolkit::GetTopWidget(whichWindow, getter_AddRefs(topWidget));
+        if (nsToolkit::OnMacOSX() ||
+            !topWidget || NS_FAILED(topWidget->IsEnabled(&enabled)) || enabled ||
+            (anEvent.modifiers & cmdKey)) {
+          Rect screenRect;
+          ::GetRegionBounds(::GetGrayRgn(), &screenRect);
+          ::DragWindow(whichWindow, anEvent.where, &screenRect);

It's best not even to call DragWindow on OS9 because DragWindow itself Selects
the window if the command key isn't down, so calling DragWindow allows the
disabled window to come to the front. After the user lets go of the mouse the
window is quickly shoved back below the modal window by our window layering code
which notices the problem and fixes it. It's unsightly.

Comment 11

[Robert John Churchill]

One thing to note with Mac OS X is that a top-level document window can have
multiple sheets opened on it.  For example: open up a blank composer window,
choose Insert->Table (which brings up a sheet), then click the "Advanced Edit"
button which opens another sheet on top of that.  You'll want to test that case
to make sure that modality/focus/etc all still work.

Comment 12

[Mike Pinkerton (not reading bugmail)]

i'm not convinced this new patch will help. not every modal dialog in our app is
a sheet (nor should it be, probably). we still can have cases where we have an
app-modal dialog up, right rjc?

oh, and i came across the "sheets having sheets" thing. oh my god, what do the
UI guidelines say about that? 

Comment 13

[Dan M]

  Let me be the second to admit that this patch probably isn't perfect. But it's
functional and pretty close. I want to get it into 0.99 because I want it to
have a chance to bake in public for a milestone before it gets set in 1.0
concrete. I'm ready to write changes for specific objections, but nonspecific
uneasiness is a bad reason to hold it up.
  There are no application-modal dialogs in our OSX build. All modal dialogs are
sheets and the event filter that makes all windows dead has already been
unplugged on OSX.
  I bet there's no other application with a UI so bad that we can look to it for
guidelines on the treatment of stacked sheets. And you've no doubt noticed there
are problems with our stacked sheets. My patch neither helps nor hinders that.

Comment 14

[Mike Pinkerton (not reading bugmail)]

my point was just that while we don't have any _now_, we will have non-sheet,
app modal dialogs in the future. not every modal dialog in our app should be a
sheet. we have several bugs to go clean that up.

Comment 15

[Dan M]

  App modal? Really? I thought OSX was all about the non-app-modal dialogs.
  I'm just currently trying to convince drivers that we should take this change
for 0.99 and I'd like to summarize that there are no known current problems,
though as Mike says we can foresee that it may need to be patched again when we
bring on another feature. Probably we'll have to note in the parent window that
it has a sheet up.
  PS drivers -- yes, the patch has been reviewed (comment 6) and sooper reviewed
(comment 7).

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 70979 [details] [diff] [review]
use window enabling model on the Mac, improved version

a=asa (on behalf of drivers) for checkin to 0.9.9. This was noted in email but
not on the bug.

Comment 17

[Dan M]

Thanks -- i'll try not to disappoint. Patch is in. Window enabling is the new
way on the Mac.

Comment 18

[Colin Phillips]

with this patch, is there still a way to have modal windows which completely
freeze all mozilla windows that are currently open?  Ie, can I have dialog x
freeze just its parent, and dialog y freeze all windows?  Is there a modifier we
can pass on the window.open() call to say "make this window halt everything
until I'm dealt with"?  

Comment 19

[Robert Ginda]

Colin: modal windows will do what they're "supposed" to do on each platform. 
That is, they're window modal everywhere except mac OS<X.  However, now that any
window can be disabled, you could easily enumerate all open windows via
nsIWindowWatcher, and disable as many windows as you'd like.  A pair of utility
functions, perhaps |disableOtherWindows(win)| and |enableOtherWindows(win)|,
should be trivial to implement.