Summary: CodeMirrorEditor shouldn't force line endings to "\n". This causes files with Windows line endings (CRLF) to be converted, leaving a trailing CR on every line. This messes up text selection and copy/pasting text from resources. Steps to Reproduce: 1. Inspect https://app.cloudbilling.nl 2. Select some HTML from the editor in the Resources tab 3. Copy 4. Paste somewhere else => Pasted text doesn't match the selection This regressed in https://bugs.webkit.org/show_bug.cgi?id=153529.
<rdar://problem/39689180>
Created attachment 342354 [details] Patch
Hmm, it looks like not all type tokens are being added. Need to investigate more.
Comment on attachment 342354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342354&action=review > Source/WebInspectorUI/UserInterface/Views/TextEditor.js:123 > - if (this._codeMirror.getValue() !== newString) { > + if (this._codeMirror.getValue() !== newString) > this._codeMirror.setValue(newString); > - console.assert(this.string.length === newString.length, "A lot of our code depends on precise text offsets, so the string should remain the same."); > - } else { > + else { Hmm, I have a bad feeling about this. I'm pretty sure we make a bunch of assumptions that lines are up to \n in backend and frontend code. This feels very risky to me.
Comment on attachment 342354 [details] Patch Attachment 342354 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8094509 New failing tests: http/tests/security/local-video-source-from-remote.html
Created attachment 342358 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 342354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342354&action=review >> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:123 >> + else { > > Hmm, I have a bad feeling about this. I'm pretty sure we make a bunch of assumptions that lines are up to \n in backend and frontend code. This feels very risky to me. I'm not sure this is the case. The backend seems to be largely line-ending agnostic: Inspector protocol Debugger.Location, ScriptProfiler.ExpressionLocation, and ScriptProfiler.StackFrame use line/column, as do commands for setting breakpoints. Pause location, error/warning widgets, CodeMirrorCompletionController, and CodeMirrorTokenTrackingController also deal with lines/columns. One area where we rely on the original source and CodeMirror-transformed source being identical are the execution highlight and code coverage highlight. I'll see if this can be based on line/column as well.
Created attachment 342492 [details] [Image] Incorrect execution highlight Incorrect ranges for code coverage and execution highlight in scripts with Windows (CRLF) line endings.
Created attachment 342597 [details] WIP
(In reply to Matt Baker from comment #9) > Created attachment 342597 [details] > WIP This fixes missing type token annotations. Currently working on fixing execution and basic block highlighting.
Comment on attachment 342597 [details] WIP Attachment 342597 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8161803 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 342645 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
A few clarifying notes: This bug was caused by https://webkit.org/b/153529. Prior to this change, a) CodeMirror would automatically detect line endings b) In CodeMirror a script with Windows line endings (\r\n) would have a different length, and thus different token offsets, than the equivalent script in the backend/Esprima c) The text editor in the frontend always stripped all line separator characters from the displayed text, preventing the issue raised in this bug As a result of b) above, the assertion in TextEditor.js:123 would have failed for Windows scripts, had it existed at the time. The solution is to let CodeMirror auto-detect line endings again. Three parts of the frontend will need to be updated to use line/column based locations instead of offsets when dealing with the AST before this change can be made: 1) Execution highlighting <https://webkit.org/b/187532> 2) Type token annotations <https://webkit.org/b/187612> 3) Code coverage highlighting <https://webkit.org/b/187613>
(In reply to Matt Baker from comment #13) > A few clarifying notes: > > This bug was caused by https://webkit.org/b/153529. Prior to this change, > > a) CodeMirror would automatically detect line endings > b) In CodeMirror a script with Windows line endings (\r\n) would have a > different length, and thus different token offsets, than the equivalent > script in the backend/Esprima > c) The text editor in the frontend always stripped all line separator > characters from the displayed text, preventing the issue raised in this bug > > As a result of b) above, the assertion in TextEditor.js:123 would have > failed for Windows scripts, had it existed at the time. > > The solution is to let CodeMirror auto-detect line endings again. Three > parts of the frontend will need to be updated to use line/column based > locations instead of offsets when dealing with the AST before this change > can be made: > > 1) Execution highlighting <https://webkit.org/b/187532> > 2) Type token annotations <https://webkit.org/b/187612> > 3) Code coverage highlighting <https://webkit.org/b/187613> I think we should be explicit about lineEndings instead of using CodeMirror's auto behavior (even though we will match its current behavior I want to make sure if CodeMirror changes we still get our expectation). These sound like a good change. They should get us to work better with files containing various line endings (1) \n (2) \r\n (3) \r (4) mixed. Instead of right now where we only do a good job with (1). There is at least one additional change needed in the backend for (3) in ContentSearchUtilities lineEndings(), but that can come later since \r only is so rare, that we can basically ignore it at the moment.
Comment on attachment 342597 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=342597&action=review > Source/WebInspectorUI/ChangeLog:3 > + Web Inspector: REGRESSION (r195723): CodeMirrorEditor shouldn't force line endings to "\n" I really like the direction of this patch. I think a better name for it would be "Web Inspector: Improve support for resources with '\r' and '\r\n' line endings" because that is really what we are starting to do here, starting with the most impactful: making the editor features (selection, copy) work with such files. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js:-31 > - if (options.lineSeparator === undefined) > - options.lineSeparator = "\n"; This is where I'd prefer we be explicit with a comment, instead of defaulting to CodeMirror's default (because what if CodeMirror changes). // Default line endings match typical expected line endings for JavaScript (at least those supported by JavaScriptCore). if (options.lineSeparator === undefined) options.lineSeparator = /\r\n?|\n/; This doesn't handle the exotic line endings (U+2028 LINE SEPARATOR, U+2029 PARAGRAPH SEPARATOR) that ECMAScript specifies / Esprima supports, but given how unlikely they are its safe to not focus on those. https://tc39.github.io/ecma262/#sec-line-terminators On top of these, CSS also supports \f: https://www.w3.org/TR/css-syntax-3/#newline-diagram I don't think we need to given how unlikely that is but we could specify different line endings for CSS resources. Either way, this better handle \r, and \r\n with CSS resources as well, in case we needed to match up with line numbers we get from the backend for those resources (assuming the backend respects those line endings, and CSSTokenizer appears to). I didn't check but this is almost certainly an improvement for viewing HTML resources as well, I just didn't see where it determines line numbers for any error messages it produces.
Created attachment 344956 [details] Patch
Comment on attachment 344956 [details] Patch Clearing flags on attachment: 344956 Committed r233824: <https://trac.webkit.org/changeset/233824>
All reviewed patches have been landed. Closing bug.