Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Use addCSS for pseudo-classes? #510

Open
pristas-peter opened this issue Jan 11, 2016 · 18 comments
Open

Use addCSS for pseudo-classes? #510

pristas-peter opened this issue Jan 11, 2016 · 18 comments

Comments

@pristas-peter
Copy link
Contributor

Would not it be better to support pseudo classes with style keeper just like mediaqueries and keyframes are?

@ianobermiller
Copy link
Contributor

It is an interesting thought, but goes against much of what Radium was designed to do, which is to be a pure inline solution. StyleKeeper is a necessary evil, and should only be used when absolutely necessary. There are already subtle bugs with precedence and overriding once you introduce actual CSS.

@pristas-peter
Copy link
Contributor Author

I meant to still support only the hover, fpcus and active, the purpose was to let the browser do its magic behind this and remove the delay when the site bootsraps from SSR.

@ianobermiller
Copy link
Contributor

Yes, that would make sense. I'm also unsure of the performance implications of addCSS and the StyleKeeper modifying a style sheet. The good news is that with Radium's modular architecture, it should be trivial to create a version of the resolveInteractiveStyles plugin that renders to CSS.

@ianobermiller
Copy link
Contributor

I am actually coming around to this idea, slowly, since it would fix many bugs similar to #524 and #527. I have two concerns, though:

  1. Will we be able to maintain strict overriding, such that if you have :active after :hover, it will take precedence, and so forth for media queries with nested states. If you insert the styles in the sheet in order they appear, the later ones will override the earlier ones and that will be fine. But you can imagine a case where one style is not active, so it doesn't get added, but then a later style is active, and gets added after, even though it was defined before (for example if you, for some reason, want hover to override active). Or, if we still assign classnames based on hash, an earlier component could have had the same hash (same style object) and added it to the sheet before you. Ditching deduping is fine, but how do we create a unique identifier that is still predictable for server side rendering (the answer is probably a counter in the StyleKeeper)?
  2. Do we need to garbage collect styles, and will it be performant? I'll wager 99% of apps have a finite number of styles, and so not garbage collecting will be fine. But what if you actually generate a wide range of hover styles? We could remove them from the stylesheet on unmount, and since we are not deduping due to 1, we'd be OK.

As you can see there are a bunch of issues at play here. If we can do this correctly, though, this would open up just about any CSS selector to be easily used from within radium (e.g. :disabled). It also significantly changes the original architecture of Radium (pure inline styles, which has already been compromised), but really does stay true to the dynamic runtime-only awesomeness.

@alexlande, @coopy, @rofrischmann, @knowbody, any thoughts?

@ianobermiller ianobermiller changed the title would not it be better to support pseudo classes with style keeper just like mediaqueries and keyframes are? Use addCSS for pseudo-classes? Jan 16, 2016
@robinweser
Copy link
Contributor

I've not checked all the StyleKeeper code right now, but within Look I am removing media query CSS immediately after the app was fully mounted (after window.matchMedia is available) and use the inline method again while font-faces, keyframes stick to the CSS solution as there is no pure javascript one.

The only reason why media queries get rendered into CSS is for sure: initial rendering on server-side.
As hover, focus and active do not work until the JS code is loaded completely, this might make sense as well (but I'd still remove them immediately after the app was mounted as I'd like to keep global CSS rules as small as possible).
I even heard once that there is some kind of performance issue if you're having tons of (unused) CSS selectors as e.g. when hovering an element the browser compares every single selector over and over again. Yet I do not know if this is/was or ever was a real issue and if it is true at all.

Another benefit actually is that you're able to use CSS rules to add support for all the other pseudo classes that are (not yet) supported by Radium or will never be as there is no pure javascript solution e.g. :visited.
I also use this nice tiny tool to mimic some special pseudo classes e.g -webkit-input-placeholder.
See Pseudo to CSS mixin for more information.
You could e.g. have a plugin that takes some kind of pseudo class mapping from your config and translates those rules to CSS. Then users can choose for themselves which pseudo classes they'd like to use.

Still I am not sure wether I like the idea of converting everything to CSS as it somehow brings back all the problems with CSS which are (at least one of the main) reasons we even started building CSS in JS solutions, right?

TL;TR

For polyfilling unsupported pseudo classes as well as for initial server-side rendering this is great. Also especially hover, active and focus could benefit at is uses the browsers built-in solution (and fixes many bugs). But always keep in mind where the initial idea behind CSS in JS solutions came from ;)

@coopy
Copy link
Contributor

coopy commented Jan 18, 2016

@ianobermiller I think you meant to mention @alexlande ;]

@alexlande
Copy link
Contributor

I actually think that using StyleKeeper for states is a pretty great idea. I'd even go a little (OK, a lot) further and say that I'm sort of interested in treating CSS as a render target for everything instead of actual inline styles.

If you still get the benefits of encapsulation, specificity equalization, and runtime computation without having to duplicate CSS features (and without losing any potential browser optimizations for CSS vs inline), what's the downside? :trollface:

@ianobermiller
Copy link
Contributor

I'm sort of interested in treating CSS as a render target for everything

Heresy! We should definitely prototype this :).

@robinweser
Copy link
Contributor

I've done several performance tests and experiments. Some stuff I noticed:

  • Using the current workflow (passing an object as style) does not improve performance (at least not noticeable)
  • Problems with :hover, :active, :focus are gone
  • And so is the State API (or why would you else need the event listener?)
  • Presort pseudo classes to avoid LVHA problems. *(Note: Leave the rest in the exact order or you'll have no option to order those)
  • Debugging is great, you actually got classes to manipulate one for all.
  • Pseudo classes (dynamically added) do not show up in the Chrome DevTools by default (? strange behaviour as those added with CSS files directly do)
  • Less bandwidth as hashed classNames are only added once and then can be reused
  • Styles that heavily depend on props, state or context will create new classes over and over again (if values differ) (Note: this also means you need to resolve them every time if they include dynamic values)

Also I do not know if there are performance improvements using insertRule in favor of rerendering the whole <style> every time, but with the second approach you could perhaps do some kind of carbage collection better. Also the CSS styles get cleared automatically if you route to another root Component, which might help big applications.

As I mentioned above, one blogger once posted that every CSS selector slows down the rendering process (sure we're talking about really small values).

I once built an experimental module that does some kind of diffing to only update those CSS rules, but I don't think it does any improvement at all. Still it might be helpful: Dynamic Style Sheets (CSSSheet and DOM Interface did this job)

@ianobermiller
Copy link
Contributor

Awesome write-up, thanks @rofrischmann.

And so is the State API (or why would you else need the event listener?)

Very good point about the getState API, I had forgotten about that. Users would have to manually add event listeners as a replacement, but I don't think getState is used that much anyway.

Presort pseudo classes to avoid LVHA problems. *(Note: Leave the rest in the exact order or you'll have no option to order those)

LVHA is a good point too, might mean you will still want to merge such styles before application to preserve the order, the exact opposite of what I just did with media queries.

Pseudo classes (dynamically added) do not show up in the Chrome DevTools by default (? strange behaviour as those added with CSS files directly do)

Unfortunate, maybe we can file a task to get that one fixed.

@johanneslumpe
Copy link

@ianobermiller @alexlande Have you done any prototyping yet regarding css as render target? I'm wondering if this would allow us to somehow build a babel plugin which can traverse the whole build and figure out which media queries are needed and then render them to a separate file which can be instantly rendered on the client to prevent flickering on the first render of components which mobile-first styles and media queries.

@alexlande
Copy link
Contributor

@johanneslumpe: No one has, to my knowledge. That said, media queries are already rendered to CSS in a style tag, so in theory I would expect that one could already try to make such a babel plugin, without having to render all CSS that way (I think?).

@ianobermiller
Copy link
Contributor

You could make a radium plugin to test this out. We haven't yet prototyped
it.
On Wed, Mar 16, 2016 at 9:12 AM Alex Lande notifications@github.com wrote:

@johanneslumpe https://github.com/johanneslumpe: No one has, to my
knowledge. That said, media queries are already rendered to CSS in a style
tag, so in theory I would expect that one could already try to make such a
babel plugin, without having to render all CSS that way (I think?).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#510 (comment)

@pristas-peter
Copy link
Contributor Author

Hi, I wrote the prototype as a seperate plugin 'no-inline', check this fork:
https://github.com/pristas-peter/radium

@alexlande
Copy link
Contributor

@pristas-peter: I tried testing your fork over the weekend but there were build errors and it wouldn't run. Is there something I need to do to get it in a working state?

@pristas-peter
Copy link
Contributor Author

Hi, sorry I didnt try to build it, I'm using lodash in pkugin and I havent add it to package.json. If this would not be the case let me know and I will try to run the build by myself

@pristas-peter
Copy link
Contributor Author

hi, it's fixed, there was a typo in plugins/index.js

@TigerC10
Copy link

I think this would be great for all the browser specific pseudo selectors... Like ::-moz-focus-inner. 😅

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

No branches or pull requests

7 participants