910183 - highlight trailing whitespace

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Every time you hit RETURN in the scratchpad, it maintains your indent level. Therefore, if you want to have a blank line between two lines of code, you often end up with trailing whitespace on the blank line, and won't realize it until you save your scratchpad and open it in your normal editor.

We should highlight this whitespace so people can catch it earlier. And maybe delete trailing whitespace on save too.

Comment 1

[Brandon Benvie [:benvie]]

Highlighting trailing whitespace is really easy to do with CodeMirror.

Comment 2

[benjamin]

Hello! First time contributor, I implemented this functionality tonight.  In it's current state it looks like http://i.imgur.com/CmOZriE.png

I'm not much of a designer, opinions?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Nice. How about using a light red background color? Most editors I've used do something similar.

Comment 4

[Anton Kovalyov (:anton)]

Thanks Ben!

IMHO, this should be an Editor thing—not just Scratchpad thing. Other clients (debugger, styleeditor and or addons) might want to use that feature as well.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Awesome work! Since you haven't uploaded a patch, I can't tell how generic your solution is, but this should definitely be fixed at the source editor level.

If you have more questions about the source editor, Anton is a good person to ask (here or on irc).

Cheers!

Comment 6

[benjamin]

Attachment: add whitespace to editor

Comment 7

[benjamin]

added a patch .. pretty new to hg so sorry if i botched that, still need to fixup the styling whenever I have a chance

Comment 8

[Anton Kovalyov (:anton)]

Comment on attachment 8336511 [details] [diff] [review]
add whitespace to editor

Review of attachment 8336511 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/sourceeditor/codemirror/activeline.js
@@ +36,5 @@
>      cm.addLineClass(line, "wrap", WRAP_CLASS);
>      cm.addLineClass(line, "background", BACK_CLASS);
>      cm.state.activeLine = line;
>    }
> +})();

Why was this file changed?

::: browser/devtools/sourceeditor/codemirror/mozilla.css
@@ +69,5 @@
>    font: message-box;
> +}
> +
> +.cm-trailingspace {
> +  trailingspacebackground-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAQAAAACCAYAAAB/qH1jAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3QUXCToH00Y1UgAAACFJREFUCNdjPMDBUc/AwNDAAAFMTAwMDA0OP34wQgX/AQBYgwYEx4f9lQAAAABJRU5ErkJggg==);

What is 'trailingspacebackground-image'?

::: browser/devtools/sourceeditor/codemirror/trailingspace.js
@@ +1,1 @@
> +(function() {

You need to use two spaces instead of tabs in this file and everywhere else.

Comment 9

[benjamin]

Attachment: bug910183_editor_trailing_whitespace

Comment 10

[benjamin]

updated patch .. I don't have windows or mac to check styling there, hoping somebody could give me a hand there (I didn't add that png to those dirs yet)

Comment 11

[Anton Kovalyov (:anton)]

Attachment: WIP 2

I took previous patch, replaced an image with a base64 string and added a test case.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 8359506 [details] [diff] [review]
WIP 2

Review of attachment 8359506 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

This is not a very common option enabled by default in editors (at least, not as far as I know). Shouldn't we provide a pref to allow the user to disable the highlighting of trailing whitespace?

Also, this is showing we care about trailing whitespaces, but the editor is 'happy' to leave such whitespace. Type |if (foo) {<Return><Return>|. We should remove the trailing whitespace when you add new empty lines.

(I wouldn't like the editor to remove any trailing whitespaces on save. That can cause undesired changes in files, on lines the user never touched.)

Comment 13

[Anton Kovalyov (:anton)]

Thanks!

> Shouldn't we provide a pref to allow the user to disable the highlighting of trailing whitespace?

Agreed, I'll make a pref (false by default) before landing. Then we can add a View item for it in bug 953206. Also filed bug 959803 for not adding trailing space on new lines.

Comment 14

[Anton Kovalyov (:anton)]

Attachment: WIP 3

Added a pref (off by default).

Comment 15

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/e0ec3c82b7c1

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e0ec3c82b7c1