Closed Bug 76617 Opened 23 years ago Closed 23 years ago

some embedding apps can not get input focus

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9

People

(Reporter: masaki.katakai, Assigned: ccarlen)

References

Details

(Whiteboard: critical for mozilla 0.9, have patch, need r=,sr=,a=)

Attachments

(5 files)

I found gtkEmbed of nightly build has a problem that INPUT
can not get any input focus. I could not enter any characters,
no cursor appeared. Try the following simple HTML, start gtkEmbed
and click on INPUT and TEXTAREA. Only INPUT field has problem.
I can give a focus to TEXTAREA.

<HTML> <TITLE> FORM TEST </TITLE>

<BODY>

<FORM> <INPUT TYPE="text" NAME="TESTMSG1" SIZE=40> </FORM>
<FORM> <TEXTAREA NAME="TEST_TEXTAREA" ROWS=3 COLS=40></TEXTAREA> </FORM>

</BODY> </HTML>
Reassigning
Assignee: adamlock → blizzard
I can confirm this bug on both TestGtkEmbed and galeon.
Can it be fixed for 0.9 ? It makes embedders unusable ... THX :)
Argh!  Bad bug, bad bug!
Status: NEW → ASSIGNED
saari's got some focus changes he's going to be checking in.

Cc:ing him
This is happening on Mac (PPEmbed) too. Started happening only a few days ago.
I've seen this: When the root focus controlller is found for a text input
element, here:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#695,
the test of IsActive is always FALSE with an embedded app. With Seamonkey, it's
active when you would expect. 
Have a look to make sure that the NS_ACTIVATE event is making it down to the
event state controller.  It should be.  If it's not then we've got a problem.
Alright, found the problem. Neither gtkEmbed, gtkWidget, or PPEmbed call
nsIWebBrowserFocus::Activate or Deactivate. Now, that's nescesary. Damn it, it
would have been nice to have gotten a heads-up on embedding@netscape.com about
nsIWebBrowserFocus coming on-line and that unless a small addition was made to
your embedding app, it was going to become #@*#! useless. Here's all it takes to
fix PPEmbed. I need to get this into 0.9.  
Summary: gtkEmbed: INPUT can not get input focus → some embedding apps can not get input focus
Also, winEmbed suffers the same problem.
This is really strange because when the webBrowser gets an NS_ACTIVATE event
it's supposed to call ::Activate().  I wonder what else has broken?
OK, something really bad has changed here and I'm not sure what it is.  The way
that the focus would be grabbed in the embedding widget would look something like:

o User clicks on the text area

o Layout calls |nsIWidget::SetFocus| on the text area's native widget 

o in |SetFocus| we would grab focus to the toplevel mozilla widget and set the
virtual focus inside of the mozilla window heirachy to that window

o send an NS_GOTFOCUS and NS_ACTIVATE signal to the layout engine

o unwind

and it worked.

Now when a user clicks on the text area the layout engine never calls SetFocus.

From looking at the code in |nsWebBrowser::Activate| it appears that we have to
call nsIFocusController::Activate(PR_TRUE) on the focus controller?  I guess I
can do this when the widget is initialized before there are any focus changes to
make it "active."  The problem here is that I don't want to unconditionally give
focus to the mozilla window until a user does something that makes mozilla ask
for focus and it appears that I have to do that given the way that the focus
controller works.

I'm going to play with this more....
OK, I've got a patch now that calls the Activate() method of the focus
controller and it actually does get focus to the window correctly.  However, as
soon as you put focus somewhere else in the application or give focus to another
application you can't get focus back into the window presumably because you have
to call the Activate() method on the focus controller again.

We need to be able to get focus onto an embedded mozilla window even when the
window isn't activated and I bet that the focus controller or the code using it
in nsEventStateManager isn't allowing focus to be gived to a window when it
isn't active.  hyatt?  I remember focus checkins from you a few days ago.  Does
this sound like the intended behaviour or not?  If you need to talk about this,
let me know.
Whiteboard: critical for mozilla 0.9
setting TM to 0.9 (please forgive the trespass chris.) trying to get the 0.9
lists to make some sense.
Target Milestone: --- → mozilla0.9
I guess this regression was caused by my fix for the textbox focus bug (one of 
the top annoyances in SeaMonkey and all the embedding clients).

In order to prevent windows from coming forward when they shouldn't, a textbox 
will not allow itself to be focused (using the Focus() method) unless the 
window it is in is active.

I had no idea that some embedding clients didn't respond to this message.  
MFCEmbed worked fine with the patch.

All you have to do is ensure that the Activate method of nsWebBrowser is called 
when your window is activated.  I assumed (seeing as how the method is 
implemented and called by MFCEMbed) that the embedding clients all supported 
this method.  I guess I was wrong.

Anyway, it has always been the case that you needed to call this method when 
the window gets activated, or focus would have been horked in other ways for 
you anyway.
Can I get some r=/sr= on the patch to PPEmbed? It's really simple.
BeTarget/DontBeTarget are PowerPlant methods which can be overridden by any
object which can take the focus. These methods are called by the framework at
the right time and just forward things along to nsIWebBrowserFocus.
hyatt, maybe I'm confused.  Is the activate method related in any way to the
NS_ACTIVATE event?  It used to be that when someone called SetFocus() on a
widget that the NS_ACTIVATE signal would be sent after the NS_GOTFOCUS event. 
Now that I think about it what you have here is a lot better than what we have
had in the past assuming that what you say is true.

It's relatively simple ( although ugly as hell ) for me to call activate when
the toplevel window is activated although I have to do some testing.
The new scheme is much better than the old. Before, somehow, the focus just got
set automatically. The problem was that this can't take native widgets into
account. In the old way, it was possible for a text field within mozilla to be
focused at the same time as a native widget in the window (two blinking
cursors). Now that the focus has to be explicitly set, I can control it from the
focus management code my embedding app. The problem of a native widget and a
mozilla widget being focused at the same time is gone.
I never had that problem in gtk since the code in widget/src/gtk would handle it
for me.
OK, a few comments about this patch that I've uploaded:

I've removed the handlers for NS_ACTIVATE and NS_DEACTIVATE since we shouldn't
get those on an inner widget and the new way of doing things 

The gtk code now seems to handle focus correctly but I wasn't able to use the
nsWebBrowser |Activate| or |Deactivate| methods for the toplevel window changes
because they were doing too many things.  There still isn't a clear seperation
in that code that distinguishes between the toplevel window being "active" and
the widget itself being active.  Because of this I handle the cases myself.

There is one really suspect part of the gtk code:

+void
+EmbedPrivate::FocusOut(void)
+{
+  nsCOMPtr<nsPIDOMWindow> piWin;
+  GetPIDOMWindow(getter_AddRefs(piWin));
+
+  if (!piWin)
+    return;
+
+  piWin->Deactivate();
+
+  // but the window is still active until the toplevel gets a focus
+  // out
+  nsCOMPtr<nsIFocusController> focusController;
+  piWin->GetRootFocusController(getter_AddRefs(focusController));
+  if (focusController)
+    focusController->SetActive(PR_TRUE);
+
+}

I have to call focusController->SetActive(PR_TRUE) because if I don't then you
can't give focus back to the mozilla window after I've put it somewhere in the
application.  Am I using nsPIDOMWindow->Deactivate incorrectly?  The nsIWidget 
losing focus isn't enough to be able to turn the blinking cursor off in the
application.
The new patch just includes an api change so that people who embed mozilla out
of process ( read nautilus ) can track toplevel changes since get_toplevel
probably won't work the way I expect.
Chris, that turned out to be a lot of work for gtk. How exactly do
nsIWebBrowserFocus::Activate/Deactivate do too much? Is that at the root of the
awkwardness? I can't really review because I know next to nothing about about
gtk. Anyway, I'd really like to get this in - my app is hosed without it. CC'ing
more people.
Whiteboard: critical for mozilla 0.9 → critical for mozilla 0.9, have patch, need r=,sr=,a=
This is the big bad nasty final patch that allows you to keep track of the top
level window focus changes.  The reason that it's so complicated and that it
uses Xlib to do it is because of a bug in anything gtk < 1.2.9 ( which most
people use ) and because of gtkplug/gtksocket.

Conrad, I'm going to look at the Activate/Deactivate code in a second here to
see if I can actually use it or not.
Conrad, to answer you question the Activate() and Deactivate() methods in
nsWebBrowser have some problems related to when they try to take focus.  For
example, if you start a lot of www.google.com in an embedded window and then put
focus in another window you will end up with a flashing cursor in the text area
of the google search page even though the toplevel doesn't have focus.  That's
my biggest complaint about it.
Oh, this fixes 71224, too.
patch id=32538 has my r= in that the changes to nsWebBrowser.cpp are fine.
patch id=31760 needs r=. Simon - since it's PowerPlant, can you r= that one?
QA Contact: mdunn → depstein
Keywords: patch
saari and/or hyatt need to look at these patches.  I don't think I'm qualified
to review changes to our delicate focus system.
The gtk code is checked in on the tip and on the 0.9 branch.

Over to ccarlen since he still has an outstanding patch in this bug.
Assignee: blizzard → ccarlen
Status: ASSIGNED → NEW
*** Bug 78610 has been marked as a duplicate of this bug. ***
CC'ing scc for sr of attachment 31760 [details] [diff] [review].
Status: NEW → ASSIGNED
Can we get this in 0.9?
This problem occurrs on Win32 as well.  The patches attached do not fix the
problem on Win32.  Changing OS and Platform to ALL
OS: Linux → All
Hardware: PC → All
Upped severity 
Severity: normal → major
Upped severity 
Hold on there - I thought it worked in mfcEmbed. The remaining patch here is for
the PowerPlant embedding sample will go in when I get sr=.
I just tested with the latest version mfcembed(my bits are in sync with the 
trunk as of this morning) and it seems to work just fine.

My test involved sending a test e-mail message using mfcembed from my yahoo 
webmail account i.e. i used the mail compose form which has a combination of 
text and textarea fields.
Well, it works in mfcembed, but doesn't work in WinEmbed or Webclient.  Can 
someone please tell me why?
Ed : Looks like the code to handle the WM_ACTIVATE windows messages are missing 
in WinEmbed and WebClient.

There's the following code in mfcembed/BrowserFrm.cpp to set the focus 
explicitly:

void CBrowserView::Activate(UINT nState, CWnd* pWndOther, BOOL bMinimized) 
{
	nsCOMPtr<nsIWebBrowserFocus> focus(do_GetInterface(mWebBrowser));
	if(!focus)
		return;
    
    switch(nState) {
        case WA_ACTIVE:
        case WA_CLICKACTIVE:
            focus->Activate();
            break;
        case WA_INACTIVE:
            focus->Deactivate();
            break;
        default:
            break;
    }
}
But how does this Activate() get called?  By virtue of what interface?  This 
Activate is certainly win only thing, right?  The Activate() you picture here 
is not the same as nsIWebBrowserChrome::Activate().
See:
http://lxr.mozilla.org/seamonkey/source/embedding/tests/mfcembed/BrowserView.cpp#977

It's unfortunately different for every embedding app. In the case of mfcEmbed,
the main window gets a WM_ACTIVATE message and forwards it along to the
BrowserView::Activate() method.
Ed, I should've mentioned this but see the discussion on bug 78514. The patches
here may turn out to be a temporary measure.
Remaining patch for PPEmbed checked into trunk. While gtkWidget was critical for
the branch, I'm not sure how many are using this code. At this point, marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I've reopened 78610 since I'm still seeing this in winEmbed and webclient.  I
feel that, if I didn't need to make any special calls to allow my users to type
in forms before, why should I have to do so now.
verified code checkin for CBrowserShell.cpp, nsWindow.cpp,EmbedPrivate.cpp,
gtkmozembed2.cpp, & gtkmozarea.c. changes are in local mozilla build. 



 
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: