Closed
Bug 1079665
Opened 10 years ago
Closed 9 years ago
[e10s] Findbar focusContent doesn't work when a link was found
Categories
(Firefox :: General, defect)
Tracking
()
People
(Reporter: billm, Assigned: Felipe)
Details
Attachments
(1 file, 1 obsolete file)
4.04 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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?
Flags: needinfo?(enndeakin)
Updated•10 years ago
|
Assignee: nobody → evilpies
Comment 1•10 years ago
|
||
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)
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•9 years ago
|
Points: --- → 5
Flags: qe-verify+
Assignee | ||
Comment 2•9 years ago
|
||
Marco, can you add this bug to this iteration?
Assignee: evilpies → felipc
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Assignee | ||
Comment 4•9 years ago
|
||
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?
Flags: needinfo?(enndeakin)
Comment 5•9 years ago
|
||
(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)
Updated•9 years ago
|
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Updated•9 years ago
|
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Assignee | ||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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?
Attachment #8579101 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8580169 -
Flags: review?(enndeakin) → review+
Updated•9 years ago
|
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Assignee | ||
Comment 9•9 years ago
|
||
Aargh, I thought I had already pushed this :S https://hg.mozilla.org/integration/fx-team/rev/690cfd1cf842
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/690cfd1cf842
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Comment 11•9 years ago
|
||
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
Flags: needinfo?(felipc)
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
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
Flags: needinfo?(felipc)
Comment 14•9 years ago
|
||
Verified fixed on Ubuntu 13.10 32bit using latest Nightly 39.0a1 (buildID: 20150330030203): content is in focus and the arrow keys correctly scroll.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•