codemirror / codemirror5

In-browser code editor (version 5, legacy)
http://codemirror.net/5/
MIT License
26.84k stars 4.97k forks source link

losing focus in search dialog #4587

Closed JensLincke closed 7 years ago

JensLincke commented 7 years ago

We want to use code mirror ( version: 5.23.0.) as a web component. But in that scenario, the search dialog did not show up when pressing CTRL+F.

By instrumenting the code, we figured out that the dialog lost the focus and closed itself immediately after opening.

focus on dialog input field: [object HTMLInputElement] 
    at HTMLInputElement.focus 
    at CodeMirror.eval [as openDialog] 
    at dialog ( ... /code-mirror/addon/search/search.js:71:27)
    at doSearch (... /code-mirror/addon/search/search.js:157:7)
    at CodeMirror.commands.find (.../code-mirror/addon/search/search.js:243:61)

focus on the document: [object HTMLTextAreaElement]
    at HTMLTextAreaElement.focus 
    at TextareaInput.focus (/code-mirror/lib/codemirror.js:8772:25)
    at ensureFocus (/code-mirror/lib/codemirror.js:2997:45)
    at endOperation_W2 (/src/external/code-mirror/lib/codemirror.js:3693:20)
    at endOperations (/src/external/code-mirror/lib/codemirror.js:3628:7)

As a symptomatic fix in "addon/dialog/dialog.js" we delayed the focusing of the dialog so it did not lose its focus so quickly:

setTimeout(() => {
          inp.focus(); 
          if (options.closeOnBlur !== false) CodeMirror.on(inp, "blur", close);
      })
marijnh commented 7 years ago

I tried to reproduce this with the code below, but the dialog seems to work fine inside a web component. Could you get me a minimal reproducing example of your problem?

<!DOCTYPE html>
<meta charset="utf-8">

<script src="codemirror/lib/codemirror.js"></script>
<script src="codemirror/addon/search/searchcursor.js"></script>
<script src="codemirror/addon/dialog/dialog.js"></script>
<script src="codemirror/addon/search/search.js"></script>

<body>

<my-component></my-component>

<script>
document.registerElement("my-component", class extends HTMLElement {
  createdCallback() {
    this.root = this.createShadowRoot();
    this.root.innerHTML = `
      <link rel=stylesheet href="codemirror/lib/codemirror.css">
      <link rel=stylesheet href="codemirror/addon/dialog/dialog.css">`
    this.cm = new CodeMirror(this.root, {
      value: "one\ntwo\nthree four five\ntwo\n",
      extraKeys: {"Ctrl-F": "search"}
    })
  }
})
</script>

</body>
JensLincke commented 7 years ago

I used your example to reproduce the issue in our context. I could track down the issue, that our component did not store the shadowRoot as root as in the line this.root = this.createShadowRoot();

the property seemed is used by activeElt...

function activeElt() {
  var activeElement
  try {
    activeElement = document.activeElement
  } catch(e) {
    activeElement = document.body || null
  }
  while (activeElement && activeElement.root && activeElement.root.activeElement)
    { activeElement = activeElement.root.activeElement }
  return activeElement
}

which let to confusion who should has the focus when the operation ended. Maybe it would be good to also look for "shadowRoot", because this was very hard to track down.

marijnh commented 7 years ago

Ah, right, that should definitely use the .shadowRoot property, since .root just seems to be an accidental convention that happened to be used in any examples I saw so far. Attached patch changes this. Thanks for figuring that out.