210373 - Gtkmozembed keep stealing focus

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Marco Pesenti Gritti]

Every time we call load_url the focus is moved on the web page.
In epiphany we need to keep the focus on the location entry after homepage load,
mozilla can do it (and does for about:blank).

This seem to happen because in toplevel_focus_in we SetActive (PR_TRUE);. Then
the PresShell for every new loaded page does a if (GetActive) window.focus ();

Is it necessary to SetActive also when the focus is not on gtkmozembed ?

Comment 1

[Marco Pesenti Gritti]

I'm going to attach a patch that use nsIWebBrowserFocus to activate and
deactivate on child widget focus_in/focus_out.
I'm not sure why the toplevel_focus stuff was introduced, it doesnt seem to be
necessary to me: we are getting child focus events also when changing window. I
can be missing something though.
I tested the patch for a few days and I've not been able to find problems with it.
It fixes also #127694.

The nsWebBrowser.cpp is necessary to make notebooks work correctly. When
inserting a gtkmozembed widget in the notebook, for reasons that I've not be
able to track completely, focus is given to the gtkmozembed widget (gtknotebook
is grabbing focus on the child when switching to a page, maybe it's that).
In Epiphany we grab focus to the location in that case but it doesnt work
because of a WebBrowserFocus inconsistency.
In Activate focusController->SetActive(PR_TRUE) is reached also when there is
not a PresShell, in Deactivate we return before if the PresShell doesnt exist.
(so GetActive will return PR_TRUE also when focus will be actually out of the
embed).
I'm not sure why that was necessary but I think the two functions should have a
consistent behavior.

Comment 2

[Marco Pesenti Gritti]

Attachment: Proposed fix

Comment 3

[John M]

Is this is the right fix? If it is we need to get it in ASAP.

Comment 4

[Marco Pesenti Gritti]

Could someone comment on the patch ? This is a major problem for embedders.
It's years that focus is borked in gtk embedding ... it's a shame there is
absolutely no activity on it :/

Comment 5

[Marco Pesenti Gritti]

Blizzard, I think you are right, gtkmozembed implementation is correct.
                                                                               
                                             
I looked at more in detail at how focus is implemented
in gtkmozembed and in nsIWebBrowserFocus. I think gtkmozembed
implementation is better because it keeps the focus
controller active flag in sync with actual state of the toplevel window.
                                                                               
                                             
What confused me is this part of the nsIWebBrowserFocus documentation:
                                                                               
                                             
" On non-windows platforms,
  deactivate() should also be called when focus moves from the browser
  to the embedding chrome.
"
                                                                               
                                             
Doing this cause the focus controller active flag to be FALSE when
the focus is in the embed chrome. It happen to work around the problem
but it's not quite right.
                                                                               
                                             
So I tried to track what make focus work incorrectly in gtkmozembed.
I think there are two problems.
                                                                               
                                             
1 In PresShell.cpp CheckForFocus
                                                                               
                                             
  nsCOMPtr<nsIDOMWindowInternal> focusedWindow;
  aFocusController->GetFocusedWindow(getter_AddRefs(focusedWindow));
  if (!focusedWindow) {
    // This should never really happen, but if it does, assume
    // we can focus ourself to keep the window from being keydead.
    focusedWindow = ourWin;
   return;
  }
                                                                               
                                             
This can actually happen in embed case, when no embed has ever get focus.

2 When we call deactivate on child out a Blur event is fired but it doesnt
set the FocusedWindow to NULL. This is correct in the "moving to another window
case", in fact you want to keep the focusedwindow in focus memory but is not correct
in the "move to embed chrome" case: there is no more a focused window in that case.
                                                                               
                                             
This happen because in nsEventStateManager.cpp, NS_DEACTIVATE, we:
                                                                               
                                             
      // Suppress the focus controller for the duration of the
      // de-activation.  This will cause it to remember the last
      // focused sub-window and sub-element for this top-level
      // window.
                                                                               
                                             
It works correctly for mozilla because the chrome is a DOM Window too, but put
the embed widget in an incosistent state: no DOM window is selected but
GetFocusedWindow
still return the last selected DOM Window. Note that I'm talking of the
"move to embed chrome" case _only_ here, when moving to another window it's
perfectly fine
to rember the focused Window/Element ...
                                                                               
                                             
I hope this is of any help, now I really shut up ;)

Comment 6

[Mårten Woxberg]

Come on!

It's been here for ages.

Comment 7

[John M]

Seriously, this has to be fixed. 
Blizzard/Marco have you looked at this recently?

Comment 8

[Denis Leroy]

Would it be possible to bump up the priority on this bug ? I really has become a
major nuisance in both epiphany and galeon.

thanks

Comment 9

[Vladimir Kondratyev]

Any chance to get it fixed??? I'm _very_ annoyed by this bug in Epiphany
browser. Please help me! :(

Comment 10

[Hendrik Visage]

Also quite painfull for me in galeon :(

Comment 11

[pitrou]

Same here. Very annoying in Galeon. :(

Comment 12

[Ivan Noris]

This patch used to work for me with Moz 1.6 and Galeon and Epiphany.
Now with Mozilla 1.7, it won't apply cleanly to the sources. Any reworked
patches on the way? Could be submitted here? Or some other way to fix? Thanks.

Comment 13

[Marco Pesenti Gritti]

*** Bug 127694 has been marked as a duplicate of this bug. ***

Comment 14

[Daniel Drake]

See bug 226708 for a patch that works against mozilla 1.7.7. I got it from SuSE
9.3 RPMs. At least it explains why suse users were unaffected.

Comment 15

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch; use nsIWebBrowserFocus

This patch makes gtkmozembed use nsIWebBrowerFocus on gtk2. It fixes the
security issues of other tabs stealing focus from the current tab in a
multi-tab gtkmozembed app (Epiphany), and the stealing of focus on page load in
the current tab. And also fixes bug 244664.

Since I don't have a gtk1 build, I left the gtk1 code path unchanged (just
added some #ifdef MOZ_WIDGET_GTK where the code isn't needed anymore for gtk2).

Comment 16

[Marco Pesenti Gritti]

Comment on attachment 192933 [details] [diff] [review]
updated patch; use nsIWebBrowserFocus

Ok. I agree it's safer to leave gtk1 behavior unchanged.

Comment 17

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 192933 [details] [diff] [review]
updated patch; use nsIWebBrowserFocus

This should go on branch too. It's an embedding-only fix and fixes an important
bug (with security implications when the embedding app uses gtkmozembed tabs,
analogous to ff bug 262887 [first issue]).

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

trunk:
Checking in embedding/browser/gtk/src/EmbedPrivate.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrivate.cpp,v  <--  EmbedPrivate.cpp
new revision: 1.56; previous revision: 1.55
done
Checking in embedding/browser/gtk/src/EmbedPrivate.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrivate.h,v  <--  EmbedPrivate.h
new revision: 1.28; previous revision: 1.27
done
Checking in embedding/browser/gtk/src/gtkmozembed2.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembed2.cpp,v  <--  gtkmozembed2.cpp
new revision: 1.46; previous revision: 1.45
done

MOZILLA_1_8_BRANCH:
Checking in embedding/browser/gtk/src/EmbedPrivate.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrivate.cpp,v  <--  EmbedPrivate.cpp
new revision: 1.53.2.2; previous revision: 1.53.2.1
done
Checking in embedding/browser/gtk/src/EmbedPrivate.h;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedPrivate.h,v  <--  EmbedPrivate.h
new revision: 1.25.12.2; previous revision: 1.25.12.1
done
Checking in embedding/browser/gtk/src/gtkmozembed2.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/gtkmozembed2.cpp,v  <--  gtkmozembed2.cpp
new revision: 1.43.8.3; previous revision: 1.43.8.2
done