361272 - Switching back to the browser does not work properly (Ctrl+1 or Window->Navigator)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Bengt-Erik Soderstrom]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061120 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061120 SeaMonkey/1.5a

I have noticed that switching back from one other choice (mail, composer, address book, IRC Chat) to the navigator window does not work properly any longer. A couple of weeks ago it was certainly OK.

Reproducible: Always

Steps to Reproduce:
1. Open the browser. Any URL. Then in the menu click Window -> Mail & Newsgroups. The Mail & Newsgroups opens in a normal, expected way.
2. In the menu now click Window -> Navigator. Nothing seems to happen. Actually the browser is still in the background,to see it close the mail window.
3. The same thing happens try to come back from the other menu choices (IRC, Composer etc).
4. The same thing happens if you try Ctrl+1 or the browser button in the bottom bar

Actual Results:  
Switching back to the browser window does not work

Expected Results:  
With the menu choices , bottom bar buttons, or Ctrl+1 we should come back to the browser (from Mail & Newsgroups, IRC, Composer, Address Book)

I am almost certain that this worked fine some 2-3 weeks ago.

Comment 1

[Mats Palmgren (inactive)]

Regression window: 2006-11-09-01 -- 2006-11-10-01
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-11-09+00%3A01&maxdate=2006-11-10+02%3A00&cvsroot=%2Fcvsroot
Bug 330006 is the culprit, confirmed by local backout.

Strangely, the bug does not occur if I give about:blank on the command line
(or set "Startup: Blank Page" in Preferences).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is just regressing back to the pre-bug 295447 behavior, right?

Any chance you could debug the widget hierarchy you're seeing and see why it doesn't meet the criteria I added?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Also, make sure the JS code is actually calling focus() on the chrome window and not the content window inside it.  If it's the latter, then it's the JS code that should be fixed.)

Comment 4

[Mats Palmgren (inactive)]

Attachment: Debug data

(In reply to comment #2)
> This is just regressing back to the pre-bug 295447 behavior, right?

I guess so.

> Any chance you could debug the widget hierarchy you're seeing and see why it
> doesn't meet the criteria I added?

See attached debug data. It seems we have a child window but it's not
a direct child. 

(In reply to comment #3)
> (Also, make sure the JS code is actually calling focus() on the chrome window
> and not the content window inside it.  If it's the latter, then it's the JS
> code that should be fixed.)

Looks like it's a chrome window, that's what the JS stack suggests anyway.
"this = [object ChromeWindow ..." in all the frames.

Comment 5

[Ginn Chen]

For dialogs, gdk_window_get_parent(mDrawingarea->clip_window) equals to owningWindow->mDrawingarea->inner_window.

But for seamonkey navigator window or composer window, etc., you need to call gdk_window_get_parent several times to make it equal. (7 times on my box)
For firefox browser window, you also need several times. (9 times on my box)

Comment 7

[Manuel Reimer]

(In reply to comment #3)
> (Also, make sure the JS code is actually calling focus() on the chrome window
> and not the content window inside it.  If it's the latter, then it's the JS
> code that should be fixed.)

Are you really sure? One example, for an function which sets a focus on the "last focused item" inside the window, which has to get the focus, may be this one:

http://lxr.mozilla.org/seamonkey/source/suite/common/tasksOverlay.js#87

And there are many more of them.

I think this one is definetly a bug. Especially as in Windows, the windows still seems to get the focus, if something inside them gets the "focus()"-call. So is someone here, with GTK2 knowledge, who is able to fix this one?

Comment 8

[Manuel Reimer]

I did some tries, downloaded some patches and "reverse-patched" my seamonkey. Now I'm sure that this one is caused by the patch for bug 330006 and seems to have nothing to do with the bug mentioned some comments earlier.

So what's to do now? If reopening bug 330006 is the right thing to do, then could please someone do so for me? I don't have the required permissions.

With this knowledge, my first patch suggestion for this one is the opposite of the patch in bug 330006 (removing the change) and reopening the bug, so someone is able to create a new patch.

Comment 9

[Manuel Reimer]

... I seem to have not seen that bug 330006 already has been found as the reason for this bug. Sorry for that.

And thanks to Ginn Chen for the hint with the multiple call of gdk_window_get_parent

Patch follows...

Comment 10

[Manuel Reimer]

Attachment: First try to fix the bug

Comment 11

[Manuel Reimer]

Attachment: Second patch. Missed some cleanup.

Removed the "break" and the curly brackets

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like you're writing child when you mean descendant.

I think this will also regress bug 330006.  Calling focus on something that's not a top level window should not do a raise.

Comment 13

[Manuel Reimer]

Someone will have to try that, as I'm unable to do. I tried to reproduce bug 330006 with IceWM, but there I don't have any problems.

Where would be the difference if we now do something like

foobarwindow.focus();
foobarwindow.somechild.focus();

in the Javascript part. Wouldn't that also regress bug 330006? Maybe this is just a bug in the window manager, which should be fixed there?

And, of course, on windows we still get the focus if something inside a window gets the focus. So if someone really plans to break this feature, then please break it for all platforms.

Comment 14

[Ginn Chen]

I think this patch works as same as backing out patch of bug 330006.

Also I noticed, if I backed out the patch, bug 330006 is harder to reproduce than several months ago, maybe because of the change of bug 348369, Adam Guthrie mentioned it at https://bugzilla.mozilla.org/show_bug.cgi?id=330006#c18.

Comment 15

[Manuel Reimer]

(In reply to comment #14)
> I think this patch works as same as backing out patch of bug 330006.

Hmmm. According to this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=330006#c17 you are right. My patch is in fact the same as removing the patch for bug 330006.

> Also I noticed, if I backed out the patch, bug 330006 is harder to reproduce
> than several months ago, maybe because of the change of bug 348369, Adam
> Guthrie mentioned it at
> https://bugzilla.mozilla.org/show_bug.cgi?id=330006#c18.

Yes, you are right. This bug is "double-fixed" ;-)

For me it seems like those two patches are in fact patches for the same issue. So the patch for bug 330006 makes the patch for bug 348369 obsolete and vice versa.

So my question is: What do the poeple, who own the related backend part, want?

Focus on an element focuses the window: Yes or No?

If Yes, then I'll create an "undo-patch" for the patch in bug 330006.

If No, then I'll start to fix all those things, which got broken in SeaMonkey by the fix for bug 330006, now. But then I'll also file another bug and attach a patch which also removes the "element focus also focuses window"-Feature from the Windows widget backend.

If this modification is only done for Linux, then I'm sure we will get some extensions, which will only work on Windows.

For the other platforms someone else will have to do the needed changes.

Comment 16

[Ginn Chen]

With patch of bug 348369, without patch of bug 330006,
bug 330006 still can be reproduced, the chance is about 1/10 for me.
So the patch of bug 330006 is not obsolete yet at this time.

Comment 17

[Manuel Reimer]

So there is a patch needed, which also disables focusing of windows by focusing something inside them, for Windows.

So far I couldn't find out where to patch the nsWindow.cpp, but I hope I'll find someone to help me with this.

But lets get back to the topic. As it seems to be impossible to get the patch for bug 330006 removed, we will have to add some "window.focus()" calls to SeaMonkey to make focusing of windows working again.

Comment 18

[[e-mail]]

(In reply to comment #3)
>(Also, make sure the JS code is actually calling focus() on the chrome window
>and not the content window inside it.  If it's the latter, then it's the JS
>code that should be fixed.)
The reason we focus content windows is to avoid moving the focus around within a window when all we want to do is raise it. Focusing the focused window used to do this nicely. It has an added benefit in the case of Editor, in which only the content window is focusable anyway, and focusing the chrome window simply causes focus to get lost. If we can't focus content windows to raise them, and there is no other API to raise a chrome window, then we'll have to save the previously focused element and/or window, focus the chrome window, then restore the focus.

Additionally focusing an unfocused window in code doesn't correctly blur the focused element. I dug around a bit but never did manage to work out why.

Note that we have a pref dom.disable_window_flip to allow content scripts to raise windows. This pref is therefore now meaningless on GTK2 builds.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Then it sounds like you need a raise() method.

It should be possible to switch the focus within a window without raising the containing window.

Comment 20

[[e-mail]]

(In reply to comment #19)
>It should be possible to switch the focus within a window without raising the
>containing window.
Indeed, and dom.disable_window_flip forces just that, only not for chrome.

Comment 21

[Manuel Reimer]

In other words:

- GTK2/Linux is not longer consistent with other platforms. All other platforms get their focus on the window if anything gets the focus inside the window.
- A pref, which works on all other platforms (dom.disable_window_flip) is broken for GTK2/Linux.
- Fixing the SeaMonkey code is only possible using hacks (like remembering the previously focused element, focusing the window and restoring the previously focused element).
- All this because of a fix where noone knows if it really is needed. If I try to reproduce bug 330006 with IceWM, then the window doesn't move the desktop for me, but my window manager automatically switches back to the desktop where the SeaMonkey window has been. Seems like this has to be fixed in GNOME!

For me this is a pretty ugly situation. There should *never* be inconsistence between platforms!

Comment 22

[Manuel Reimer]

(In reply to comment #18)
> It has an added benefit in the case of Editor, in which only
> the content window is focusable anyway, and focusing the chrome window simply
> causes focus to get lost. If we can't focus content windows to raise them, and
> there is no other API to raise a chrome window, then we'll have to save the
> previously focused element and/or window, focus the chrome window, then
> restore the focus.

It's also possible to fix this using a hack in Composer, but this wouldn't be much less ugly as your fix. I tried to find out why Composer loses focus, but I couldn't figure out why.

For any reason we get the "onfocus" for four times when calling "window.focus()" on Composer. For the first two times we still have the right element focused inside the window. With the third event, the last focused element is gone and replaced with "window". Using Venkman I've found out, that the edit menu gets updated between the second and third call of "onfocus", but even when commenting out this part I still lose the last focused element... :-?

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed by backing out and refixing bug 330006.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #20)
> (In reply to comment #19)
> >It should be possible to switch the focus within a window without raising the
> >containing window.
> Indeed, and dom.disable_window_flip forces just that, only not for chrome.

No, bug 330006 happens even with dom.disable_window_flip turned on (which is the default in both Firefox and SeaMonkey, although not Gecko).

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, sorry, missed the "only not for chrome".