1211038 - Cmd-F does not focus the debugger search input

Status: VERIFIED FIXED

(DevTools :: Debugger, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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."

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=21015383ac41061eab25d01b546cd2713ff17b36&tochange=2bfe5435c1d1

Regressed by: Bug 1132557

Comment 4

[Nick Alexander :nalexander [he/him]]

I can confirm: no focus, does add the #.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 6

[Marijn Haverbeke]

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.

Comment 7

[Brian Grinstead [:bgrins]]

(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.

Comment 8

[Marijn Haverbeke]

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) {
```

Comment 9

[Brian Grinstead [:bgrins]]

(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!

Comment 10

[Marijn Haverbeke]

Great -- I've merged it into CodeMirror's master branch

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1211038 - Fix focus issue with debugger searchbox using CodeMirror upstream patch;r=jryans

Bug 1211038 - Fix focus issue with debugger searchbox using CodeMirror upstream patch;r=jryans

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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!

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Pulsebot]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/988b51b837a3

Comment 16

[Nazir Ahmed Sabbir [:NaSb]]

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

Comment 17

[Badiuzzaman Pranto]

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]

Comment 18

[Mohammad Maruf Islam]

As this bug is verified on both Linux (comment 16) and Windows (Comment 17), I am marking this as verified!