Closed Bug 210373 Opened 17 years ago Closed 14 years ago

Gtkmozembed keep stealing focus

Categories

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

x86
Linux
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mpgritti, Assigned: chpe)

References

Details

(Keywords: fixed1.8)

Attachments

(1 file, 1 obsolete file)

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 ?
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.
Attached patch Proposed fix (obsolete) — Splinter Review
Blocks: 98995
Attachment #126558 - Flags: review?(blizzard)
Is this is the right fix? If it is we need to get it in ASAP.
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 :/
Severity: normal → major
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 ;)
Attachment #126558 - Attachment is obsolete: true
Attachment #126558 - Flags: review?(blizzard)
Depends on: 226708
Come on!

It's been here for ages.
Seriously, this has to be fixed. 
Blizzard/Marco have you looked at this recently?
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
Any chance to get it fixed??? I'm _very_ annoyed by this bug in Epiphany
browser. Please help me! :(
Also quite painfull for me in galeon :(
Same here. Very annoying in Galeon. :(

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.
*** Bug 127694 has been marked as a duplicate of this bug. ***
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.
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).
Attachment #192933 - Flags: review?(mpgritti)
Comment on attachment 192933 [details] [diff] [review]
updated patch; use nsIWebBrowserFocus

Ok. I agree it's safer to leave gtk1 behavior unchanged.
Attachment #192933 - Flags: review?(mpgritti) → review+
Attachment #192933 - Flags: superreview?(roc)
Attachment #192933 - Flags: superreview?(roc) → superreview+
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]).
Attachment #192933 - Flags: approval1.8b4?
Attachment #192933 - Flags: approval1.8b4? → approval1.8b4+
Assignee: blizzard → chpe
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
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.