codemirror / dev

Development repository for the CodeMirror editor project
https://codemirror.net/
Other
5.52k stars 348 forks source link

Decide how to distribute/manage CSS #40

Closed marijnh closed 5 years ago

marijnh commented 5 years ago

Connecting modules to the CSS they rely on doesn't seem to be a solved problem in the JS ecosystem yet. Our plugins will often need a few lines of CSS to work. Requiring people to manually add <link> tags for each of those (which come from NPM and are probably not even in their web root by default) is a poor user experience.

I'm also not partial to schemes that inject the CSS into the page at run-time. When users are optimizing their page, they'd prefer to have control over how and when CSS is loaded, and not have that done at unpredictable moments by a script.

Who knows any existing approaches that we could take inspiration from?

marijnh commented 5 years ago

(There's the style field in package.json that, in principle, could be used to help with this, but tool support doesn't seem to be great.)

marijnh commented 5 years ago

(Also, making it easy to write themes that integrate with the various plugins would be an advantage. This is one reason current codemirror.css defines all kinds of non-core styles—themes will want to include all of those as well.)

marijnh commented 5 years ago

(I'm somewhat tempted to try and do at least some of this in code, so that themes could declare some basic colors and components pick those out and directly set them where appropriate. But then you've left the land of cascading style sheets and that is going to surprise and confuse people.)

serapath commented 5 years ago

I can highly recommend the css-in-js-approach. A library i like is called csjs and i'm often using csjs-inject while prototyping. This makes styles requirable/importable and lots of other benefits.... a lot thx to template literals

guybedford commented 5 years ago

What do you think of jss - http://cssinjs.org/?

guybedford commented 5 years ago

Static compilation is missing of course. Ideally we want CSS-in-JS patterns that can be statically analyzed with build tooling at that level of static analysis. Incremental adoption win of CSS-in-JS is huge though.

leeoniya commented 5 years ago

i use https://github.com/krasimir/absurd to compile static stylesheets from composed/modular js. it has quite good automatic merging and dedupe facilities as well. it's a bit different from other libs that generate classnames like styletron, etc.

for classname generation, i'm partial to the minimal https://github.com/cxs-css/cxs. here's a huge list of other options: https://github.com/MicheleBertoli/css-in-js

curran commented 5 years ago

FWIW I took a stab a creating an Ubuntu theme package for CodeMirror 6. It was fun! The biggest difference from CM5 is having to deal with native selections, which can be styled with ::selection.

image

The main suggestion I have after doing this is to more clearly separate the "core" CSS (required for correct layout) vs. "theme" CSS (that sets colors).

Currently, there is a rough separation of these, where the "core" CSS lives in view/style/editorview.css and the "theme" CSS lives in legacy-modes/style/codemirror.css. However, there are certain things in view/style/editorview.css that themes may want to override. It would be great to split these out into a separate file, which could be part of the default theme (or "theme base") instead. Namely the following:

.CodeMirror-focused {
  outline: 1px dotted #212121;
  outline: 5px auto -webkit-focus-ring-color;
}

.CodeMirror-content {
  caret-color: black;
} 

.CodeMirror-gutter {
  background: #f5f5f5;
  border-right: 1px solid silver;
} 

.CodeMirror-gutter-element {
  color: #999; 
}

.CodeMirror-secondary-selection {
  background-color: #3297FD;
  color: white !important;
  background-color: Highlight;
  color: HighlightText !important;
}

.CodeMirror-secondary-cursor {
  border-left: 1px solid #555;
}

Regarding distribution, my gut feeling is that well organized plain old CSS would be best. Folks seem to have no problem importing CSS files from NPM packages using Webpack import or other build tools.

curran commented 5 years ago

FWIW I had good luck using rollup-plugin-postcss to pull in CSS from CodeMirror 6.

The import looks like this:

import 'codemirror-6/codemirror.next/view/style/editorview.css';

Working demo code.

zikaari commented 5 years ago

@marijnh

When users are optimizing their page, they'd prefer to have control over how and when CSS is loaded, and not have that done at unpredictable moments by a script

What do you believe could go wrong when scripts insert style tag into the document?

I'm also developing a filetree widget and your knowledge can help before I publish it. It's React based and right now I'm using styled-components which do exactly that.

futurist commented 5 years ago

I use this css-in-js lib:

https://github.com/porsager/bss

It's elegant and embrace javascript, but a little hacky...

marijnh commented 5 years ago

I've been looking into the state of the CSS-in-JS art in the past days, as well as thinking a bit more about our requirements.

Being a packaged component, we have somewhat different requirements than what most CSS-in-JS / CSS modules libraries are written for.

Nice to haves:

I was somewhat attracted to the idea of making all styles inline at first, since it nicely avoids the problem of having to create <style> tags and CSS rules at all, but that will often create really ugly DOM, and (more importantly) makes it impossible to support things like pseudo-classes and media queries, so I think we don't want that.

The alternative is to provide a way for scripts to generate class names from descriptions, and directly use those class names in the DOM. That doesn't work very well for highly dynamic CSS, but I guess the bulk of our CSS will fit the pattern just fine, and we can fall back to the style attribute for the rest.

So then the question is how to do this with the minimal amount of overhead code, in a way that still makes it possible for client code to override styles. I wasn't able to find any existing library that does precisely what we need, but I was able to write a very small one that I think would cover it.

style-module is much like a classical CSS module library, but works with sets of classes, rather than individual classes. The idea behind that is that a plugin would define its styling as a style module containing classes for all the styleable elements it creates, and export that module. Client code can then extend this module and pass it back to the plugin as part of a configuration. If all plugins make sure they do this, and use the configured style module (with their own base module as a fallback) when styling, that should help with the overriding problem (without relying on side effects or global scopes).

The editor view itself would use a similar mechanism to allow the editor to be themed.

Style modules have to be mounted in a root (document or shadow root) before they can be used. This ensures that the module's rules have been added to the document. A view plugin should call module.mount(editorView.root) every time it draws something (where module is the configured style module), to ensure that the rules are present in the editor's root.

To style language tokens, I think it would make sense to include a function that maps token types to CSS classes in themes. That way, we can add rich information to tokens (like identifier.type.definition.local.rust) without spamming the DOM full of separate classes.

Thoughts?

marijnh commented 5 years ago

(Also, style-modules is comparatively small—about 2000 bytes uncompressed—since it does only what we need and avoids additional conveniences.)

zikaari commented 5 years ago

I was also working with styled-components for the past few days (I'm making a file tree component with react-virtualized). In a nutshell, and in my opinion, the performance is just pathetic compared to old school "non-runtime" CSS. This becomes apparent when pairing it with things like virtualized lists which render frequently (like CodeMirror lines).

I dropped CSSinJS at least for this case where performance IS the core component of the thing.

To style language tokens, I think it would make sense to include a function that maps token types to CSS classes in themes. That way, we can add rich information to tokens (like identifier.type.definition.local.rust) without spamming the DOM full of separate classes

There's a lot to learn from VSCode team, including the challenges of bridging TextMate scopes to CSS and how to make it work.

VSCode | Syntax Highlighting Optimizations > Token Matching

marijnh commented 5 years ago

Thanks for the link—that's interesting background (as are other posts on that blog)

marijnh commented 5 years ago

There is an issue with the proposed library—it works great for single-source style extensions, but for top-level editor styling, you'd need to be able combine any number of extensions (from plugins, presumably) repeatedly without leaking CSS rules, which this doesn't support (in order to ensure the correct rule order/precedence, it'd have to recreate all rules with higher precedence than a changed style).

Still trying to work out an alternative that doesn't have this issue.

guybedford commented 5 years ago

@marijnh I'd be fascinated to hear if you could come up with a simple statically-optimizable template-string approach for this space.

At risk of bikeshedding something incredibly stupid in the wrong place:

import { css } from 'static-css';

// injection handled automatically
// main feature is the enacapsulation replacement through the "class symbol"
// CSS escaping handled automatically through the template implementation
const className = css.class();
css`
.${className} {
  ...
}
` === null; // template string doesnt actually return, just injects

const html = `<div class=${className}>`;

// class name exported to support easy extension
export { className as myClassName }

where composition is handled through simple class exports.

A build tool loader (if wanted) could then optimize the above knowing how static-css works into just:

const className = '[inlined-unique-name]';
const html = `<div class=${className}>`;

// class name exported to support easy extension
export { className as myClassName }
marijnh commented 5 years ago

@guybedford A side-effect-oriented approach like that is what I'm trying to avoid.

guybedford commented 5 years ago

I see. Simply having the tagged template return an interface with mount/unmount could avoid that.

No projects I've seen have quite solved the packaging problem while leaving the door open to static optimization hence my interest in following your research here.

My main point is I tend think that simple string-based compilation with easy static optimizations will always be fastest and closest to how CSS actually works than highly structured approaches. I've been out of it a while (was involved in early discussions around CSS modules and was always a little disappointed with how it turned out). If you wanted to I'm sure you could come up with something simple while you're down in the details working on this that could likely help shift things forward.

zikaari commented 5 years ago

I think what @guybedford is describing already exists. Static CSS from CSSinJS that is.

callstack/linaria

guybedford commented 5 years ago

Ahh, that looks amazing, thanks.

marijnh commented 5 years ago

I've updated style-module with the following changes:

This made the library a little bigger (you now need to minify it to stay under 2k bytes), but should address the issue I raised above (the editor view can now gather all the style extensions from plugins, use them to extend its base styles, use the resulting classnames until its state is replaced).

I'm probably not going to be working on this much in the coming weeks, but if no issues are raised I'll take a look at integrating it in early January. (cc @adrianheine)

marijnh commented 5 years ago

Two more issues that I've ran into:

Styling by casual users

People expect to be able to open devtools, inspect an element, take its class name, and add a rule to their .css file to target it. With an approach like this, they'll just see generated class names that aren't even stable, so their familiar path will be blocked.

Of course, part of the point of all this is to disconnect styling from the global namespace so that it works predictably with JS modules (i.e. it becomes safe to import two versions of CodeMirror alongside each other). Styling in CSS files is fundamentally at odds with that (it is deeply tied to that global namespace), so you probably can't have both.

You could do a thing where you generate predictable names (say, the style name prefixed by something like cm-) until the actual situation occurs where you have two instances of the library in a given document, and you actually need to add a disambiguating counter to the end of the name. That would kind of give you the best of both worlds—in typical use cases, naive styling just works, and in the corner case where we need the uniqueness guarantees, the library itself doesn't break down.

But I feel that'd give a kind of confusing signal—if there's this infrastructure for modular styling, but you can just ignore it and hard-code strings, and that'll work just fine in 99% of situations, it's hard to sell the reasons for using the infrastructure properly. It also moves the (implied) public interface from 'only the stuff we export' to 'everything you can discover in the devtools', which might not be desirable.

So that's a dilemma—do we complicate life for casual web developers and designers by locking the styling to scripts, or do we provide them with a solution that usually works but undermines the modularity that we're trying to create with this system in the first place?

'Gated' selectors

A type of selectors that I expect will be useful to support is the one where a rule for the currently defined style only applies when some parent has another class (which indicates something like the editor being right-to-left, focused, using a dark theme, etc). The current sketch for the solution doesn't support this.

To add support for it we'd have to somehow make it possible to pull classes defined in other modules into a selector. Since the CSS class names are generated only when the modules are mounted, you can't reduce them to a string in advance, you need to somehow store both a reference to the module and the name of the style in that module. Since selectors are currently the property names in a notation of nested objects, storing object identities in them is hard, and you'd probably end up with some awkward out-of-band way to declare names for modules that you want to reference in the selector strings.

And awkward (/complicated) is bad, since we have to sell this to users as being an actual improvement over plain-old-CSS.

marijnh commented 5 years ago

This patch adds support for parent class selectors to style-module in a relatively usable way. But yeah, by reinventing styling we're running into more and more required complexity here.

keybits commented 5 years ago

Probably not a dependency you want to introduce, but sharing in case this approach is of interest (works very well for me and I've found the learning curve for 'styling by casual users' to be shallow):

Suggest to read this first: https://adamwathan.me/css-utility-classes-and-separation-of-concerns/

Then this: https://www.mikecr.it/ramblings/functional-css/

Then Tailwind docs: https://tailwindcss.com

marijnh commented 5 years ago

Functional CSS makes the DOM directly reference the style. This works fine if you're in full control of the website, but a reusable component that wants to allow outside styling can't do that.

marijnh commented 5 years ago

I guess I'm now leaning towards a rather different approach:

So then extending or overriding a style by 3rd-party code is done in plain-old-CSS, and the only real problem our CSS-in-JS shim solves is automatic inclusion of the styles distributed with the library (and maybe some conveniences like automating the use of a shared prefix all through the code).

zikaari commented 5 years ago

...when someone needs two conflicting versions of the style sheets in their page

Won't using a unique classname in the wrapper get the job done without using shadow DOM and/or iframes?

// app.js

cm1.getWrapperElement().classList.add('my_cm_1')
cm2.getWrapperElement().classList.add('my_cm_2')
<!-- rendered DOM -->

<div class="codemirror my_cm_1 monokai zxC012n"
  <div class="gutters nCv127f"></div>
</div>

<div class="codemirror my_cm_2 monokai zxC012n"
  <div class="gutters nCv127f"></div>
</div>

<!-- Here `zxC012n` and `nCv127f` represent classnames auto-generated by codemirror -->
// styles.sass

.codemirror
  // styles common to all cm instances
  .gutters
    font-family: 'Consolas', monospace
  &.my_cm_1
    // styles unique to cm1
    .gutters
      background: red
  &.my_cm_2
    // styles unique to cm2
    .gutters
      background: blue
marijnh commented 5 years ago

Won't using a unique classname in the wrapper get the job done without using shadow DOM and/or iframes?

For per-editor styling, yes, but what I was trying to address is having multiple versions of the library, with incompatible styles, loaded.

zikaari commented 5 years ago

Perhaps I'm not seeing your vision, but if you're talking about internal required styling and not user styling, aren't auto-generated classnames (nCv127f etc.) supposed to isolate multiple contexts?

What if the algo that generates those "hashed" classnames can take in cm version number as "salt"?

marijnh commented 5 years ago

Yes, they are, but if we need non-generated class names for user styling, it seems reasonable to use those for built-in styling as well. Though I guess maybe a hybrid approach could make sense---attach built-in styles to generated names, and also attach static names for external styling.

zikaari commented 5 years ago

That sounds neat. I guess having version number as classname to wrapper element?


<div class="codemirror v6_1_2 my_cm_1 monokai zxC012n"
  <div class="gutters nCv127f"></div>
</div>

<div class="codemirror v7_0_1 my_cm_2 monokai axB210c"
  <div class="gutters dcv247b"></div>
</div>
marijnh commented 5 years ago

I don't think we'd need version classes---all the version-specific internals would be applied with anonymous/generated classes. Themes (or plugins in general) would be able to add classes to the top-level element and to tokens, and would usually use anonymous classes for that. But site-specific styling can just target the static class names and ignore the CSS-in-JS magic.

marijnh commented 5 years ago

Does anyone know whether there's a downside to using custom element names (not Custom Elements, just non-built-in tag names) in 2018 anymore? I could see tag names like <codemirror-editor> and <codemirror-line> being a useful replacement for <div> elements with fixed class names, being targetable directly by CSS, with the added advantage that we don't get issues in the editor when someone adds weird CSS targeting div elements.

marijnh commented 5 years ago

Turns out that custom tag names are mostly unproblematic, and look so much nicer when you are inspecting the DOM. But, unfortunately, CSS rules targeted on tag names are less specific than those targeted on class names, so using these as CSS targets is very unergonomic—by default, your rules will be overridden by those from the generated class names, and you have to mess with !important or add extra selector junk to work around that. Given that, I guess we'll stick with classes.

leeoniya commented 5 years ago

can i suggest to use shorter classnames in general. eg cm-line rather than codemirror-line. currently everything is longform and harder to grok:

CodeMirror-gutter CodeMirror-linenumbers could be cm-gutter cm-linenumbers

marijnh commented 5 years ago

I don't think the cost of long classnames is very big—looking at the editor's DOM isn't something that you have to do in routine activities, and even there, readable might beat short. Browsers probably intern strings seen in the DOM, so I don't think there's even a measurable memory cost.

leeoniya commented 5 years ago

it's more a visual optimization (and smaller stylesheets perhaps). i don't think it will affect perf at all.

marijnh commented 5 years ago

I've started on an implementation in this branch, but I'm putting it on hold as I work on #64, because how theming and such will work depends heavily on what the view extension system will end up looking like.

marijnh commented 5 years ago

I've pushed c9878954f08aeb, which seems to work well. What remains is providing a way for extensions to add CSS classes (and other attributes) to the editor, so that themes can add their own context classes.

I did have to add a thing where, when creating an editor, you have to declare the root (document or shadow root) that it'll live in, so that can immediately mount the styles. Otherwise we got into issues with layout trashing due to the styles being loaded too late (the editor doesn't know when it's added to the DOM), which, with our already delicate DOM update cycle, led to all kinds of issues.

marijnh commented 5 years ago

(Also, the library that builds the CSS rules ended up here, because style-module was already taken on npm)

adrianheine commented 5 years ago

Just wanted to say that the solution you went with has really nice HTML/CSS output, is extremely small and seems to avoid serious problems I can think of … so, yay!

marijnh commented 5 years ago

Thanks! Took me long enough to come up with it :/

marijnh commented 5 years ago

I think this can actually be closed now—the machinery to add classes to the top and content DOM has also landed. We'll open separate issues for the further complications that'll no doubt show up when we start creating actual themes.