Implement |shouldFocusContent| for RemoteFinder.jsm

RESOLVED FIXED in mozilla35

Status

()

P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mikedeboer, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
mozilla35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10s+)

Details

Attachments

(3 attachments)

Comment hidden (empty)
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.
Status: NEW → ASSIGNED
Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".
tracking-e10s: --- → +
Blocks: 997462
Flags: firefox-backlog+
Whiteboard: p=5
Priority: -- → P2
Assignee: evilpies → nobody
(Assignee)

Updated

4 years ago
Assignee: nobody → mconley

Updated

4 years ago
Points: --- → 5
Flags: qe-verify?
Whiteboard: p=5
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Created attachment 8485235 [details] [diff] [review]
Allow findbar binding to cancel content focusing when using RemoteFinder in an e10s window. r=?
(Assignee)

Comment 7

4 years ago
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.
Attachment #8485235 - Flags: review?(evilpies)
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.
Attachment #8485235 - Flags: review?(evilpies) → review+
(Assignee)

Comment 9

4 years ago
Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/33359cd78e81
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/33359cd78e81
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla35

Updated

4 years ago
Iteration: --- → 35.1

Comment 12

4 years ago
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.
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.