226708 - Solving problems with focus activate embedding api

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Marco Pesenti Gritti]

I talked a bit of this with bryner on irc and decided
to open a bug so that it can be discussed with blizzard
too (any other developers I should cc ?).
This problem is blocking the solution of several focus
bugs, I'll mark them as dependents.
                                                                               
                                             
CURRENT SITUATION
                                                                               
                                             
There is an inconsistency in the interpretation and
implementation of focus activation handling in
gtkmozembed/photon (1) and other embedding browsers
cocoa, activex, powerplant (2).
                                                                               
                                             
1 SetActive on focus controller is meant to refer
to the real toplevel window of the embedding browser.
                                                                               
                                             
Toplevel focus in: set focus controller active
Toplevel focus out: set focus controller not active
Embed widget focus in: activate the dom window
Embed widget focus out: deactivate the dom window
                                                                               
                                             
2 SetActive on focus controller is meant to refer
to the embed root window.
                                                                               
                                             
From the nsIWebBrowserFocus.idl:
                                                                               
                                             
Activate(): is a mandatory call that must be made to the browser
when the embedding application's window is activated *and* the
browser area was the last thing in focus"
                                                                               
                                             
Deactivate(): is a mandatory call that must be made to the browser
when the embedding application's window is deactivated *and* the
browser area was the last thing in focus.  On non-windows platforms,
deactivate() should also be called when focus moves from the browser
to the embedding chrome.

Activate internally does both set focus controller active and activate
the dom window.
                                                                               
                                             
In practice (ignoring the comment about windows platform, unfortunately
I have no idea of how windows focus works):
                                                                               
                                             
Toplevel focus in: do nothing
Toplevel focus out: if the browser area had focus set focus controller not
active and deactivate the dom window, otherwise do nothing
Embed widget focus in: set focus controller active and activate dom window
Embed widget focus out: set focus controller inactive and deactivate dom window
                                                                               
                                             
REQUIREMENTS
                                                                               
                                             
A PresShell should give focus to the new loaded page only
  when the focus was already in the browser area. It's up to the user
  (the embedding application) to grab focus on the browser area
  (aka activate the dom window) when they want to.
B javascript .focus() should work also when the focus is not already
  in the browser area.
                                                                               
                                             
Atm implementation 1 does not meet A, implementation 2 does not meet B.
For what it's worth A is certainly a more critical problem than B for the
final user.
                                                                               
                                             
TRYING TO SOLVE THE MESS
                                                                               
                                             
I think one of the two interpretations need to be choosen and the implementation
fixed accordingly. Unfortunately nsIWebBrowserFocus is already FROZEN.
                                                                               
                                             
1 Pro: IHMO it's the more correct implementation.
       For javascript .focus() to work correctly a way to track
       the focus of the real browser toplevel is necessary.
       Using current nsIWebBrowserFocus would mean that if the focus
       is in the browser location entry and a web page call .focus()
       on a form input field, the input field would not be focused.
  Cons: There are at least two bugs in mozilla code to solve
        to make it work well.
        (See 210373 for a detailed explanation. One of them is
         simple to fix and it's already covered by 224185,
         the other seem pretty difficult).
        No idea on how to solve the problem with the already
        frozen focus api.

2 Pro: nsIWebBrowserFocus is already based on it, mozilla code
       is more or less correct and working in regard of it,
       in general it seem easier to deal with.
  Cons: Without a way to track toplevel focus I cant think
        of any way to fix the problem with javascript .focus()
                                                                               
                                             
I hope it's clear enough, if it's not please please ask ;)

Comment 1

[Christopher Blizzard (:blizzard)]

mmm. This problem again.  I've always wanted to ditch the entire activate scheme
and just run focus through the widget code every time.  This would solve a lot
of problems here.  It would also be nice to have a two pass system for focus
changes - first part would be starting a focus change, then all the various bits
would negotiate where the focus should be and then you would "commit" the focus.
 That way you only get one blur and one focus in event.

Comment 2

[Marco Pesenti Gritti]

That sounds like the best solution on the long time.
Though, for the time being, would you favourable to move gtkmozembed to use the
nsIWebBrowserFocus activate api like other embedding implementations ?
The .focus bug is minor compared to the bugs in current implementation.

Comment 3

[Daniel Drake]

I'd like to help fix this old bug - it's still present and there hasn't really
been any activity in a while.

Whats the latest stance on the correct fix? I note there are a few different
patches around on these bugs, are they (or were they) incorrect? Or do/did they
just lack being reviewed and checked in?

If someone could provide a brief explanation on how it should be fixed, and
which areas of code would need modifying, then I'll have a stab at fixing it for
real. I'm a newcomer to the mozilla source, but I know GTK well.

Comment 4

[Daniel Drake]

Attachment: mozilla-focus-fix.patch

Here's another potential fix to add to the collection. I haven't seen it
mentioned on these bugs, although it is perhaps similar to some of the other
patches. This is from SuSE's mozilla-1.7.7-0.1 RPM.

Comment 5

[Daniel Drake]

Attachment 182889 [details] [diff] fails REQUIREMENTS.

                                             
A PresShell should give focus to the new loaded page only
  when the focus was already in the browser area. It's up to the user
  (the embedding application) to grab focus on the browser area
  (aka activate the dom window) when they want to.

It only does this the first time. If I enter www.mozilla.org in the location
bar, the focus stays in the entry box (good!). If I then click a link, the focus
goes to the mozembed and the new page is loaded (good!). If I then go back to
the location bar and go back to www.mozilla.org, mozilla.org re-loads (good!)
but the focus then jumps to the mozembed (bad!) for no apparent reason.

B javascript .focus() should work also when the focus is not already
  in the browser area.

Not quite. I'm using https://bugzilla.mozilla.org/attachment.cgi?id=116147 for
reference, and on first visit, it warns me about viewing the page over a secure
connection. When I click OK, the focus is returned to the location bar and not
into the textbox as the javascript suggests (bad!). When the page
auto-refreshes, focus is transferred to the textbox even if the focus was on
another component (good!). When the box about secure pages does not come up, the
focus thing works first time as well (good!).

So there's a bit of work to be done anyway. I'll see what I can come up with.

Comment 6

[Daniel Drake]

Attachment: my focus fix

This satisfies REQUIREMENTS and seems to work OK in a GtkMozEmbed sample app
(the one that comes included with the Gecko# documentation).

However, it breaks the real mozilla. Normally you can enter a URL, and then use
the arrow keys to scroll the page with no further intervention. This doesn't
happen anymore, you have to click on the page to get the focus (however, I'm
not really sure where the focus goes once the page is loading, its certainly
gone from the location bar..).

Before I work on fixing the above issue with mozilla, can anyone please review
this code and let me know if I'm on any wrong tracks? Thanks.

Comment 7

[Giancarlo Niccolai]

> 
> Before I work on fixing the above issue with mozilla, can anyone please review
> this code and let me know if I'm on any wrong tracks? Thanks.

I have tested your patch on the Mozilla cvs 1.7.7 release branch. It applies
correctly and improves functionality; however, there are still two problems in
the GTKMOZEMBED widget:

- If loading a flash in the mozembed, the focus may not be returned to a
location input widget correctly. You will see the cursor flashing in the input
widget, but the keys are still sent to the flash application.

-Same problem If switching to an external application and then returning to the
mozembed based app by closing the other app. That is, when the z-order of the
gtkmozembed app goes from 1 (having another app over it) to 0 (foreground). In
this case is not clear who gets the keys.

In both this cases, the focus goes to the correct widget via alt-tabbing out and
in again or resetting the X server (ctrl-alt-F1, ctrl-alt-F7). Also, focusing
out and in the location widget again (i.e. by pressing a button in the app)
works almost always.

Prior to your changes, the focus problem required an application shutdown, this
tricks didn't worked.

Comment 8

[Simon Fraser [no longer active]]

My observation from Camino is that calling
nsIWebBrowserFocus::Activate()/Deactivate() when the window is activated and
deactivated is not sufficient to update the color of the selection (which
refects the focus state). I had to add code to dispatch NS_GOTFOCUS/NS_ACTIVATE
and NS_LOSTFOCUS/NS_DEACTIVATE events to fix this.
nsIWebBrowserFocus::Activate() should be doing this for me.

Another issue is that calling nsIWebBrowserFocus::Activate() ends up calling
back into the embedder's nsIEmbeddingSiteWindow::SetFocus() implementation,
which could potentially attempt to raise the window re-entrantly. We have to
guard against these re-entrant call to prevent bad stuff from happening.

Comment 9

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

(In reply to comment #8)
> My observation from Camino is that calling
> nsIWebBrowserFocus::Activate()/Deactivate() when the window is activated and
> deactivated is not sufficient to update the color of the selection (which
> refects the focus state). I had to add code to dispatch NS_GOTFOCUS/NS_ACTIVATE
> and NS_LOSTFOCUS/NS_DEACTIVATE events to fix this.

I'm using Activate/Deactivate in Epiphany and the colour of selected text
changes from blue to grey on focus out and back to blue on focus in; I don't
post any additional events.

> nsIWebBrowserFocus::Activate() should be doing this for me.

If it's really necessary, yes it should.

Comment 10

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.