[Themes] Built-in "Thor Dark" theme won't work in minified builds

[peterflynn]

The ThorDark theme we include in core is built in a way that relies on being able to load LESS copies of some of the core Brackets default styles, which are not actually present in minified builds.

I think the way ThorDarkTheme/main.less currently works is:

• It redefines a bunch of the core LESS code's color variables
• Earlier in the file, it uses @include to load a copy of the core LESS code.
• LESS has a sort of "variable hoisting," so when the @include-ed core code earlier in the file is compiled to CSS, it uses the dark color values instead of the default values.
• The result is two copies of many of core's CSS rules -- with all the properties identical except for the color values. When the dark theme is enabled, the dark versions of all the rules override the light versions (they both have higher specificity, due to the extra theme-name prefix that gets added by ThemeManager.lessifyTheme(), and also were loaded more recent which would give them precedence even if they had identical specificity).

If that is indeed how it works, it has several problems:

• Won't work in minified builds, since the default core LESS files aren't present
• Is very inefficient, since it results in redundant copies of many of core's CSS rules -- far fewer rules would be needed to just override the few CM colors that need overriding
• Doesn't set a good example for extension themes to follow, for both of the above reasons

[MiguelCastillo]

@peterflynn that's exactly how it works. So, the idea was to enable theme authors to mixin with predefined values defined in brackets to allow for #4850. I had one primary constrain, which was that theme extensions needed to be javascript-less... Meaning, just a css/less files. And there are not that many options available to me to satisfy mixins and themes being only css/less files.

• The minification is really a blocker... We need to resolve this in order to use this approach.
• The inefficiency part. Yes it is inefficient, but I don't know how to quantify it to determine that this isn't a viable approach. We can create smaller files to provide more granular control of what's duplicated? I don't know. I would love to hear some ideas or how we can even redo this to make better :)

So, something I believe is really important about the ability to override values and do mixins is that I wanted to leverage the CSS rules that are already well tested in Brackets. This is so that people authoring themes don't have to constantly duplicate them, and worse yet, come up with stuff that breaks when Brackets changes the DOM. So, imagine a theme author having to figure out how to customize the look and of an inline widgets? We can offer the option of overriding values and Brackets control how they are consumed, or we can ask everyone to come up with their css rules, of course everything in between.

I personally have written all my themes with straight css/less without mixing in any of the stuff from Brackets. So, I can go either way. But I had to satisfy mixins and scriptless requirements.

[dangoor]

I think the use of mixins discussed in #4850 is more for extensions than themes. I looked through all of the themes yesterday, and nearly all of them are just replacing some colors... which, as you say, works in straight up CSS.

If we wrap the theme's CSS with the theme name and #editor-holder, then a theme like delkos just contains simple rules like this:

.CodeMirror {background: #262626; color: #f8f8f8;}
.CodeMirror-selected {
    background-color: #484848;
}
.CodeMirror-gutters {background: #222; border-right: 0pt;}
.CodeMirror-cursor {
    border-left: 1pt solid rgb(255, 255, 255) !important;
}

.CodeMirror-activeline .CodeMirror-gutter-elt {
    color: #eeeeee;
}

.cm-comment {color: #95927e;text-shadow: 0 0 2pt #000;}

which seems pretty clean. This also takes care of #8415.

This also, I believe, solves Peter's inefficiency concerns because the only rules in the theme are the ones that actually define that theme (as opposed to everything else that's brought in because of imports). It likely makes sense to disable the themes that aren't in use (or not even have their styles living in the DOM until they're needed). But, that's not a huge issue.

What do you think?

[MiguelCastillo]

@dangoor I personally think that seems like a really reasonable thing to do. I mean, the two lines of code to enable imports does not need to be removed from the ThemeManager, we just have to make the Thor themes we ship simply have straight CSS as you mention above. That's the pattern we are following here https://github.com/Brackets-Themes too, with the exception of RubyBlue in which we were test driving imports with.

I am not sure I follow the part of disabling themes that aren't in use. The styles are injected into the DOM, but only for the theme that's currently in use. When a theme selected, the injected style for the old theme is removed from the DOM and the theme class I add to the body tag is also cleaned up. There should really be no residue that the theme when a theme is unloaded. Is this what you are referring too?

So, to summarize we are just adding #editor-holder right here https://github.com/adobe/brackets/blob/master/src/view/ThemeManager.js#L159 and we are also converting Thor themes to straight CSS. Am I missing anything?

I think we found some more work for @larz0 :D

[dangoor]

@MiguelCastillo For some reason I was thinking that all of the themes were being loaded and then the selector was determining which theme was actually in use. Ahh. Right... I know why I was thinking that... it's the same thing that confused me a week ago. loadCurrentThemes is designed such that multiple themes could be active at once.

So, yeah, your summary looks correct.

Somewhat unrelated: I wonder if we should rename "Thor Light" to "Default" so that people trying out different themes can find their way back to Brackets' initial look?

[MiguelCastillo]

@dangoor Renaming "Thor Light" to "Default" sounds good. I can definitely make another pull request sometime today along with the couple of items coming out of this discussion.

[peterflynn]

This all sounds good to me!

Just one quick clarification @MiguelCastillo: "the two lines of code to enable imports" in ThemeManager don't actually enable imports (of core files), afaict. Any theme that relies on that will only work in dev builds, because the LESS files aren't present in production builds. (And per the original concerns in this bug, it's basically never desireable for a theme to @include into itself a whole copy of any of the core style files).

[MiguelCastillo]

@peterflynn Yup, you are correct imports are just enabled by the mere fact that I am feeding the files through the LESS processor. What the two lines do is make sure that @import url("") can also load stuff other resources. For example, it will allow fonts to be properly load when importing this https://github.com/adobe/brackets/blob/master/src/styles/brackets_fonts.less

But yeah the whole imports stuff falls all apart in production... :-/ It just didn't even cross my mind to test that, so this was a great catch! We'll take care of that though.

[MiguelCastillo]

cc @larz0 I think I will need your help converting the less imports and variable overrides to straight css.

[dangoor]

@MiguelCastillo I'll take a crack at rearranging the styles in the morning.

[MiguelCastillo]

@dangoor Just to make sure we are on the same page...

Are we talking about not allowing themes to customize things like:

1. problems-panel
2. find-in-files-results
3. status-bar

I am all good with that change, it just seems a bit too aggressive.

[peterflynn]

@MiguelCastillo See #8415 for longer discussion, but in brief:

• If themes could style any part of the UI, then any DOM or CSS change in Brackets would be a breaking API change. We'd have to be much more careful about even small UI tweaks and take time to document all those changes precisely.
• All themes that restyle the overall UI would have to be responsible for keeping up with every single new piece of UI Brackets adds each sprint. That's a lot to ask, and I think the likely result would be most themes having "holes" in them -- where users see default-styled UI or even worse, UI with broken layout that mixes default CSS & theme CSS in unexpected combinations.
• Brackets will provide the UI-wide light/dark switch, so themes can control the general appearance of the rest of the UI that way. And unlike the free-for-all approach, this is a setting that the core team would maintain, rather than making each theme author do it individually.
• Themes' CSS would be simplified because they don't need to put a .CodeMirror ... { prefix on all their selectors.

As @dangoor pointed out, we could eventually loosen this up if we see a strong need arising, for example:

• Instead of a light/dark switch, themes could provide a "theme color" for the rest of the UI, which Brackets picks up. (Would be much more work to implement, but easy to maintain and still non-fragile).
• Or we could eventually allow themes to include a second file "unstable.less" which lets them include CSS that touch the rest of the UI -- with the understanding that it could break at any time. (And we could give users a checkbox that controls whether the theme's "extras" are turned on or not).

Overall, I think it's safer to start with a constrained first iteration and then loosen it up over time as needed, compared to starting with few constraints and possibly regretting it later :-)

[MiguelCastillo]

@peterflynn I knew I had read that somewhere, but I just couldnt remember where. Thanks for pointing me in the right direction.