1079665 - [e10s] Findbar focusContent doesn't work when a link was found

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

STR:
1. Open a web page with some links (mxr is a good example).
2. Open the findbar and start typing a search term that will cause the text of a link to be found.
3. Hit ESC to close the findbar.
4. Try to use the arrow keys to scroll.

Expected:
Content should be in focus and the arrow keys should scroll.

Actual:
Content not in focus and no scrolling occurs.

I tracked this down to this code in the content process:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Finder.jsm#232
We take the foundLink branch, but it looks like nsIFocusManager.setFocus is not doing what we want. The third branch (this._getWindow().focus()) does seem to work. If I add a call to this._getWindow().focus() before the setFocus call, everything seems to work as expected. Seems kinda hacky though.

Neil, do you have any thoughts on this?

Comment 1

[Neil Deakin]

There's some code in findbar.xml::close which calls focusContent to focus asynchronously. Then, it calls blur() on the find field. A race happens as an attempt is made to both focus the link from the content process and blur the findbar from the parent process at the same time, resulting in nothing getting focused.

It looks like the call to blur() was added by 666816, as previously the code didn't do this. I'd suggest either just removing the blur() call or explicitly focusing the browser when the findbar closes.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Marco, can you add this bug to this iteration?

Comment 3

[Marco Mucci [:MarcoM]]

Added to IT 38.3

Comment 4

[:Felipe Gomes (needinfo for replies!)]

I tried Neil's suggestion on comment 1, and it does fix the bug as described. However, there's still a difference from non-e10s, in that when the findbar closes, the link that is found doesn't end up with the focus outline ring.

So I guess that removing the blur() only ensures that the race condition doesn't happen and that the content still has focus, but the fm.setFocus(fastFind.foundLink, ..) call still doesn't work, and it requires what Bill described in comment 0 to work. (calling this._getWindow().focus() before). But it still feels a bit of a hack.

I sent a patch to tryserver with these two changes, to see what tests will think of it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3d6ed223e17


There's another difference between e10s and non-e10s, but it is not related only to the foundLink case: when the findbar closes, in non-e10s, the selection returns to normal mode, whereas in e10s it remains in active mode (green selection). I'm investigating this but I believe this can be handled in a separate bug. fwiw I tried calling setSelectionModeAndRepaint(Ci.nsISelectionController.SELECTION_NORMAL) from the findbar's close method, but that didn't work, perhaps due to timing.

Neil, what do you think of all this?

Comment 5

[Neil Deakin]

(In reply to :Felipe Gomes from comment #4)
> I tried Neil's suggestion on comment 1, and it does fix the bug as
> described. However, there's still a difference from non-e10s, in that when
> the findbar closes, the link that is found doesn't end up with the focus
> outline ring.

This is because RemoteFinder.jsm calls fastFind (and findAgain) but doesn't pass the aDrawOutline argument to draw the outline. The outline is intended to be drawn during the find, and it remains afterwards, by assigning an outline manually within _outlineLink.

The selection not displaying appears to be some regression I think as I don't remember this problem when I tested this earlier. There are other issues with selection as well, for instance trying to select something while the findbar is open doesn't display the highlight. Likely, the findbar is manipulating the selection state incorrectly. Do we have some bugs already for these?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Removing the .blur() isn't enough to get a good behavior, we need to explicitly focus the browser() so that content gets focus/activated and the outline is correctly drawn, + pressing Enter will work (will navigate to the link, instead of finding the next occurrence)

Comment 7

[Neil Deakin]

Comment on attachment 8579101 [details] [diff] [review]
Patch

Seems ok. Do we need to check if something in the findbar is focused when closing it before focusing the browser?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

Good observation, yeah we should. Turns out there's already a function for that in findbar.xml, shouldFocusContent. That discovery led to improving the patch.

So the root of the issue was that findbar.close() calls focusContent, and on the e10s version of focusContent, we send the message to the child process to be focused, but the browser in the parent was not focused. It does not affect non-e10s because focusing something from the content implies the browser to be focused as well, since it's just one hierarchy.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Aargh, I thought I had already pushed this :S

https://hg.mozilla.org/integration/fx-team/rev/690cfd1cf842

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/690cfd1cf842

Comment 11

[Luís Miguel [:quicksaver] (not reading bugmail)]

Maybe I'm reading this wrong, but I'm almost sure this line [1]
> this._finder.setSelectionModeAndRepaint(Ci.nsISelectionController.SELECTION_ON);
is supposed to be
> this._finder._fastFind.setSelectionModeAndRepaint(Ci.nsISelectionController.SELECTION_ON);

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteFinder.jsm#229

Comment 12

[Luís Miguel [:quicksaver] (not reading bugmail)]

Also in case I'm right and this needs fixing, while we're at it, there's a an extra newline in line 227
> 226       case "Finder:FastFind":
> 227 
> 228         this._finder.fastFind(data.searchString, data.linksOnly, data.drawOutline);
> 229         this._finder.setSelectionModeAndRepaint(Ci.nsISelectionController.SELECTION_ON);

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteFinder.jsm#227

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Thanks for the catch, Quicksaver. That line was accidentally merged from another patch I was using to debug things.

https://hg.mozilla.org/integration/fx-team/rev/d8decd5b3e51

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Ubuntu 13.10 32bit using latest Nightly 39.0a1 (buildID: 20150330030203): content is in focus and the arrow keys correctly scroll.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d8decd5b3e51