Add theme option addModeClass (for language-aware themes)

[marcelgerber]

Adds the theme option addModeClass, which enables the CM option addModeClass if set.
When enabled, themes can apply styles language-aware with rules like .cm-m-javascript.cm-variable.
CMs addModeClass uses innerMode for class name determination, so modes like htmlmixed, php or gfm work correctly, too.

See also #8579.
cc @MiguelCastillo

[TomMalbran]

This is great. But does it needs to be an option? Does it slow things or make is something that users might want it not active.

With this active by default we can fix issues like #2842 and fix the syntax styles for other too, so I think that we should just have it always enabled.

[marcelgerber]

I haven't tested it yet, but I think it will make CM performance worse as it applies many classes (namely to every single token).
Also, I don't think we should have mode styles in Brackets core stylesheets as they're hard to override.

[TomMalbran]

Wait, is an option added from the theme in the package.json. That seems fine then, since the theme author knows if it uses it or not. I was thinking that it was an option added for the users.

We will still need to use this on the light default theme. But I guess that we can do that on the theme extension.

[MiguelCastillo]

This should probably be a theme setting in the theme dialog, otherwise users would have to go to specific themes package.json to disable this. Similar to enable/disable scrollbars. What do you think?

[marcelgerber]

I don't think so. The setting itself is not immediately user-visible (while custom scrollbars are) and some themes just wouldn't work without this setting.
So for example, IIRC one asked for this particular feature when he wanted to create an authentic-looking Notepad++ theme. Such a theme just wouldn't be possible without this option set.

[MiguelCastillo]

Without specific metrics, I am only speculating that performance won't be noticeably impacted.

I am curious to see a theme to test this puppy out though. Particularly a theme that has mixed mode with html, javascript, and css which is why we didn't move forward with my initial attempt at supporting this. I will adjust my visual studio theme, which is what I use to test this.

@marcelgerber if you can also get another theme started to test this, I will be happy to help you where needed. It can be just a repo in your account and we can install from the URL

[marcelgerber]

Yup, I'm thinking the same about performance now, but I think it should stay opt-in, as when a theme doesn't need it, it's pretty much useless.
I've actually never done a theme myself, but I'm interested in seeing this in action, too. So I guess I'll just take a theme I like (I won't do one from ground up) and apply some language-specific styles.

[marcelgerber]

So I've just created a "Custom Lion" theme based on @larz0's Lion theme I kind of like.
@larz0 is definitely better at creating themes, but that's not what it's all about. I tried it in a htmlmixed doc and it worked ;)
Gist: https://gist.github.com/MarcelGerber/f6bddd17cbc86516083c
Screen:

You can see the HTML tags (like <style>) and the CSS tags (like h1) are styled differently.

[MiguelCastillo]

@marcelgerber Awesome it works! I haven't had a chance to test it out... Will do that a bit later today.

As far as the setting goes... Putting it in the package.json is not really making it opt-in... It is just making it more difficult if you want to enable/disable it. Opt-in means that it is disabled by default and you can opt-in to enable the functionality.

There should be a way to disable this globally and I can also see this being a setting in brackets.json. I just really don't think that enabling/disabling CodeMirror functionality should go in a package.json. If there is a bug in the CodeMirror feature, you are now stuck forcing every user to change a package.json in each theme, and unless you go through each package.json you will not know which theme is affected. -- Too cumbersome.

[TomMalbran]

I think that we should check the performance first. If there is no difference or is not noticeable, I this that this should be enabled by default on the core. Beside performance I don't see why someone would disable this. If there is an issue with codemirror, that should be fixed on the core, and not have the users change the preference to fix an issue. Not every codemirror option needs to be a preference.

There are already syntax highlight issues, in some languages with the core themes that could be fixed with this change, and people want a fix for that.

[MiguelCastillo]

@TomMalbran Yup, not every CodeMirror option needs to be a Brackets preferences, not what I meant. My point is that if we are going to provide a way to disable a CodeMirror feature, it shouldn't be done with a flag in the extension package.json.

I also don't really see a reason why users would want to disable this feature anyways. Not sure we would want to make it an option -- unless there are performance side effects.

[TomMalbran]

@MiguelCastillo I don't think that is a way for users to disable the preference. Is a way for theme authors that want to use different styles for different languages to enable it. Depending on the performance, this might or not work. If performance doesn't matter, I don't see why we need to make theme authors enable it if they want it. If performance matters, maybe users should be able to disable it.

[MiguelCastillo]

@marcelgerber have you had a chance to run any performance tests on this yet?

[redmunds]

Start by looking at the DOM generated by CM. I seriously doubt that it applies the mode class "to every single token" as stated above. The mode class only needs to be applied to the root element of the mode, so the performance impact should be negligible.

[MiguelCastillo]

@redmunds it does actually apply the style classes to roughly each tokens... Which is how CM can cleanly handle mixed mode. I will run some tests either tonight or tomorrow and report back.

[redmunds]

Yeah, I guess the free form DOM structure makes it hard to create selectors in Themes otherwise.

[marcelgerber]

So, I just managed to get a timeline of enabling the CM option addModeClass (in CEF 2171, btw):

I used codemirror.js (in the Brackets repo) as a test file, which consists of 7923 lines. But the line count actually doesn't matter much, as CM only renders the currently visible lines (in the test, there were only 433 tokens in the DOM).
I also checked scrolling performance and got a ~3% worse result.

Then, I checked jquery-2.1.1.min.js, a minimized file with 12139 tokens in the DOM:

[marcelgerber]

So, my conclusion is:
The performance hit is not too big, but there is one, and still, if a theme author doesn't need the extra class (and I think most won't need it), why have it loafing around?
But the user shouldn't be able to disable it, as that could break themes.

[MiguelCastillo]

@marcelgerber really cool! At a quick glance, it seems like there won't be a noticeable difference. Although I can't really tell if the performance hit is because of the extra processing by the CM plugin or because of extra work the rendering engine is doing to process the new CSS rules. I have a feeling it is because of the first reason since the document mode are probably only changing font colors, and I don't believe that has any layout implications.

• Is there a performance difference when document mode is on and a theme does not have any CSS rules for it?

We both agree that we need a way to disable the CM document mode setting. I understand that you want the theme (author) to determine whether or not document mode should be enabled. But I really think this is a Brackets preference. Primarily for consistency, since CM settings are exposed as Brackets preferences, which is a good thing.

Here is why I think it is a good idea to control CM settings as Brackets preferences in general. Allowing multiple extensions (including themes) to control the same CM setting will increase the likelihood of friction and complexity. I may create an extension to toggle document mode on and off. Changing the same setting by a theme will increase the likelihood of friction between my extension and the theme.

We should defer some of this conversation to other folks as well... /cc @dangoor @peterflynn

[marcelgerber]

Apparently, you've never worked with a Chrome Timeline before :)
So, both in the pie chart and the graph, yellow lines stand for JavaScript activity, purple lines stand for rendering (checking the new elements against the rules in the stylesheet, calculating dimensions) and green lines stand for painting (changing pixels).
As you can see, scripting and rendering take up the most part, while painting is not a thing.

Is there a performance difference when document mode is on and a theme does not have any CSS rules for it?

I ran the tests with a normal theme with no mode-specific styles. And that's certainly why painting is super fast.

So, the CM options are things like tabSize, spaceUnits, wordWrap or highlightMatches - all of them are immediately user-visible. Others are hardcoded in Editor.js...
Also, prefs don't save us from extensions changing an CM option - we don't do anything fancy to prevent them from doing that.

But yeah, I agree, we should really get a statement from the Brackets team...

[dangoor]

I agree with the statement made in this thread that we don't need this to be a Brackets pref. Themes that take advantage of this can turn it on, otherwise it is turned off. The one thing I wonder about that is @TomMalbran's comment that the Brackets light theme would need to have this turned on. That would make the default experience that this is on.

@marcelgerber do you know if there's any measurable impact on typing performance?

Editor performance is something we'd like to see get better. Our biggest performance issues are probably around stuff like switching files, but I'm still wary of taking a typing or scrolling performance hit by default.

[marcelgerber]

@dangoor I can't do exact measurements today as I'm pretty busy, but I think it's pretty similar to scrolling performance (that is, only a minorish hit around 3%).

[dangoor]

Odds are that the performance hit for this is probably dwarfed by the impact of other things we do, but I'm still a little wary of taking a performance hit in the default case (Brackets Light), at least to start with.

I do know that this feature is something people would like to have, so it would be good to see it land as an option for theme creators.

[dangoor]

We're wrapping up 1.1 and this looks safe to land still because it's so tiny and completely optional.

[MiguelCastillo]

@dangoor the issue with facilitating extension authors to change CM behavior directly bypassing the preferences system is that it creates a fragile system. One extension should not affect another extension without any sort of notification. The notification part is key. Changing the CM property directly will not provide a way for other code to know of the change. Setting it as a brackets preference that the theme itself can change will at least enable other code to register event handlers and react to changes.

-- But if we are all good with this, then this is really safe to merge.

[dangoor]

@MiguelCastillo This CM switch just adds an additional class to the nodes in the editor, right? Are there other extensions that will (or do) depend on the state of addModeClass?

There could be a notification on ThemeManager, if a notification is really all that's needed.

[MiguelCastillo]

Ok cool, let's merge it

[MiguelCastillo]

@dangoor we can cross that bridge when we need to. I just think it is a bad a idea to have extensions directly change properties in CM, regardless of how simple they might be.

[marcelgerber]

@dangoor No, there's currently no extension using (or relying on) addModeClass.
@MiguelCastillo Right now, every (normal) extension can change every CM option everytime they want, and many do, and that's a great thing.

[MiguelCastillo]

@marcelgerber the instance of CodeMirror is private... So, no it is not a good thing for extensions to go in potentially break the state of other extensions and Brackets.

[dangoor]

@MiguelCastillo This isn't providing unfettered access to CodeMirror, though. It's just a signal from a theme that it needs to use mode classes. The way that signal is implemented is completely in core code and the core code can access CodeMirror APIs.

[fergaldoyle]

I've been setting CodeMirror.defaults.addModeClass = true; in my Visual Studio Dark theme for some time which I recently published. No discernible performance difference compared to using the default theme.

I can't properly register the extension as a theme because once you specify "theme" in package.json nothing in main.js is executed. Would love to see addModeClass on by default or allow theme authors switch it on in the theme's packge.json. If the option is user controllable then themes built with addClassMode in mind will look terrible if the user has addModeClass turned off.

[marcelgerber]

@fergaldoyle Yeah, starting with Brackets 1.1, you can set package.json's addModeClass option - there's no longer a need for a main.js file.

[MiguelCastillo]

Too bad the option isn't on by default. Every theme developer will now need to release another version just to enable the feature. Which is one of the things I would have liked to avoid.

[marcelgerber]

@MiguelCastillo I cannot follow your line of reasoning.
Right now, not a single theme has classes like .cm-m-javascript in its stylesheet. So yeah, they need to make an update either way, to get a benefit out of the option.

[MiguelCastillo]

@marcelgerber My line of reasoning is strictly based on simplicity for users and theme authors. If a theme author is defining .cm-m-javascript, they DO WANT to take advantage of the option. So why is there an extra level of indirection? That level of indirection also adds discoverability problems of the option. I will guarantee you that at some point some theme authors will be wasting a bunch of time trying to figure out why their css isn't working, and it will be because the option isn't enabled in their package.json.

This option should be enabled by default... And sure, if you want to expose the ability to disable it via the package.json then go ahead. As you already know, package.json is not where I would put it.

[marcelgerber]

So, well, as pointed out above, addModeClass is a performance hit, even though it's not that great.
But I suppose there will only be a few themes using this option, and for those, it's totally fine to have it disabled by default (imo).
As @dangoor stated above, we don't want to affect performance by enabling an unused option.

[holdav]

Hello,

I have got installed Brackets v 1.1 and in my package.json I have got this "addModeClass": true, but if I open editor in Developer Tools JS strings has got still this simple class .cm-string instead of .cm-string-javascript. Any ideas?

Thank you.

[TomMalbran]

@holdav Each element should have 2 classes, like cm-m-javascript cm-string, and not a big one. You might need to reload Brackets too before testing.

@marcelgerber We will need to use in the core themes, since the syntax coloring for some languages is really basic, so it might be an issue.

[holdav]

Even if I reload Brackets, there is still no "cm-m-javascript cm-string" but only "cm-string" class :-( I have got Brackets version sprint 1 develop version 1.1.0-0 (master c76533c) and full package.json looks like this:

{
"title": "Quick Light",
"name": "quick-light",
"description": "A light theme that makes your code pretty and readable.",
"keywords": ["light", "theme", "quick"],
"homepage": "https://github.com/cheesypoof/QuickLight",
"license": "MIT",

"repository": {
    "type": "git",
    "url": "https://github.com/cheesypoof/QuickLight.git"
},

"bugs": {
    "url": "https://github.com/cheesypoof/QuickLight/issues"
},

"author": {
    "name": "cheesypoof",
    "url": "https://github.com/cheesypoof"
},

"theme": {
    "file": "main.less",
    "dark": false
},

"engines": {
    "brackets": ">=1.0.0"
},

"version": "1.1.0",
"addModeClass": true,

}

[sbruchmann]

Hi @holdav,

you’ll have to place addModeClass inside the theme object:

"theme": {
    "file": "main.less",
    "dark": false,
    "addModeClass": true
},

[holdav]

Even if I put addModeClass inside theme object, it still doesn't show "cm-m-javascript cm-string" :-(

[TomMalbran]

I just changed that in the package.json file of a theme and it works fine. Are you using that theme and the file you are checking is a js file?

[holdav]

I have solved it already. The mistake was that changes works only in newly opened files so I closed every files than open again and It works.

Offtopic: Why are PHP strings marked as "cm-m-clike" I supose "cm-m-php" or something like this.

Thanks

[holdav]

I have googled it already :) sorry for spam, C like :D http://codemirror.net/mode/clike/

[TomMalbran]

Is because php is based on the clike language, like many other c languages.

[marcelgerber]

Btw, I've updated the "Creating Themes" wiki page with some extra instructions: https://github.com/adobe/brackets/wiki/Creating-Themes/_compare/34f42f73ce615042d55d540652caf96d82dd6091...5c8f28d57db049898ab811fdff9ff9af7c97bc36

[MiguelCastillo]

@marcelgerber very cool. I will read it and provide some feedback later today. I think I can talk @larz0 into updating some of our brackets themes to use doctype styling :D