Closed Bug 295447 Opened 19 years ago Closed 19 years ago

Ctrl+1/2/4/5/6 raises window without focus

Categories

(Core :: Widget: Gtk, defect)

Product:
Core
Component:
Widget: Gtk
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

References

Details

Attachments

(1 file, 1 obsolete file)

patch v2
1.15 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review 
Open Mozilla with one navigator window and one mailnews window
1)
Use Alt+Tab to focus the navigator window.
Use Ctrl+2 to switch to the mailnews window.
Notice the mailnews window raises without focus.

2)
Use Alt+Tab to focus the mailnews window.
Use Ctrl+1 to switch to the navigator window.
Notice the navigator window raises without focus.
Here is the log of nsWindow::SetFocus() (widget/src/gtk2)

Suppose window[0x8aae6a8] is active.
Press Ctrl+1 to switch to window[0x8757d28]
We will get
  SetFocus [0x8757d28]
  grabbing focus for the toplevel [0x8757d28]
Press Ctrl+2 to switch back
We will get
  SetFocus [0x8aae6a8]
  already have focus [0x8aae6a8]

A possible fix is
change gtk_widget_grab_focus(owningWidget);
to gtk_window_present((GtkWindow*)owningWindow->mShell);
in nsWindow::SetFocus()

Because gtk_widget_grab_focus doesn't work at all.

I also found following comments.

void
nsWindow::OnContainerFocusInEvent(GtkWidget *aWidget, GdkEventFocus *aEvent)
{
    LOGFOCUS(("OnContainerFocusInEvent [%p]\n", (void *)this));
    // Return if someone has blocked events for this widget.  This will
    // happen if someone has called gtk_widget_grab_focus() from
    // nsWindow::SetFocus() and will prevent recursion.
    if (mContainerBlockFocus) {
        printf("Container focus is blocked [%p]\n", (void *)this);
        return;
    }

But nsWindow::OnContainerFocusInEvent is never invoked by calling
gtk_widget_grab_focus().

Next, in nsWindow::SetFocus() after gtk_widget_grab_focus(),
DispatchActivateEvent(); is never called either.
Because mActivatePending is set PR_TRUE in nsWindow::OnContainerFocusInEvent()

        gtk_widget_grab_focus(owningWidget);
        owningWindow->mContainerBlockFocus = PR_FALSE;

        DispatchGotFocusEvent();

        // unset the activate flag
        if (owningWindow->mActivatePending) {
            owningWindow->mActivatePending = PR_FALSE;

            DispatchActivateEvent();
        }
see bug 202145
it is a possible fix if
change gtk_widget_grab_focus(owningWidget);
to gtk_window_present((GtkWindow*)owningWindow->mShell);
in widget/src/gtk2/nsWindow.cpp
Attached patch patch (obsolete) — — Splinter Review 
bryner, can you review it?
Thanks.
Assignee: blizzard → ginn.chen
Status: NEW → ASSIGNED
Attachment #184993 - Flags: review?(bryner)
*** Bug 202145 has been marked as a duplicate of this bug. ***
*** Bug 281561 has been marked as a duplicate of this bug. ***
Does GTK1 have the same problem?
yes, it works the same in both gtk1 and 2

see bug 202145 -- blizzard consider this a feature.
What I meant to say, was do we need the same fix for GTK1 as well?
*** Bug 295194 has been marked as a duplicate of this bug. ***
(In reply to comment #9)
> What I meant to say, was do we need the same fix for GTK1 as well?

I wonder how many people are still building with GTK1.
Firefox requires GTK2 now.
I don't know how to fix this for GTK1 since GTK1 doesn't have gtk_window_present or a method to do 
so.

I'm sure it's a bug not a feature.
Consider we have a navigator window A with 2 or more tabs in workspace 1, and another navigator 
window B with only 1 tab in workspace 2. If we press CTRL+Q within the navigator window B. A 
"Confirm close" dialog will be popped up in workspace 1 but we could not notify it.
It's bad as we're going to press CTRL+Q time and time again with no responses.
It also blocks Bug 283103, Bug 260560 which are well-known security issues.

From gtk2 documents:

gtk_window_present ()

void        gtk_window_present              (GtkWindow *window);
Presents a window to the user. This may mean raising the window in the stacking order, deiconifying it, 
moving it to the current desktop, and/or giving it the keyboard focus, possibly dependent on the user's 
platform, window manager, and preferences.

If window is hidden, this function calls gtk_widget_show() as well.

This function should be used when the user tries to open a window that's already open. Say for example 
the preferences dialog is currently open, and the user chooses Preferences from the menu a second 
time; use gtk_window_present() to move the already-open dialog where the user can see it.
Attachment #184993 - Flags: review?(bryner) → review?(roc)
I'm worried that this might make us start popping windows to the top unexpectedly. Did gtk_widget_grab_focus() grab the global focus and start directing all keystrokes there? Or did it just set the focus for the window it's in?

Shouldn't we be raising windows only if aRaise is true in nsWindow::SetFocus?

Why are the changes to navigator.js and tabbrowser.xml needed?
(In reply to comment #12)
> I'm worried that this might make us start popping windows to the top
> unexpectedly. Did gtk_widget_grab_focus() grab the global focus and start
> directing all keystrokes there? Or did it just set the focus for the window
> it's in?
From GTK's manual,

gtk_widget_grab_focus ()

void        gtk_widget_grab_focus           (GtkWidget *widget);
Causes widget to have the keyboard focus for the GtkWindow it's inside. widget must be a focusable widget, such as a GtkEntry; something like GtkFrame won't work. (More precisely, it must have the GTK_CAN_FOCUS flag set.)

So I think it can't grab the global focus.
It should not be used in nsWindow::SetFocus.

> Shouldn't we be raising windows only if aRaise is true in nsWindow::SetFocus?
I don't think it's necessary.
Can a window be focused but not showing?
I think we should ignore aRaise.
We should also remove GetAttention(-1) from nsWindow::SetFocus, gtk_window_present does the same job.
I checked other platforms' nsWindow.cpp, aRaise is ignored.
 
> Why are the changes to navigator.js and tabbrowser.xml needed?
Make sure the "Confirm close" window get focus.

Thanks for your comment!

> Can a window be focused but not showing?

Sure.  Lowered windows have focus all the time with sloppy focus.
If a web page calls focus() on one of its controls (e.g. when google.com loads it focuses the search box), will that cause the window to come to the front now?

If changes were needed in navigator.js and tabbrowser.xml, what makes you think those are the only changes needed?
Attached patch patch v2 — — Splinter Review 
More comfortable patch.
1) keep gtk_widget_grab_focus(owningWidget);
2) test aRaise
3) split changes of navigator.js and tabbrowser.xml to another bug
Attachment #184993 - Attachment is obsolete: true
Attachment #205289 - Flags: review?(roc)
Attachment #184993 - Flags: review?(roc)
(In reply to comment #15)
> If a web page calls focus() on one of its controls (e.g. when google.com loads
> it focuses the search box), will that cause the window to come to the front
> now?

No, set focus to a control won't bring the window up.
window.focus() can bring it up.

Let me clarify what will change with this patch.
Without it:
  GetAttention(-1) raises the window, but it doesn't have focus.
  The old window under it still has focus. (It can cause a security hole.)
With it:
  The window is raised with focus.

It doesn't change the behaviour of whether the window is brought to front.


> If changes were needed in navigator.js and tabbrowser.xml, what makes you think
> those are the only changes needed?
No, I don't think they're the only changes needed.
A customer complained about the dialog, so I fixed it.
Now I decide to split the patch, since they should be different bugs.


Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.153; previous revision: 1.152
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This also seems to have caused bug 330006 and bug 324348, and I really don't like the whole idea; see bug 330006 comment 0.
Depends on: 330006
Please address the issues in bug 330006, or I'll back this out next week.

(In reply to comment #21)
> Please address the issues in bug 330006, or I'll back this out next week.
> 

I'm ok to back it out if the issue in bug 330006 comment 4 is addressed.
I'm wondering which is really a problem for user, bug 330006 or bug 330006 comment 4 plus this bug and bug 283103.
(In reply to comment #22)
> I'm ok to back it out if the issue in bug 330006 comment 4 is addressed.

That's a modality bug, not a focus bug, and certainly shouldn't be solved by complicating low-level focus code with extraneous raises.
Also, I think this bug, as stated, is invalid.  If you don't like your window manager focus preferences, change them.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: