129591 - nsWindow::Enabled is a no-op on GTK

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Robert Ginda]

The debugger *needs* to be able to rely on this method working on all platforms
for 1.0.  Chris, please help stem the tide of stomach acid by providing some
status as things progress.

Comment 1

[Robert Ginda]

Attachment: proposed patch

This patch makes nsWindow::Enable do what it's supposed to do on GTK.  It also
removes nsWindow::SetModal and ModalWidgetList.  Now that windows can be
properly disabled the generic modality code will take care of this, as it does
on Windows and Mac.

This fixes a number of other modality bugs on linux as well.

Comment 2

[Christopher Blizzard (:blizzard)]

That's pretty close to what I was going to do. :)  Yay, rginda.  I need to work
out the details of how that's going to interact with embedding, though.

Comment 3

[Robert Ginda]

FWIW, Windows works the same way, and they don't seem to have embedding issues.
 The tree nazis are going to make it very hard to check things in after Monday,
please think quickly.

Comment 4

[Robert Ginda]

Attachment: updated patch

removes the commented out code I added in the previous patch

Comment 5

[Christopher Blizzard (:blizzard)]

Sadly, embedding is really different with windows and linux.

Comment 6

[Christopher Blizzard (:blizzard)]

Attachment: patch

This patch adds enter/leave tracking for grab widgets, puts the tracking in the
right places and remerges with the tip.

Comment 7

[Robert Ginda]

Sweet.  Does this patch address your embedding concerns too?

Comment 8

[Christopher Blizzard (:blizzard)]

I played with it in embedded and it didn't seem to have any problems.  I think
this code should handle the cases properly.  Obviously, we need to test the crap
out of this code.

Comment 9

[Robert Ginda]

Blizzard, If you'd explain what the leave/enter tracking is for, I'll r=.  Or
maybe pav would give a better review.

Comment 10

[Stuart Parmenter]

Comment on attachment 74028 [details] [diff] [review]
patch

r=pavlov

Comment 11

[Alec Flett]

Comment on attachment 74028 [details] [diff] [review]
patch

nice cleanup.
sr=alecf

Comment 12

[Robert Ginda]

jrgm, Drivers has requested some QA coverage before giving the a=.  Could you
help test this out?  Do you need me to spin a build for you, or can you build
with this patch?

Comment 13

[Christopher Blizzard (:blizzard)]

Builds should show up here shortly:

http://ftp.mozilla.org/pub/mozilla/nightly/experimental/bug_129591/

Comment 14

[Robert Ginda]

hmm, latest patch doesn't seem to stop the user from closing a disabled window
via the window manager.

Comment 15

[Christopher Blizzard (:blizzard)]

Attachment: new patch

patch that handles the destroy case

Comment 16

[Robert Ginda]

I think I found a way to reproduce the activation problem you saw, blizzard.

1. Open two nav windows
2. Enter "1" into the location bar in one of the windows and press enter
3. While the "Connection refused" modal dialog is up, focus the other location
bar and type some text.
4. Dismiss the "Connection refused" dialog.
5. Try to focus the location bar on the first nav window.

Comment 17

[Christopher Blizzard (:blizzard)]

Attachment: checkpointing changes

Comment 18

[Christopher Blizzard (:blizzard)]

Attachment: working patch

This patch fixes the focus issues, at least as near as I can tell.  Please take
it out for a test drive.

Comment 19

[Christopher Blizzard (:blizzard)]

Reviews?  Testing?  Anyone?

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Taking QA from jrgm.  I've been running with previous versions of the patch and
apart from the issues posted here it's been fine.  Downloading the latest one
now and I'll take it for a spin.  Btw, thanks for doing this.  This fix does rock :)

Comment 21

[Robert Ginda]

there are too many issues on the trunk right now to know where the regressions
are coming from.  I'm creating a branch build to test with now, I'll report back
when I have more info.

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

I see no regressions as a result of this patch.  Over the past few days I've
been doing testing with "Always ask me whether to accept cookies" -- this
displays a ton of modal dialogs.  I've been bringing up various other dialogs in
the prefs window, mailnews account windows, done testing with opening multiple
modals at once, closing them in reverse order, etc.  I've inspected modals and
debugged modals with no side effect that I could discern.  It all worked
beautifully!

I have only noticed one thing which might not even be related to this patch, and
just may be a lesser bug that we've uncovered: Trying to DOM inspect a modal
window does not work right away.  The node view doesn't immediately refresh all
the time.  For it to work, you must first switch views (eg. from DOM Object view
to JS Object view) and then back.  This is not a terribly huge issue as there is
a workaround and it is surely better than not able to inspect them at all.  I'm
fine with this patch.  But would be even happier of course if this little minor
issue were fixed.

Comment 23

[Tom Mraz]

If this fixes the bug 65521 then it's surely worthy to have it in for
mozilla1.0. Is it possible to catch the m1.0 train?

Comment 24

[Christopher Blizzard (:blizzard)]

Attachment: new patch

This patch fixes a subtle bug I found where you could highlight and drag off
the window, releasing over another mozilla window and the mouse up event would
be missed.  It also cleans up some other buglets that I've found.

Comment 25

[Christopher Blizzard (:blizzard)]

Attachment: patch

Fix a couple of other bugs that I've found.  Please test this one.

Comment 26

[Christopher Blizzard (:blizzard)]

Attachment: patch

This fixed problems with clicks not working on combo boxes.

Comment 27

[Christopher Blizzard (:blizzard)]

Attachment: patch

Fix problems with crashes on some combo boxes with native scrollbars.

Comment 28

[Christopher Blizzard (:blizzard)]

Attachment: patch

Patch that fixes modal windows in embedding.  Needed to check to see if the
event target window was the child of a regular grabbing gtk widget - if so
don't rewrite the event.

Comment 29

[Christopher Blizzard (:blizzard)]

Argh.  There are still issues here.  I need a different approach.

Comment 30

[Christopher Blizzard (:blizzard)]

Attachment: patch

This is a patch that seems to work in all the cases that I can find.  And, it's
much smaller and much cleaner.	I would appreciate it if people would test this
out.

The old patch:
 5 files changed, 228 insertions(+), 316 deletions(-)
This patch:
 5 files changed, 214 insertions(+), 417 deletions(-)

Comment 31

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 80734 [details] [diff] [review]
patch

This patch looks good.	Fixes the DOM Inspector issue I had, as well another
issue I forgot to mention here with right clicks not working in any window
while a modal in another was up.

Comment 32

[Christopher Blizzard (:blizzard)]

Attachment: patch

This patch fixes a problem where if you brought up a popup window and then
clicked on the root window the popup window wouldn't dismiss.  Only minor
changes from previous patch.

Comment 33

[clkao]

the patch works great; no more accidental crash because of closing wrong window
after search.

Comment 34

[Christopher Blizzard (:blizzard)]

I don't know of any outstanding problems with the latest patch so I want to run
the gauntlet and try to get this into the tree.  Rob was seeing some
instabilities but it was hard to tell if it was from the fact that he was using
the tip or from my patch.  I certainly haven't seen any problems.

Comment 35

[Robert Ginda]

I havn't seen any on the branch, and someone recently filed bug 140088
"Chatzilla is crashing when switching tabs", which is most likely what I had
been seeing.

Comment 36

[Christopher Aillon (sabbatical, not receiving bugmail)]

I hadn't encountered many of the issues posted here with every day use.  I also
think this patch is ready for prime time.  All the issues I had were resolved
and it seems rock solid.  Let's get this in :)

Comment 37

[Colin Phillips]

Will this be just the trunk, or RC2 as well?

Comment 38

[Christopher Blizzard (:blizzard)]

I'd like to get it into RC2 but time is running short.  This needs to bake for a
bit on the trunk.

Comment 39

[Brian Ryner (not reading)]

Comment on attachment 81005 [details] [diff] [review]
patch

>+    // Find out if there's a grabbing widget.  If there is and it's a
>+    // regular gtk widget and our current event window is not the
>+    // child of that grab widget we need to rewrite the event to that
>+    // gtk grabbing widget.

A couple of commas could do wonders for this comment.


>Index: nsWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/gtk/nsWindow.cpp,v
>retrieving revision 1.369.2.1
>diff -u -r1.369.2.1 nsWindow.cpp
>--- nsWindow.cpp	10 Apr 2002 03:35:04 -0000	1.369.2.1
>+++ nsWindow.cpp	25 Apr 2002 18:48:46 -0000
>@@ -169,6 +169,24 @@
> }
> #endif
> 
>+
>+// This function will check if a button event falls inside of a
>+// window's bounds.
>+static PRBool
>+ButtonEventInsideWindow (GdkWindow *window, GdkEventButton *aGdkButtonEvent)
>+{
>+  gint x, y;
>+  gint width, height;
>+  gdk_window_get_position(window, &x, &y);
>+  gdk_window_get_size(window, &width, &height);
>+
>+  if (aGdkButtonEvent->x >= x && aGdkButtonEvent->y >= y &&
>+      aGdkButtonEvent->x <= width && aGdkButtonEvent->y <= height)
>+    return TRUE;
>+
>+  return FALSE;
>+}

Shouldn't you be checking |x + width| and |y + height| as the upper bound, not
just |width| and |height|?


>+
>+NS_IMETHODIMP
>+nsWindow::IsEnabled(PRBool *aState)
>+{
>+  NS_ENSURE_ARG_POINTER(aState);
>+
>+  *aState = !mMozArea || GTK_WIDGET_IS_SENSITIVE(mMozArea);
>+
>+  return NS_OK;
>+}
>+

There's really no need to null-check the out parameter here.

>@@ -1466,6 +1526,107 @@
> 
> //////////////////////////////////////////////////////////////////////
> /* virtual */ void
>+nsWindow::OnMotionNotifySignal(GdkEventMotion *aGdkMotionEvent)
>+{
>+  // If there's a rollup widget and this window is not a popup window,
>+  // drop the event.
>+  if (gRollupWidget && GetOwningWindowType() != eWindowType_popup) {
>+    return;
>+  }
>+
>+  XEvent xevent;
>+  GdkEvent gdk_event;
>+  PRBool synthEvent = PR_FALSE;
>+  while (XCheckWindowEvent(GDK_DISPLAY(),
>+                           GDK_WINDOW_XWINDOW(mSuperWin->bin_window),
>+                           ButtonMotionMask, &xevent)) {
>+    synthEvent = PR_TRUE;
>+  }

This is more compact, but it seems like:

if (XCheckWindowEvent(GDK_DISPLAY(),
		      GDK_WINDOW_XWINDOW(mSuperWin->bin_window),
		      ButtonMotionMask, &xevent))
  synthEvent = PR_TRUE;
while (XCheckWindowEvent(GDK_DISPLAY(),
			 GDK_WINDOW_XWINDOW(mSuperWin->bin_window),
			 ButtonMotionMask, &xevent));

would be slightly more efficient.  Your call.

Comment 40

[Christopher Blizzard (:blizzard)]

> A couple of commas could do wonders for this comment.

Yeah, it's a bit of a run on sentence.  I'll fix it.

>>+  gdk_window_get_position(window, &x, &y);
>>+  gdk_window_get_size(window, &width, &height);
>>+
>>+  if (aGdkButtonEvent->x >= x && aGdkButtonEvent->y >= y &&
>>+      aGdkButtonEvent->x <= width && aGdkButtonEvent->y <= height)
>>+    return TRUE;
>>+
>>+  return FALSE;
>>+}
> 
> Shouldn't you be checking |x + width| and |y + height| as the upper bound, not
> just |width| and |height|?

No, the event is relative to the window that gets the event.  This means that if
you clicked in the very upper left hand corner of the window the x,y would be
0,0 while if you clicked in the lower right hand corder the event x,y would be
width,height.

> This is more compact, but it seems like:
> 
> if (XCheckWindowEvent(GDK_DISPLAY(),
> 		      GDK_WINDOW_XWINDOW(mSuperWin->bin_window),
> 		      ButtonMotionMask, &xevent))
>   synthEvent = PR_TRUE;
> while (XCheckWindowEvent(GDK_DISPLAY(),
> 	
	 GDK_WINDOW_XWINDOW(mSuperWin->bin_window),
> 	
	 ButtonMotionMask, &xevent));
> 
> would be slightly more efficient.  Your call.

I don't think it's any more efficient.  You don't end up calling it any fewer times.

I'll upload a new patch in a bit.

Comment 41

[Christopher Blizzard (:blizzard)]

Attachment: patch that addresses bryner's comments

Comment 42

[Christopher Blizzard (:blizzard)]

Attachment: patch

oops, add width + x and height + y when calculating if the event is in the
window.

Comment 43

[Brian Ryner (not reading)]

Comment on attachment 81245 [details] [diff] [review]
patch

r=bryner

Comment 44

[Johnny Stenback (:jst)]

Comment on attachment 81245 [details] [diff] [review]
patch

sr=jst

Comment 45

[Christopher Blizzard (:blizzard)]

Checked in on the trunk.

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 81245 [details] [diff] [review]
patch

a=[e-mail] (and [e-mail]) for branch checkin

Comment 47

[Christopher Blizzard (:blizzard)]

Checked in on the 1.0 branch.

Comment 48

[Ben Bucksch (:BenB)]

This bug caused a severe regression (menu mouseover doesn't work), see bug 141943.

Commands to back this out:
cvs update -j1.117.2.2 -j1.117.2.1 widget/src/gtk/nsWindow.h
cvs update -j1.369.2.2 -j1.369.2.1 widget/src/gtk/nsWindow.cpp
cvs update -j1.123.2.2 -j1.123.2.1 widget/src/gtk/nsWidget.h
cvs update -j1.274.2.2 -j1.274.2.1 widget/src/gtk/nsWidget.cpp
cvs update -j1.169.2.2 -j1.169.2.1 widget/src/gtk/nsGtkEventHandler.cpp
cvs update -j1.30.16.2 -j1.30.16.1 widget/public/nsILookAndFeel.h

Comment 49

[Asa Dotzler [:asa]]

adding branch resolution keyword

Comment 50

[Christopher Aillon (sabbatical, not receiving bugmail)]

Verified fixed.  Linux RC2 and Linux trunk 2002051008.