Cmd-F does not focus the debugger search input

VERIFIED FIXED in Firefox 44

Status

defect
VERIFIED FIXED
4 years ago
11 months ago

People

(Reporter: jryans, Assigned: bgrins)

Tracking

({regression})

unspecified
Firefox 44
Dependency tree / graph

Firefox Tracking Flags

(firefox43 unaffected, firefox44 verified)

Details

(Whiteboard: [polish-backlog][bugday-20160111])

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Assignee

Updated

4 years ago
Flags: needinfo?(bgrinstead)
Reporter

Updated

4 years ago
Duplicate of this bug: 1212178
Reporter

Comment 2

4 years ago
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."
Reporter

Updated

4 years ago
Whiteboard: [polish-backlog]
I can confirm: no focus, does add the #.
Reporter

Comment 5

4 years ago
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)

Comment 6

4 years ago
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)
Assignee

Comment 7

4 years ago
(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

4 years ago
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) {
```
Assignee

Comment 9

4 years ago
(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

4 years ago
Great -- I've merged it into CodeMirror's master branch
Assignee

Comment 11

4 years ago
Bug 1211038 - Fix focus issue with debugger searchbox using CodeMirror upstream patch;r=jryans
Attachment #8679521 - Flags: review?(jryans)
Reporter

Comment 12

4 years ago
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+
Reporter

Updated

4 years ago
Assignee: nobody → bgrinstead
Assignee

Comment 13

4 years ago
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
Assignee

Updated

4 years ago
Flags: needinfo?(bgrinstead)
https://hg.mozilla.org/mozilla-central/rev/988b51b837a3
Status: NEW → RESOLVED
Last Resolved: 4 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]

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.