Skip to content

Decide how to distribute/manage CSS #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
marijnh opened this issue Oct 2, 2018 · 44 comments
Closed

Decide how to distribute/manage CSS #40

marijnh opened this issue Oct 2, 2018 · 44 comments

Comments

@marijnh
Copy link
Member

marijnh commented Oct 2, 2018

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
Copy link
Member Author

marijnh commented Oct 2, 2018

(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
Copy link
Member Author

marijnh commented Oct 2, 2018

(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
Copy link
Member Author

marijnh commented Oct 2, 2018

(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
Copy link

serapath commented Oct 2, 2018

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
Copy link

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

@guybedford
Copy link

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
Copy link

leeoniya commented Oct 2, 2018

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
Copy link
Contributor

curran commented Oct 25, 2018

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
Copy link
Contributor

curran commented Nov 2, 2018

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
Copy link

zikaari commented Dec 4, 2018

@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
Copy link

futurist commented Dec 5, 2018

I use this css-in-js lib:

https://github.com/porsager/bss

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

@marijnh
Copy link
Member Author

marijnh commented Dec 20, 2018

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.

  • Allow separate packages to come with their own styles, and make it easy for the user to include those. Since no other convenient, cross-tool way to automatically import styles seems to exist, this probably means that the styles must be embedded in the scripts somehow.

  • No extra build step needed.

  • Must be fast.

  • Allow extension/overriding specific styles from 3rd party code. For example, the autocompletion dialog plugin might come with a default style, but client code may want to change that. Since CSS modules defined in JS tend to create unpredictable class names, the classic approach of taking the class name and adding more rules for it won't work.

  • Work with shadow doms and iframes. (Surprisingly many libraries assume everything happens in the main document and shadow DOM doesn't exist.)

Nice to haves:

  • Somewhat compact HTML.

  • Don't pull in big dependencies.

  • Be independent of the global CSS namespace (so that each editor can have its own styles without juggling complicated parent class selectors, and there's no risk of colliding with classes defined by other versions of the library)

  • Support pseudo-classes, media selectors, and so on.

  • Don't be too different from classic CSS, don't introduce new learning curve.

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
Copy link
Member Author

marijnh commented Dec 20, 2018

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

@zikaari
Copy link

zikaari commented Dec 20, 2018

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
Copy link
Member Author

marijnh commented Dec 20, 2018

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

@marijnh
Copy link
Member Author

marijnh commented Dec 21, 2018

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
Copy link

@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
Copy link
Member Author

marijnh commented Dec 21, 2018

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

@guybedford
Copy link

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
Copy link

zikaari commented Dec 21, 2018

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

callstack/linaria

@guybedford
Copy link

Ahh, that looks amazing, thanks.

@marijnh
Copy link
Member Author

marijnh commented Dec 21, 2018

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

  • Module objects don't expose their class names directly. You have to mount them to get an object of class names (meaning it becomes impossible to forget to mount something, and the interface becomes a little less side-effectey)

  • Extension relationships are no longer specified when creating a module, but when mounting it. And you can specify multiple extensions. The mount logic then makes sure that the modules' rules are added to the document in the right order, so that the precedence works out (extensions get a higher precedence than the base module or other extensions that come before them).

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 marijnh mentioned this issue Jan 2, 2019
@marijnh
Copy link
Member Author

marijnh commented Jan 2, 2019

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
Copy link
Member Author

marijnh commented Jan 2, 2019

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
Copy link

keybits commented Jan 4, 2019

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
Copy link
Member Author

marijnh commented Jan 4, 2019

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
Copy link
Member Author

marijnh commented Jan 4, 2019

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

  • Fixed/static class names, to make external styling easy. In the future, 'put it in a shadow dom' will be a viable answer when someone needs two conflicting versions of the style sheets in their page. Until then, I guess iframes are the only credible workaround. It's probably a good idea to avoid clashing with classes used in CodeMirror version 2-5.

  • Our own CSS is included in scripts and injected into the root before use. Not sure yet whether I prefer objects or template strings for the content. (Leaning towards objects, since they are easier to parameterize and to indent in an editor. Conversion to string rules can be done in ~30 lines of code, and we'll need separate strings per rule anyway.)

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
Copy link

zikaari commented Jan 4, 2019

...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
Copy link
Member Author

marijnh commented Jan 4, 2019

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
Copy link

zikaari commented Jan 4, 2019

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
Copy link
Member Author

marijnh commented Jan 4, 2019

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
Copy link

zikaari commented Jan 4, 2019

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
Copy link
Member Author

marijnh commented Jan 4, 2019

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
Copy link
Member Author

marijnh commented Jan 4, 2019

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
Copy link
Member Author

marijnh commented Jan 7, 2019

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
Copy link

leeoniya commented Jan 7, 2019

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
Copy link
Member Author

marijnh commented Jan 8, 2019

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
Copy link

leeoniya commented Jan 8, 2019

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

@marijnh
Copy link
Member Author

marijnh commented Jan 11, 2019

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
Copy link
Member Author

marijnh commented Jan 16, 2019

I've pushed c987895, 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
Copy link
Member Author

marijnh commented Jan 16, 2019

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

@adrianheine
Copy link
Collaborator

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
Copy link
Member Author

marijnh commented Feb 13, 2019

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

@marijnh
Copy link
Member Author

marijnh commented Feb 13, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants