Syntax highlighting colors for PHP are not informative

[joelrbrandt]

We have PHP mode enabled, but we don't have good color mappings for the token types.

For example, variables get the token type cm-variable-2 which is just the default text color.

[njx]

Reviewed. Moving to low priority just because PHP itself isn't a primary focus of Brackets, but I agree it's worth looking at soon. One issue might be that we might specifically want some of these colors to be the same as the default text color for JS (due to the XD design for the default theme). Is there a way we can change the colors per mode? Or, perhaps you could make the case that we want them to be different colors in JS as well. Feel free to research and make a recommendation :)

[joelrbrandt]

@njx I agree that "low priority" is probably right. I marked it as medium because a prof at Stanford is evaluating using Brackets in his intro HCI course and reported this bug to me. It'd be great to have them use it -- about 200 young, impressionable students each year. :-)

Anyway, I think it's a relatively easy fix, and I'm happy to do the lifting next sprint.

[joelrbrandt]

Removed sprint 21 milestone. I'm still going to try to do this during sprint 21, though.

[albertxing]

@joelrbrandt Are you still working on this? If not, I'd happily pick up where you left it off.

[joelrbrandt]

@albertxing I haven't started on this yet (so picking up where I left off will be easy 😃). It'd be great to get your help, and I'd be happy to review your pull request. Thanks!

[albertxing]

If we were to leave the thirdparty CodeMirror definitions alone, we would have to redefine .cm-variable-2, which is also mapped to other highlights.

Any thoughts on this?

[joelrbrandt]

@albertxing Right now .cm-variable-2 is mapped to the default text color (dark grey). A quick grep of the "modes" directory suggests that .cm-variable-2 is used in both CSS and JavaScript. So, our XD person (cc @GarthDB ) should probably weigh in before we go changing that.

In JavaScript it seems to be used for the color of object identifiers in property expressions. So, in foo.bar = baz, "foo" would be a .cm-variable-2. I personally would support having this colored something other than the default color.

In CSS it seems to be used for some literals in CSS property values. In font-family: "Trebuchet MS", Arial, Helvetica, sans-serif; the values Arial, Helvetica and sans-serif are all .cm-variable-2s. However, "Trebuchet MS" are strings. Our CSS coloring is a bit inconsistent. Some values are green (numbers, usually), some are orange(strings and literals like "none" or "center").

@GarthDB -- Any input on this?

Another option would be to enable per-mode CSS rules. @DennisKehrig -- In your language extensibility work, did you do any thinking about supporting different CSS files (for syntax highlighting) for different languages? I can imagine that we'd want, for example, .cm-variable-2 to be mapped one way in language x and another way in language y. One way to accomplish this might be to add a class that is the mode name somewhere high up in the CodeMirror portion of the DOM.

@albertxing -- Sorry that isn't a direct answer to your question. @GarthDB and @DennisKehrig are probably the right people to help out.

[albertxing]

@joelrbrandt I also thought that adding language classes somewhere in the DOM was the right way to go. This shouldn't be too hard as we already have code that inserts the language into the status bar - we just need to add this to another element.

Perhaps add an attribute like cm-lang to the .CodeMirror element (the uppermost CodeMirror element), or even add a class?

[DennisKehrig]

Another option would be to enable per-mode CSS rules. @DennisKehrig -- In your language extensibility work, did you do any thinking about supporting different CSS files (for syntax highlighting) for different languages?

Not so far, no.
I can imagine that we'd want, for example, .cm-variable-2 to be mapped one way in language x and another way in language y. One way to accomplish this might be to add a class that is the mode name somewhere high up in the CodeMirror portion of the DOM.

Sounds like a good idea. However, while this is indeed easy for the main language of a document, it will be harder for embedded code. Ideally, JavaScript in a <script> tag should be highlighted the same way as JavaScript in a separate .js files, same for CSS in a <style> tag vs. a separate .css file, while we wouldn't necessarily want CSS and JavaScript to use the same colors for .cm-variable2s (so styling .lang-html .cm-variable-2 the same way as .lang-js .cm-variable-2 isn't good enough).

I'll look into it.

[TomMalbran]

Sounds like a good idea. However, while this is indeed easy for the main language of a document, it will be harder for embedded code. Ideally, JavaScript in a <script> tag should be highlighted the same way as JavaScript in a separate .js files, same for CSS in a <style> tag vs. a separate .css file, while we wouldn't necessarily want CSS and JavaScript to use the same colors for .cm-variable2s (so styling .lang-html .cm-variable-2 the same way as .lang-js .cm-variable-2 isn't good enough).

I was thinking the same, and it gets even worst in PHP, where you could have HTML, JavaScript, and CSS internal modes, which is what we want to fix. If we want to use classes, we might even need 1 per line and we would need to parse the document to add this, so it might not be great.

Would it be ok to just change the PHP mode? We could change the functions names to def (this would require to change the clike mode too) and some of the variables or all to def. Although I know that this would change lots of other CodeMirror themes too, so we might need to check the impact of this change. Changing the functions names to def, would make it more like JavaScript, so that might not be a problem, but the variables, might be depending on the change since it can add a lot more color, which might not always be wanted.

[albertxing]

Would it be ok to just change the PHP mode? We could change the functions names to def (this would require to change the clike mode too) and some of the variables or all to def. Although I know that this would change lots of other CodeMirror themes too, so we might need to check the impact of this change. Changing the functions names to def, would make it more like JavaScript, so that might not be a problem, but the variables, might be depending on the change since it can add a lot more color, which might not always be wanted.

Correct me if I'm wrong, but isn't the PHP mode part of CodeMirror and not Brackets?

[DennisKehrig]

Experimented a little bit. In CodeMirror's runMode function there's a line style = mode.token(stream, state);. We could add the following to this:

    if (style) {
      style += " mode-" + (state.localMode ? state.localMode.name : mode.name);
    }

The resulting span would then have the class .cm-variable-2 .cm-mode-css for CSS and .cm-variable-2 .cm-mode-javascript for JavaScript, allowing different styles for the different modes.

Obviously this would increase the size of CodeMirror's output. Rendering might also be impacted because one additional CSS class has to be considered for each span.

[albertxing]

@DennisKehrig Can't seem to get it to work on mine. Which file/line did you insert into?

[DennisKehrig]

There are two places with the call to mode.token, make sure to at least alter the one in function runMode. If that doesn't work right away, please ping me again, maybe my copy is outdated.

[albertxing]

@DennisKehrig Still doesn't work. I'm in src/thirdparty/CodeMirror2/addon/runmode/runmode.js, and I've inserted after line 47. There's a for loop, and now it looks like:

for (var i = 0, e = lines.length; i < e; ++i) {
  if (i) callback("\n");
  var stream = new CodeMirror.StringStream(lines[i]);
  while (!stream.eol()) {
    var style = mode.token(stream, state);
    if (style) {
      style += " mode-" + (state.localMode ? state.localMode.name : mode.name);
    }
    callback(stream.current(), style, i, stream.start);
    stream.start = stream.pos;
  }
}

The idea seems plausible, we just need to get @marijnh to agree.

[DennisKehrig]

@albertxing Ah, alright. I was referring to src/thirdparty/CodeMirror2/lib/codemirror.js, line 4019.

[albertxing]

@DennisKehrig It's working now, thanks.

[DennisKehrig]

@albertxing Glad to hear it!

[albertxing]

@DennisKehrig There doesn't seem too big a difference with performance. It works if you add

span.cm-mode-javascript.cm-variable-2 {color: lime;}

Or the like to brackets_codemirror_override.less, after all the initial span.cm- definitions:

[DennisKehrig]

@albertxing Thanks for checking! If that is true for most modes and even more complex files (many lines, long lines, maybe a minified file) and we make sure that the other places that call mode.token get the same treatment if necessary, we could submit this to the CodeMirror repo. It should probably be configurable, too.

[albertxing]

@DennisKehrig The only other instance of var style = mode.token in codemirror.js is on line 2778 - however, the usage of this is only to return the class (or type) of code at a point.

I don't think we should change this, because a lot of functions that rely on the return would break (they would have to use substr() to retrieve the information they want. I would know, another bug I patched in CodeMirror uses the function.

So unless we have to change the code in other files like runmode.js, there's only that one instance.

[DennisKehrig]

@albertxing Could be! I hope somebody with more CodeMirror experience will pick this up and work it out.

[DennisKehrig]

@albertxing More experience than I have, I mean. :)

[albertxing]

@DennisKehrig Nah, it's fine. I don't have much experience anyway.

[TomMalbran]

Correct me if I'm wrong, but isn't the PHP mode part of CodeMirror and not brackets?

It is, and it is still a problem that one mode declares function names as definitions and another doesn't, so that might be something that could be fixed in the mode. In the same way, PHP could declare some variables as definition, maybe just those that are assigned to but not for arrays/hashmaps, since those are definitions too, and differ from JavaScript to. By doing changes like this, the modes classes/tokens became more compatible.