Steps to reproduce: A gecko window is embedded in a dialog window. The gecko window has the focus. Tab through until an anchor dom node within the gecko window has the focus. When a specified anchor node (the last one in the DOM) has the focus, we attempt to reset the focus to a control in the parent dialog window by calling ::SetFocus(hWnd) where hWnd is the handle to a dialog control. Expected result: the embedded window loses focus to the dialog control. Actual result: the embedded window (and currently focused DOM node) maintains keyboard focus. I'm having difficulty unravelling the focus, unfocus, and blur messages to discern how/why our call to SetFocus() seems to succeed and yet gecko maintains keyboard focus.

Cc'ing blizzard.

We're doing work in this area to make what you are talking about work properly. The embeddor ( in this case your code ) will have to implement an interface that will listen for the fall off the end of the DOM tree and your app will have to grab focus. cc'ing saari + ccarlen since they might have more of an update on where they are on this.

That's what the nsIWebBrowserChromeFocus interface is for. Your chrome should implement this iface and it will be called when the user tabs beyond the contents' last element or shift-tabs before the first one. CC'ing dr because I think he's responsible for this iface and can explain better. Dan, when your nsIWebBrowserChromeFocus::Focus*Element is called, do you need to explictly unfocus the webBrowser by calling nsIWebBrowserFocus::Deactivate()? Or is that handled internally and you just have to focus on your element?

> Dan, when your nsIWebBrowserChromeFocus::Focus*Element is called, > do you need to explictly unfocus the webBrowser by calling > nsIWebBrowserFocus::Deactivate()? Or is that handled internally > and you just have to focus on your element? Hm. I don't actually know how that's supposed to work. Offhand, looking at nsDocShell::FocusAvailable, it seems like I ought to have set the out-param |aTookFocus| as we do in the non-embedded case. That might very well be my fault. If fixing that (i.e., if Focus[Next/Prev]Element succeeds, aTookFocus = true) doesn't work, I don't really have a clue. It seems like an explicit call to nsIWebBrowserFocus::Deactivate, either from our nsDocShell code or from the embeddor's focus code, would be a hack; so I doubt that would be the fix. Anyway, try the aTookFocus fix and see what happens... I'm confirming this bug.

You could call Deactivate if you want, but that isn't really what it was intended for, it was meant for notifiying the embedded browser when the top level window is active or not (at the user level). The embedding application needs to decide where to put focus when the user tabs out of the web browser in either direction. So, if you need native level focus on your foopy widget in the embedding client, you're going to have to set it since gecko doesn't know anything about it, and thus will keep any native concept of focus until you take it away. It is just notifying you, via nsIWebBrowserChromeFocus, that you probably want to take focus. Activate/Deactivate really just unlock/lock the focus controller's memory and sets its active state (which as you know is being checked by textfields and others). So if you have focus somewhere in gecko, go to another application (causing a deactivate of the gecko window and embedding client) and then come back, gecko will repond to the Activate call by placing focus where it last was.

Due to interest from the very important embedding customer I'm setting --- or Future target milestone to 0.9.1

-> saari

Okay, so I think this is a hole in the nsIWebBrowserFocus API. The short answer is, for now, when you focus something other than the embedded browser, call nsIWebBrowserFocus::Deactivate, and reactivate it later when it gets clicked in or tabbed into again. The long answer is that nsIWebBrowserFocus needs Blur and Focus methods in addition to Activate and Deactivate. Activate and Deactivate are intended for when the user level window of the embedding app is activated or deactivated. However, today we don't do anything really special with Activate and Deactivate (from the embeddor's perspective anyway), so they can be used interchangably. In the future, Activate will grey out controls on MacOS an other platforms as desired. That would be incorrect when the user level window still was active, and in this case you would call Blur instead. I wish there was a more automatic way to do this, but I can't think of a way given our convoluted XP focus system. The problem boils down to that we don't process the blurring of anything until we get the following focus event. The reason for this is that a control may not be able to accept focus at all, and so we don't want to blur the previous item until we can verify focus acceptance by the thing just clicked on... which we don't know about unil the focus event comes in.

Don't do anything with patching up the embedding clients with Deactivate just yet. I'm brainstorming with a bunch of people about making things better.

I think adding Focus/Blur to nsIWebBrowserFocus is the perfect answer. I am currently using Activate/Deactivate because, at this point, it does exactly what I would expect from Focus/Blur. Like you say, Activate/Deactivate may do more, such as dimming/enabling of controls. I can handle calling the two pairs of methods at the right time.

Conrad, the short answer is; that works fine for mac, but windows isn't soo pretty.

And neither is linux. Wow, did I just add an ugly hack to linux. I think that some platform experts need to sit down and figure out what to do here. I can represent linux/unix/gtk/X and we need some people who are intimate with win32 and the mac. Also, I think that saari, joki and hyatt should be there to talk about what gecko and JS needs from the platform folks. We can design a good XP interface from the widget side that way. We can also design interfaces for getting focus in and out for embedding as well and maybe even ( gasp! ) make mozilla-the-browser just another user of those interfaces. I think the best answer is to stop patching what we have, let the stack of cards fall and rebuild the focus system from the bottom up with embedding and gecko's needs from the start. How do people feel about that?

blizzard, that is, unfortunately, the conclusion I'm coming to. I talked with hyatt, danm, and joki about this a bit already. Hyatt should be there, but I can pretty much represent mac and win. Let's do this ASAP to get everyone on the same page and all issues out.

> that works fine for mac, but windows isn't soo pretty I don't see what the problem is on Windows - in fact, the child window which holds the embedded browser will get WM_SETFOCUS/WM_KILLFOCUS messages as well as WM_ACTIVATE messages. Being able to forward those calls into nsIWebBrowserFocus would be straightforward. So, I think that the API of nsIWebBrowserFocus should have Focus and Blur methods and, as long as the implementation is not interracting with the native focus in some unfortunate way, it would be fine.

/me is scared shitless.

Conrad, that might work if we changed nsWindow.cpp to always call the DefProc when WM_MOUSEACTIVATE was recieved so it bounced up the native windows parent chain. But then you have unparented dialogs, and I'm not sure how they work. Gotta talk that over with danm.

sr=hyatt

<tabs> + if (gLastFocusedContent) { + gLastFocusedContent->GetDocument(*getter_AddRefs(doc)); + if(doc) + doc->GetScriptGlobalObject(getter_AddRefs(ourGlobal)); + else { + mDocument->GetScriptGlobalObject(getter_AddRefs(ourGlobal)); + NS_RELEASE(gLastFocusedContent); </tabs> Are you freeing something that's assured to be null?? + char className[19]; + ::GetClassName((HWND)wParam, className, 19); is 19 (safe) magic?

I was trying to test this patch for myself on a W2K system and ended up with an unhandled exception when tabbing off the last link in an embedded window - line 2537 of nsDocShell.cpp (nsDocShell::FocusAvailable()) calls the following - nsCOMPtr<nsIWebBrowserChromeFocus> chromeFocus(do_GetInterface(mTreeOwner, &ret)); which brings us to Line 117 of nsDocShellTreeOwner.cpp (nsDocShellTreeOwner::GetInterface()) - return mOwnerWin->QueryInterface(aIID, aSink); where, unfortunately, mOwnerWin is NULL when a QueryInterface call is made upon it. I know you guys are too busy to be debugging my code, but any ideas on why mOwnerWin would be null at this point? I just need a nudge in the right direction.

Sorry, it was a minor issue with implementing the nsIEmbeddingSiteWindow interface. The patch works for me.

r=pink

okay, checked in

marking fixed

Clean up verification of dated code change bus