Closed Bug 1211038 Opened 5 years ago Closed 5 years ago

Cmd-F does not focus the debugger search input

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox43 unaffected, firefox44 verified)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox43 --- unaffected
firefox44 --- verified

People

(Reporter: jryans, Assigned: bgrins)

References

Details

(Keywords: regression, Whiteboard: [polish-backlog][bugday-20160111])

Attachments

(1 file)

STR:

1. Open the debugger
2. Click in the source editor (where script source is shown) so it is focused
3. Press Cmd-F (on Mac, or the appropriate shortcut for your OS) to search the current file

ER:

A "#" character is added to the search input and the input is focused so you can type there immediately.

AR:

A "#" character is still added to the input, but it is not focused, so you have to click into the input manually.

I've seen some CodeMirror bugs fly by lately, possibly related.  I think it's a recent regression.
Flags: needinfo?(bgrinstead)
Additional data from bug 1212178:

"With the source focussed, pressing Ctrl+F doesn't focus the search box. It does add # though.

Ctrl+Alt+F, Ctrl+D also don't, but Ctrl+L, Ctrl+Alt+V and Ctrl+P do."
Whiteboard: [polish-backlog]
I can confirm: no focus, does add the #.
Here is the sequence of events that happens in CM 5.7 when you press a key:

1. CM's `onKeyDown` is called
2. This method does the following early on:

  cm.curOp.focus = activeElt();

3. It calls into `handleKeyBinding`
4. For Cmd-F, `handleKeyBinding` eventually triggers the debugger to focus its custom search field via a function the debugger set on this shortcut in CM's `extraKeys`
5. After `onKeyDown` returns, `endOperation` is called via the `operation` function that wraps this event handler
6. Eventually we arrive at `endOperation_W2`, which contains:

  if (op.focus && op.focus == activeElt()) ensureFocus(op.cm);

7. Since this is true because of step 2, `ensureFocus` pulls the focus back into the editor.

Looking back at the (quite old) version of CM we used to be on (4.2), it does not seem like there was a code path that would grab focus **after** a key handler runs.

Marijn, is there an option or different approach we can take to avoid CM focusing back to itself like this?
Flags: needinfo?(marijnh)
If I follow the logic then the expression `op.focus && op.focus == activeElt()` will be false if this happens, and the editor _won't_ refocus itself. If I set up a text case where I bind a key using CodeMirror's keymap mechanism to focus an external textarea, things work as hoped -- when I press they key, the textarea gains focus, and the editor loses it.

Could it be that your focused element isn't part of the same document? That might complicate things, though my understanding is that in that case, focusing CodeMirror in the no-longer-focused document won't cause the element in the focused document to lose focus.
Flags: needinfo?(marijnh)
(In reply to Marijn Haverbeke from comment #6)
> If I follow the logic then the expression `op.focus && op.focus ==
> activeElt()` will be false if this happens, and the editor _won't_ refocus
> itself. If I set up a text case where I bind a key using CodeMirror's keymap
> mechanism to focus an external textarea, things work as hoped -- when I
> press they key, the textarea gains focus, and the editor loses it.
> 
> Could it be that your focused element isn't part of the same document? That
> might complicate things, though my understanding is that in that case,
> focusing CodeMirror in the no-longer-focused document won't cause the
> element in the focused document to lose focus.

Yes, they are in different documents.  And focusing CM in the separate document does take focus back away from the currently selected document.
Does this patch work for you?

```
diff --git a/lib/codemirror.js b/lib/codemirror.js
index ae16a87..d914807 100644
--- a/lib/codemirror.js
+++ b/lib/codemirror.js
@@ -3093,7 +3093,8 @@
 
     if (cm.state.focused && op.updateInput)
       cm.display.input.reset(op.typing);
-    if (op.focus && op.focus == activeElt()) ensureFocus(op.cm);
+    if (op.focus && op.focus == activeElt() && (!document.hasFocus || document.hasFocus()))
+      ensureFocus(op.cm);
   }
 
   function endOperation_finish(op) {
```
(In reply to Marijn Haverbeke from comment #8)
> Does this patch work for you?
> 
> ```
> diff --git a/lib/codemirror.js b/lib/codemirror.js
> index ae16a87..d914807 100644
> --- a/lib/codemirror.js
> +++ b/lib/codemirror.js
> @@ -3093,7 +3093,8 @@
>  
>      if (cm.state.focused && op.updateInput)
>        cm.display.input.reset(op.typing);
> -    if (op.focus && op.focus == activeElt()) ensureFocus(op.cm);
> +    if (op.focus && op.focus == activeElt() && (!document.hasFocus ||
> document.hasFocus()))
> +      ensureFocus(op.cm);
>    }
>  
>    function endOperation_finish(op) {
> ```

Yes, that seems to do the trick!
Great -- I've merged it into CodeMirror's master branch
Bug 1211038 - Fix focus issue with debugger searchbox using CodeMirror upstream patch;r=jryans
Attachment #8679521 - Flags: review?(jryans)
Comment on attachment 8679521 [details]
MozReview Request: Bug 1211038 - Fix focus issue with debugger searchbox using CodeMirror upstream patch;r=jryans

https://reviewboard.mozilla.org/r/23457/#review20911

Hooray, it works great!
Attachment #8679521 - Flags: review?(jryans) → review+
Assignee: nobody → bgrinstead
Looks like the infra for osx blew up on this try push, but linux is looking good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57334000da1c
Flags: needinfo?(bgrinstead)
https://hg.mozilla.org/mozilla-central/rev/988b51b837a3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Reproduced this bug by following comment 0 with Firefox Nightly 44.0a1 (2015-10-02);
(Build ID: 20151002030204) on Linux, 64 Bit 	

This Bug is now verified as fixed on Latest Firefox Dev Edition 44.0a2 (2015-12-03)

Build ID: 20151203004003
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
QA Whiteboard: [bugday-20151202]
I have reproduced this bug by following comment 0 with Firefox Nightly 44.0a1 
(Build ID: 20151002030204; User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0)

The bug is now verified as fixed on latest Beta 44.0b7 
Build ID: 20160107144911; 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0

[testday-20160108]
As this bug is verified on both Linux (comment 16) and Windows (Comment 17), I am marking this as verified!
Status: RESOLVED → VERIFIED
Whiteboard: [polish-backlog] → [polish-backlog][bugday-20160111]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.