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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Gijs, Assigned: Gijs)
References
()
Details
(Keywords: fixed1.8.1.2, Whiteboard: [need testcase])
Attachments
(2 files, 4 obsolete files)
5.02 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
roc
:
review+
roc
:
superreview+
dveditz
:
approval1.8.1.2+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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 :-).
Assignee | ||
Comment 2•18 years ago
|
||
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)
Comment 3•18 years ago
|
||
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.
Assignee | ||
Comment 4•18 years ago
|
||
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.
Assignee | ||
Comment 5•18 years ago
|
||
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)
Assignee | ||
Comment 6•18 years ago
|
||
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
Assignee | ||
Comment 8•18 years ago
|
||
> 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)
Assignee | ||
Updated•18 years ago
|
Attachment #248991 -
Flags: review? → review?(roc)
Don't cache it. That is unnecessary extra code. Other than that, it looks good.
Assignee | ||
Comment 10•18 years ago
|
||
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?
Assignee | ||
Comment 12•18 years ago
|
||
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+
Assignee | ||
Comment 14•18 years ago
|
||
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
Assignee | ||
Comment 15•18 years ago
|
||
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 → ---
Assignee | ||
Comment 16•18 years ago
|
||
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.
Comment 17•18 years ago
|
||
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 ago → 18 years ago
Resolution: --- → FIXED
Comment 18•18 years ago
|
||
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 → ---
Assignee | ||
Comment 19•18 years ago
|
||
(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...)
Assignee | ||
Comment 21•18 years ago
|
||
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+
Assignee | ||
Comment 22•18 years ago
|
||
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 ago → 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 23•18 years ago
|
||
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 24•18 years ago
|
||
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+
Assignee | ||
Comment 25•18 years ago
|
||
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
Comment 26•18 years ago
|
||
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]
Comment 27•18 years ago
|
||
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.
Assignee | ||
Comment 28•18 years ago
|
||
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).
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•