Bug 160465 - Web Inspector: CodeMirror in resource content view should not show 'CR' characters
Summary: Web Inspector: CodeMirror in resource content view should not show 'CR' chara...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL: http://www.tractorsupply.com/tsc/cata...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-02 14:40 PDT by Brian Burg
Modified: 2016-09-06 16:05 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.33 KB, patch)
2016-09-05 16:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian 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 Brian 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 Brian 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 Brian Burg 2016-09-06 15:50:22 PDT
Comment on attachment 287986 [details]
Patch

r=me
Comment 10 Brian 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.