Closed Bug 361781 Opened 18 years ago Closed 18 years ago

nsIDomInternalWindow::GetAttention on GTK2 steals focus when the app has focus, blinks otherwise

Categories

(Core :: DOM: Core & HTML, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

()

Details

(Keywords: fixed1.8.1.2, Whiteboard: [need testcase])

Attachments

(2 files, 4 obsolete files)

Steps to reproduce:
1. Open a Firefox/Seamonkey window.
2. Open ChatZilla or Venkman, or another window where you can execute JS with chrome privileges (or write a test page, get it chrome privileges and run it in another window)
3. In your privileged JS environment, run
setTimeout(window.getAttention, 5000);
and switch to your browser window.

Expected result:
The ChatZilla/Venkman/Other window flashes in your window manager's task bar/panel/whatever.

Actual result:
The window steals focus.

If you try and do the same thing and switch to a window of another application, the window does 'just' flash in the task bar/panel/whatever.

On Windows, the window always flashes and never steals focus. Compare:
http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsWindow.cpp#1403
and
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#7633

Biesi mentioned we should probably be using gdk_window_set_urgency_hint() instead. This would require upping our GTK2 version requirements, upon which biesi commented:
<biesi>	if necessary we can use dlsym
<biesi>	though the recent linux discussion seems to indicate that we might be able to require a newer gtk
See the URL of this bug for 'documentation' of the set_urgency_hint thing.
Recently, I've switched from GNOME to XFCE, and behaviour there is still different. Regardless of whether Firefox is focused, the window is raised, but not focused. This is possibly more annoying because you think the window is focused (because it was raised, and in my case it's maximized, so you can't actually see any cursor or focus ring on the previously visible window), but your input is still going to the other window. I might be able to try my hand at writing a patch tomorrow, but don't count on it :-).
This patch does a couple of things:

* typedef the function definition for the set_urgency_hint call.
* use PR_FindFunctionSymbolAndLib to get a pointer to that function whenever needed
* call it with urgency set to true in GetAttention, and false in all calls informing us of the window receiving focus, because otherwise it will be set to urgent until the end of time, which we don't want.
* fallback on the current call (gdk_window_show_unraised) if the urgency hint function isn't present, and fix the one internal caller of GetAttention to still use this directly (since it doesn't make sense for a setfocus call to set the window as urgent, at least I don't think it does).

I've never done any stuff in widget code before, so apologies in advance if I did silly things. I also don't know if I need sr, so just asking for r? for now.

I've tested the patch on a build on ubuntu edgy (6.10), which has gtk2.10.6 installed. I'll test it on a Debian Sarge machine tomorrow, which has gtk2.6.4 with some security fixes, as far as I can tell. Though of course it won't be functional there, it shouldn't break things either (and just fall back on current behaviour).
Assignee: general → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #248192 - Flags: review?(roc)
FWIW, I believe you do need super-review.

Also, you seem to be poking NSPR for the function pointer every time. Assuming NSPR doesn't cache these things, it might be better to initialise the pointer once (first time nsWindow' constructor is called, maybe?) and then re-use it in the current places.
James, I considered that, but had this discussion on IRC:

<Hannibal> roc: hi! I'm trying to hack nsWindow::GetAttention on gtk2 to use gtk_window_set_urgency_hint, if available, instead of raising the window (which seems to be what it does now)
<roc> ok
* Hannibal needs to cope with it maybe not being there, and unsetting the urgency hint when the window gets focus back
<biesi> can you just call it with false when you get a focus event?
<Hannibal> so, am I right I should unset it in SetFocus and onContainerFocusInEvent ?
<Hannibal> biesi: afaict, the nsWindow code is actually the stuff that dispatches those events
<Hannibal> so I'm not sure how I would go about doing that.
<Hannibal> http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsWindow.cpp#696 and http://lxr.mozilla.org/seamonkey/source/widget/src/gtk2/nsWindow.cpp#1955, fwiw
[2006-12-10 22:09:48] <Hannibal> roc: if that is correct, I'm wondering where I should check whether the function is available at all - do I just call nspr's dlsym wrapping stuff (like ::SetCursor) every time I need it?
<Hannibal> or can I just save that in some static reference on creation (where would that be?) and nullcheck it whenever needed?
<roc> I wouldn't bother caching it
<Hannibal> ok :)
<roc> this doesn't occur with high frequency

Maybe I misunderstood something though. Like I said, I'm just poking around for the first time.

Also, on XFCE 4.3.99.1 (as shipped on Ubuntu Edgy), setting the urgency hint doesn't really seem to do anything at all - which confuses me. It might be a bug in XFCE, or in my code or understanding of the APIs. I tested on GNOME yesterday, that worked great. I'll try to investigate tonight or tomorrow.
Comment on attachment 248192 [details] [diff] [review]
Broken Patch with dlsym-wrapper set_urgency_hint

This patch actually doesn't work because of the focus check (this != gFocusWindow) - this seems to just be false all the time. Looking into how to actually do that...
Attachment #248192 - Attachment description: Patch with dlsym-wrapper set_urgency_hint → Broken Patch with dlsym-wrapper set_urgency_hint
Attachment #248192 - Attachment is obsolete: true
Attachment #248192 - Flags: review?(roc)
Attached patch Working patch (obsolete) — Splinter Review
Patch that does actually work.

Tested on KDE, GNOME and XFCE (all on ubuntu edgy). Seems to work perfect there. As mentioned in one of the above comments, I did mean to test it on a machine where set_urgency_hint wouldn't be available, but unfortunately it turned out debian sarge knows nothing about cairo, so I gave up on that. If anyone has a machine with GTK2 < 2.8 available and willing to test this patch, I have a build  with it up on http://www.gijsk.com/temp/fx_blink.zip .
Attachment #248547 - Flags: review?(roc)
+    GtkWidget* top_window = nsnull;
+    GetToplevelWidget(&top_window);
+    if (top_window && (GTK_WIDGET_VISIBLE(top_window))) {
+        PRLibrary* lib;
+        _gdk_window_set_urgency_hint_fn _gdk_window_set_urgency_hint;
+        _gdk_window_set_urgency_hint = (_gdk_window_set_urgency_hint_fn)
+            PR_FindFunctionSymbolAndLibrary("gdk_window_set_urgency_hint", &lib);
+        if (_gdk_window_set_urgency_hint)
+            (*_gdk_window_set_urgency_hint)(top_window->window, PR_FALSE);
+    }

Make this a shared helper so you can call it from both places
Attached patch Better Patch (obsolete) — Splinter Review
> Make this a shared helper so you can call it from both places

Hm. I assumed you meant from all three places, to turn it on/off for a given top_window. I do hope I'm right, or I can go through this again. :-)

Also, I've made the helper function cache it like it does for the pix cursor thing.
Attachment #248547 - Attachment is obsolete: true
Attachment #248991 - Flags: review?
Attachment #248547 - Flags: review?(roc)
Attachment #248991 - Flags: review? → review?(roc)
Don't cache it. That is unnecessary extra code. Other than that, it looks good.
OK, done, I believe.
Attachment #248991 - Attachment is obsolete: true
Attachment #249048 - Flags: review?(roc)
Attachment #248991 - Flags: review?(roc)
Sorry! I think you need to release the PRLibrary, don't you?
OK, so then this should be better? I never thought I had to unload it because this code:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&mark=929,931,933#915
doesn't seem to do so either. Though maybe that's a separate bug? :-)
Attachment #249048 - Attachment is obsolete: true
Attachment #249053 - Flags: review?(roc)
Attachment #249048 - Flags: review?(roc)
Comment on attachment 249053 [details] [diff] [review]
Patch v5, call PR_UnloadLibrary before exiting

yes, that is a separate bug
Attachment #249053 - Flags: superreview+
Attachment #249053 - Flags: review?(roc)
Attachment #249053 - Flags: review+
Checking in mozilla/widget/src/gtk2/nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.195; previous revision: 1.194
done
Checking in mozilla/widget/src/gtk2/nsWindow.h;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.h,v  <--  nsWindow.h
new revision: 1.67; previous revision: 1.66
done

Bug 364331 filed for the release lib issue with the SetCursor code.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
luna and lhasa are failing tests, James Ross suggested I should nullcheck lib before PR_UnloadLibrary'ing it. Checked in a patch, waiting for them to cycle again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OK, so that didn't help, lhasa stayed orange after getting my checkin, so I've backed everything out again. Still not quite sure what went wrong, though.
This is what happens with this patch on gtk 2.4:

Reading libjpeg.so.62.0.0
Reading libsmime3.so
Reading libgtkxtbin.so
Reading libXt.so.4
t@1 (l@1) signal SEGV (no mapping at the fault address) in nsCommonWidget::DispatchGotFocusEvent at line 163 in file "nsCommonWidget.cpp"
  163       DispatchEvent(&event, status);
(dbx) where
current thread: t@1
=>[1] nsCommonWidget::DispatchGotFocusEvent(this = 0x8192c6f), line 163 in "nsCommonWidget.cpp"
  [2] nsWindow::OnContainerFocusInEvent(this = 0x8192c6f, aWidget = 0x8183b78, aEvent = 0x8490fb0), line 1996 in "nsWindow.cpp"
  [3] focus_in_event_cb(widget = 0x8183b78, event = 0x8490fb0), line 4306 in "nsWindow.cpp"
  [4] _gtk_marshal_BOOLEAN__BOXED(0x80c6458, 0x11fc0, 0x610, 0x0, 0x83361c0, 0x0), at 0xfe8eea9c 
  [5] 0x5(0xfe637ccc, 0x0, 0x0, 0xfe9c93ea, 0xfe9c94c4, 0xfe636d9f), at 0x5 
  [6] 0x0(0x0, 0x83552c8, 0x1c5, 0x83610f0, 0x8361c20, 0x83604f8), at 0x0 
(dbx) dump
event = CLASS
this = 0x8192c6f
status = nsEventStatus_eIgnore
(dbx) 
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Reopening because the initial checkin was backed out...

http://www.mozilla.org/projects/nspr/reference/html/prlink.html#24247

This doesn't mention anything about the value of lib if the call failed, so I suspect in case of failure; perhaps the following is what should be done:

+void
+nsWindow::SetUrgencyHint(GtkWidget *top_window, PRBool state)
+{
+    if (!top_window)
+        return;
+
+    // Try to get a pointer to gdk_window_set_urgency_hint
+    PRLibrary* lib;
+    _gdk_window_set_urgency_hint_fn _gdk_window_set_urgency_hint;
+    _gdk_window_set_urgency_hint = (_gdk_window_set_urgency_hint_fn)
+           PR_FindFunctionSymbolAndLibrary("gdk_window_set_urgency_hint", &lib);
+
+    if (_gdk_window_set_urgency_hint) {
+        (*_gdk_window_set_urgency_hint)(top_window->window, state);
+        PR_UnloadLibrary(lib);
+    }
+    else if (state)
+        gdk_window_show_unraised(top_window->window);
+}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #18)
> http://www.mozilla.org/projects/nspr/reference/html/prlink.html#24247
> 
> This doesn't mention anything about the value of lib if the call failed, so I
> suspect in case of failure; perhaps the following is what should be done:
> 
<patch snipped>

Yeah, I had actually done that, and some other modifications, myself too. I just need them tested on a machine with an earlier gtk2 version. For which purpose I've installed FC4 on the machine that used to run debian sarge. We'll soon find out if it's actually able to compile stuff - but I know for sure that even if it does, it'll be busy for at least 2-3 hours. So don't hold your breath :-)

So the difference between this patch and the last:
* only PR_UnloadLibrary the lib if the function we want is non-null.
* call the function without the dereference. I have no idea if this makes a difference - it works on this machine (with an up-to-date gtk2 version) and it seems to be how other code does this
* make _gtk_window_set_urgency_hint return void, not void*. Oops. (Not that anything cared, but...)
Patch v6 tested on GTK 2.4. Builds and runs as previously.
Comment on attachment 249119 [details] [diff] [review]
Patch with hopefully better sanity checking

Works fine on my FC4 box (GTK 2.6.something), too, so asking review again. :-)

(in case it wasn't clear, above comment did in fact mean that there were no crashes and such after the patch, either)
Attachment #249119 - Flags: superreview?(roc)
Attachment #249119 - Flags: review?(roc)
Attachment #249119 - Flags: superreview?(roc)
Attachment #249119 - Flags: superreview+
Attachment #249119 - Flags: review?(roc)
Attachment #249119 - Flags: review+
Last patch checked in, all the boxes that went orange last time have since cycled and are still green. Marking FIXED.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Comment on attachment 249119 [details] [diff] [review]
Patch with hopefully better sanity checking

I would really like to have this fix on the 1.8 branch.

Risk: low, feature is only accessible to privileged code so webpages won't break. It's used sparsely in existing shipping chrome code (ie, browser, mail).

The patch has been baking on trunk for about 3 weeks now, and as far as I know there have not been any regressions.

Reward: provide a better and more consistent user experience, especially for users of ChatZilla and/or Calendar - both of which use this code to notify the user that someone or something needs their attention. Currently, the old behaviour is inconsistent amongst different mainstream window managers (XFCE and Gnome at least) and (at least in some cases) depends on whether a window in the same process is focused. This generally leads to annoying user interface (focus stealing, or lack of any notification). See also bug 366595.

The patch provides a more consistent user experience on newer GTK2 versions, and falls back on the old behaviour on older versions of GTK2 (where the better behaviour is not available). The newer behaviour is also a lot more similar to the behaviour on Windows.
Attachment #249119 - Flags: approval1.8.1.2?
Comment on attachment 249119 [details] [diff] [review]
Patch with hopefully better sanity checking

approved for 1.8 branch, a=dveditz for drivers
Attachment #249119 - Flags: approval1.8.1.2? → approval1.8.1.2+
Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.145.2.11; previous revision: 1.145.2.10
done
Checking in nsWindow.h;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.h,v  <--  nsWindow.h
new revision: 1.55.2.2; previous revision: 1.55.2.1
done

Checked in with the backport of changing gdk_window_show_unraised in the patch to gdk_window_show. See bug 352936.
Keywords: fixed1.8.1.2
hi, can you shed a little more information on how to test this fix for QA?  I managed to get chatzilla program to flash in the background after setting the timeout, but i could do that on v1.5.10, 2.0.0.1, and 2.0.0.2.   So i'm not sure if i am repro'ing the initial problem or not.  
Whiteboard: [need testcase]
hi Gjis, can you please comment on a testcase for your fix so QA can verify? As i mentioned earlier, i could repro you're original issue you found, but not the fix.   Or if its easier for you to verify it against the 1_8 Branch for us, then that would be appreciated.  Thanks.
Tony, first of all, my apologies for not picking up on this earlier. I've been quite busy. I have checked and verified the fix on the branch myself, but it's not really good practice to mark your own bugs as VERIFIED, I think. So perhaps I should give more detailed steps to reproduce:

1. Open Firefox
2. Open ChatZilla  (Tools > ChatZilla)
3. Enter the following command:

/eval setTimeout(window.getAttention, 5000)

and hit enter, and switch to the Firefox window you still have open.

4. Wait about 5 seconds.

Results on Firefox 2.0.0.1 on Xubuntu Edgy:
The ChatZilla window is brought to the front (though not focused). As mentioned in comment #0, on Gnome on the same machine (so Ubuntu Edgy) the window is brought to the front AND focused.

(Expected) results on Firefox 2.0.0.2 on Xubuntu Edgy:
The ChatZilla item in the taskbar flashes, but is not focused or brought to the front. The same should happen on Gnome and KDE. Keep in mind that for this behaviour to occur, you need a recent version of GTK2 (2.8 or greater).
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: