Bug 927424 - Implement |shouldFocusContent| for RemoteFinder.jsm From RemoteFinder.jsm:

//XXXmikedeboer-20131016: implement |shouldFocusContent| here to mitigate
//                        issues like bug 921338 and bug 921308.
shouldFocusContent: function () {
return true;
},


mikedeboer is on PTO until next Monday (Sept 8)

What is shouldFocusContent supposed to do?

The Finder.jsm has an array of listeners, and each listener needs to implement shouldFocusContent. The findbar.xml binding is one of those listeners, and registers itself. Here's how it implements shouldFocusContent:


<!--
- This handler may cancel a request to focus content by returning |false|
- explicitly.
-->
<method name="shouldFocusContent">
<body><![CDATA[
const fm = Components.classes["@mozilla.org/focus-manager;1"]
.getService(Components.interfaces.nsIFocusManager);
if (fm.focusedWindow != window)
return false;

let focusedElement = fm.focusedElement;
if (!focusedElement)
return false;

let bindingParent = document.getBindingParent(focusedElement);
if (bindingParent != this && bindingParent != this._findField)
return false;

return true;
]] ]]> </body>
</method>


So this does a few things:

  1. Ensures that the focused window is the current window
  2. Ensures that there is a focused element
  3. Makes sure that the binding parent of the focused element is either the findbar binding, or the findbar-textbox.
  4. If the above conditions are satisfied, return true - otherwise, return false.

The straight forward way to implement this doesn't work, because "focusedElement" is null in "shouldFocusContent" in findbar.xml. This should however be the findbar in some cases, when we have focus on it. I am not really sure why this happens yet, because these live in chrome and the focus manager should be able to deal with them in that case.

And he's got a patch in there. Let me de-bitrot and apply.

Interesting… this might make more sense if the focused element we expect was from content instead of chrome, but the focused element is supposed to by the findbar text input, which is definitely in the parent process…

I think my best bet might be to do something like in bug 1050447 and attach my debugger onto a breakpoint on nsFocusManager::SetFocus. I'll need a separate computer for that.

Interesting - in the e10s-case, the call to nsFocusManager::Blur clears our the selected element here:

// now adjust the actual focus, by clearing the fields in the focus manager
// and in the window.
mFocusedContent = nullptr;


And that happens before the shouldFocusContent method call in e10s, but after in non-e10s.

Finder.jsm - has a class that looks at a docshell and returns find results on it. RemoteFinder wraps a Finder so that it can message results back up to the parent.

So here's the pattern for non-remote:

The findbar closes, and focusContent in the findbar's Finder gets called. Synchronously, Finder asks all of its listeners whether we should focus content, and then performs the actual focusing.

For remote:

The findbar closes, and focusContent in the RemoteFinder gets called. This sends an async message to the frame script, and then returns to the main event loop, which removes focus on the findbar's text input. The frame script gets the message, and asks the content-process Finder to focusContent, which then communicates with the listener to see if it should focus content.

So… what are we going to do about this?

Who else uses shouldFocusContent? shouldFocusContent was first introduced in bug 921308 by mikedeboer…

So wait… the shouldFocusContent stuff… why does it need to be different for e10s? The findbar method just checks stuff in the chrome. Why does RemoteFinder need to do anything different? What are these Findbar listeners that might be registered in the content process?

evilpie agrees - we probably don't need to go down into the content process for this stuff.

So, how should I architect my solution, to use the findbar implementation of shouldFocusContent?

The findbar binding gets the current browser, reaches into its Finder, and then tells it to focusContent. This function then calls into the Finder listeners (which is the findbar binding) asking for whether or not we shouldFocusContent, and then it does the job of focusing.

In the RemoteFinder, calling focusContent… right now, it sends a message to FocusContent, which contacts the content processes Finder, asking it to focusContent.

How should it work?

findbar binding gets the remote browser, reaches into its RemoteFinder, and then should tell the child process to focus content only if none of the listeners think we shouldn't.

Wait wait wait - it looks as if setFocus in content scripts just flatout doesn't work. Is that true? Let me try to prove it. Huh… I was wrong. My test case shows that setFocus does work.

So what the hell? Well, I can re-attach my debugger and see what the difference is in this case… I'll need my other machine. *sigh*, f***in focus.

Interesting - so in the test case, isElementInActiveWindow evaluates to true, due to:

isElementInActiveWindow = (mActiveWindow && newRootWindow == mActiveWindow);

How about in the bad case? Lemme test that. Bah. I think I'm going down a rabbit hole. This is focus manager stuff, and I really shouldn't be worrying about that right now.

I've posted my patch and let's see where I get. If evilpie likes it, I'll file a bug for this focus stuff.

File bug: In e10s, finding text within an editable and hitting escape does not focus the editable
File bug: In e10s, finding text within a link and hitting escape does not focus the link

Filed bug 1064654 .