927424 - Implement |shouldFocusContent| for RemoteFinder.jsm

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P2)

Comment 1

[Tom S [:evilpie]]

Attachment: debugging

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.

Comment 2

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Tom Schuster [:evilpie] from comment #1)
> Created attachment 817888 [details] [diff] [review]
> debugging
> 
> 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.

The problem is that focusContent for Finder in the non-e10s case is synchronous, and focusContent for RemoteFinder in the e10s case is asynchronous, since it sends an async message to the child to figure out whether or not it should be focusing content. By that time, the focus on the findbar text input has blurred away, which is why the focusedElement is null.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

So I'm not convinced that we have to use all of this machinery to see if we should focus the content or not.

The findbar implementation of shouldFocusContent checks that the focused window is the current window, that there is a focused element, and that the focused element has a binding parent that is either the findbar or the findbar textbox. None of those things can or should be done in the content process. In the case of an e10s window and a remote browser, if the focused element is some content, the parent process will think the focused element is the xul:browser, which is fine for this case.

So I think we should just use the findbar binding shouldFocusContent implementation here.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Specifically, what I plan on doing is the following:

When the findbar binding asks to focusContent, have the RemoteFinder poll the listeners in the parent process to see if any return false for shouldFocusContent. If not, then we send a message down to the child asking the child process Finder to focusContent. This means making the child process Finder listener always return true for shouldFocusContent.

Or, alternatively, make shouldFocusContent an optional function on a listener, where its absence implies a return of "true". That way, the RemoteFinderListener just doesn't have to implement it. I'm not married to that idea, but it seems simpler to me.

Patch coming up.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Allow findbar binding to cancel content focusing when using RemoteFinder in an e10s window. r=?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8485235 [details] [diff] [review]
Allow findbar binding to cancel content focusing when using RemoteFinder in an e10s window. r=?

What do you think of this, Tom? Probably the only questionable thing is that I make the shouldFocusContent handler optional in findbar listeners. I'm not 100% married to the idea, so if you don't like it, I'll just make the RemoteFinderListener continue to return true in all cases.

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8485235 [details] [diff] [review]
Allow findbar binding to cancel content focusing when using RemoteFinder in an e10s window. r=?

Review of attachment 8485235 [details] [diff] [review]:
-----------------------------------------------------------------

I think your approach is good, we can't give an answer anyway. I think some of the tests for Finder.jsm also don't implement shouldFocusContent.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/33359cd78e81

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33359cd78e81

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

Bugnotes:

http://people.mozilla.org/~mconley2/bugnotes/bug-927424.html

Comment 12

[Robert Kaiser]

I don't see how manual QA would be able to verify this bug, given all it says is to implement some abstract thing for a code module. If there's something end-user-visible to this fix that can be manually verified, please feel free to flip the flag to + and tell us how we can verify.