Closed Bug 1079665 Opened 5 years ago Closed 5 years ago

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


(Firefox :: General, defect)

34 Branch
Not set



Firefox 39
39.3 - 30 Mar
Tracking Status
e10s m6+ ---
firefox39 --- verified


(Reporter: billm, Assigned: Felipe)



(1 file, 1 obsolete file)

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.

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

Content not in focus and no scrolling occurs.

I tracked this down to this code in the content process:
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?
Flags: needinfo?(enndeakin)
Assignee: nobody → evilpies
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.
Flags: needinfo?(enndeakin)
Flags: firefox-backlog+
Points: --- → 5
Flags: qe-verify+
Marco, can you add this bug to this iteration?
Assignee: evilpies → felipc
Flags: needinfo?(mmucci)
Added to IT 38.3
Iteration: --- → 38.3 - 23 Feb
Flags: needinfo?(mmucci)
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:

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?
Flags: needinfo?(enndeakin)
(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?
Flags: needinfo?(enndeakin)
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #8579101 - Flags: review?(enndeakin)
Comment on attachment 8579101 [details] [diff] [review]

Seems ok. Do we need to check if something in the findbar is focused when closing it before focusing the browser?
Attachment #8579101 - Flags: review?(enndeakin) → review+
Attached patch Patch v2Splinter Review
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.
Attachment #8579101 - Attachment is obsolete: true
Attachment #8580169 - Flags: review?(enndeakin)
Attachment #8580169 - Flags: review?(enndeakin) → review+
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Aargh, I thought I had already pushed this :S
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
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);

Flags: needinfo?(felipc)
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);
Thanks for the catch, Quicksaver. That line was accidentally merged from another patch I was using to debug things.
Flags: needinfo?(felipc)
Verified fixed on Ubuntu 13.10 32bit using latest Nightly 39.0a1 (buildID: 20150330030203): content is in focus and the arrow keys correctly scroll.
You need to log in before you can comment on or make changes to this bug.