codemirror / dev

Development repository for the CodeMirror editor project
https://codemirror.net/
Other
5.94k stars 376 forks source link

Completion text containin \r\n can cause a RangeError due to miscalculation of SelectionRange #1434

Closed bnahas closed 2 months ago

bnahas commented 2 months ago

Describe the issue

When the text of a completion contains '\r\n' rather than just '\n' for newlines, this can lead to a discrepancy between how the length of the completion is calculated. It appears that in some length calculations both the \r and \n are counted, but when the text is converted to a ChangeSpec the '\r\n' is counted as a single character/newline. This results in the resulting RangeSelection being placed beyond the end of the completion text. If the doc string is long enough, this results in a rather benign relocation of the cursor beyond the completion text, but if the doc string is to short to accommodate the resulting RangeSelection it results in a RangeError.

I've included a short piece of code that reproduces the error below. I tried to track down exactly where the discrepancy occurs within insertCompletionText but wasn't able to locate it exactly. I also explored to see if its a platform-specific issue with codemirror where the developer is expected to convert to the appropriate form of newline for the platform in completions but I did not turn up anything obvious in the documentation or forums.

Browser and platform

Linux / Chrome

Reproduction link

const original = "ab" // this throws a RangeError
// const original = "abcd" //this works because it expands the document enough to accomodate the RangeSelection
const completion = "completion\r\ntext\r\nwith\r\nnewlines"
const state = EditorState.create({
    doc: original,
    extensions: []
})
const transactionSpec = insertCompletionText(state, completion,  0, 0)
const newState = state.update(transactionSpec)
marijnh commented 2 months ago

Thanks for spotting that! Attached patch should help.

bnahas commented 2 months ago

You're welcome, Marijn! I cloned the repo and manually verified the fix (I'll wait for a release to incorporate it). Thank you for addressing it so quickly!