Bug 160465

Summary: Web Inspector: CodeMirror in resource content view should not show 'CR' characters
Product: WebKit Reporter: BJ Burg <bburg>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, hi, joepeck, mattbaker, nvasilyev, simon.fraser, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: http://www.tractorsupply.com/tsc/catalog/home-garden/outdoor-power-equipment/generators?cm_re=20160725-_-FOURTH-_-Portable+Generators
Attachments:
Description Flags
Patch none

Description BJ Burg 2016-08-02 14:40:18 PDT
SUMMARY:

When pretty-printing is disabled, or for comment blocks (which aren't altered by pretty-printing), some pages show a 'CR' glyph at the end of each line. This is probably a CR/LF character. We shouldn't render this glyph as it's distracting and irrelevant for JavaScript.

STEPS TO REPRODUCE:

1. Go to the URL
2. Go to resources tab
3. Select 'foresee_surveydef.js'  or any other JS file served from 'gateway.answerscloud.com'
4. Deselect pretty printing (the {} button)

Now most of the lines have a little 'CR' glyph at the end. This is annoying.
Comment 1 Radar WebKit Bug Importer 2016-08-02 14:40:48 PDT
<rdar://problem/27666053>
Comment 2 Nikita Vasilyev 2016-08-02 15:08:22 PDT
https://github.com/WebKit/webkit/blob/c20f470adf2cf34821a0756d01a4df33b353c1e6/Source/WebInspectorUI/UserInterface/External/CodeMirror/codemirror.js#L6951

This appears to be hardcoded in CodeMirror, i.e. CodeMirror doesn't seem to
provide an option to turn it off. We may have to modify codemirror.js directly.
Comment 3 BJ Burg 2016-08-02 15:22:31 PDT
I don't think editing codemirror.js is going to be the right fix. Here are some alternatives:

1. Heuristically detect whether the line endings seem to be \r\n or \n and tell CodeMirror which line ending to assume (there's a setting for that)
2. Strip line endings from the text that we display (somewhat dangerous due to line:col interactions with JSC)
3. Tag the CR character with CSS and make it visibility:none.
Comment 4 Nikita Vasilyev 2016-08-02 15:39:33 PDT
(In reply to comment #3)
> I don't think editing codemirror.js is going to be the right fix. Here are
> some alternatives:
> 
> 1. Heuristically detect whether the line endings seem to be \r\n or \n and
> tell CodeMirror which line ending to assume (there's a setting for that)

This is what Chrome seems to do.
https://codereview.chromium.org/19540026/

> 2. Strip line endings from the text that we display (somewhat dangerous due
> to line:col interactions with JSC)
> 3. Tag the CR character with CSS and make it visibility:none.

I can't figure out how to write a valid CSS selector for this element:

<span class="cm-invalidchar" cm-text="
">␍</span>

These don't work:

.cm-invalidchar[cm-text="
"]
.cm-invalidchar[cm-text="\n"]
.cm-invalidchar[cm-text="\a"]
Comment 5 Timothy Hatcher 2016-08-05 10:17:03 PDT
We could just hide all .cm-invalidchar. This should be a setting switch once we have a Settings view.
Comment 6 Devin Rousso 2016-09-05 16:44:16 PDT
Created attachment 287986 [details]
Patch
Comment 7 BJ Burg 2016-09-06 11:40:04 PDT
Comment on attachment 287986 [details]
Patch

How does this look on misencoded files? Is there any way to differentiate CR from other invalid characters?
Comment 8 Devin Rousso 2016-09-06 12:22:21 PDT
(In reply to comment #7)
> How does this look on misencoded files? Is there any way to differentiate CR
> from other invalid characters?

Not really sure if I tested that.  Any sure-fire steps for reproducing a mis-encoded file?  What would that even look like?
Comment 9 BJ Burg 2016-09-06 15:50:22 PDT
Comment on attachment 287986 [details]
Patch

r=me
Comment 10 BJ Burg 2016-09-06 15:51:01 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > How does this look on misencoded files? Is there any way to differentiate CR
> > from other invalid characters?
> 
> Not really sure if I tested that.  Any sure-fire steps for reproducing a
> mis-encoded file?  What would that even look like?

On second thought, I've only really seen this on XHR responses that had the wrong MIME type. I think Johan fixed a bug in that area. Not sure this would really make it any worse...
Comment 11 WebKit Commit Bot 2016-09-06 16:05:25 PDT
Comment on attachment 287986 [details]
Patch

Clearing flags on attachment: 287986

Committed r205517: <http://trac.webkit.org/changeset/205517>
Comment 12 WebKit Commit Bot 2016-09-06 16:05:29 PDT
All reviewed patches have been landed.  Closing bug.