Skip to content
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

Export CSSOM classes #201

Open
devingfx opened this issue Apr 24, 2023 · 15 comments
Open

Export CSSOM classes #201

devingfx opened this issue Apr 24, 2023 · 15 comments

Comments

@devingfx
Copy link

It can be usefull to be able to get and work with CSS classes, as it's already used in the library, why not make it accessible in root module and window like the rest of classes?

import {parse} from 'cssom';

export * from 'cssom'
@devingfx
Copy link
Author

devingfx commented Apr 24, 2023

PS: as a linkedom user in deno, I don't know how to use CSSStyleRule, etc... appart from importing a fresh new module that will conflict with the one used bt linkedom:

import { CSSStyleRule } from 'npm:cssom'
document.querySelector('style').sheet.cssRules[0].contructor === CSSStyleRule  
// false

I would need:

import { CSSStyleRule } from 'linkedom'
// or
import { cssom } from 'linkedom'
const { CSSStyleRule } = cssom
document.querySelector('style').sheet.cssRules[0].contructor === CSSStyleRule  
// true

@WebReflection
Copy link
Owner

WebReflection commented Apr 24, 2023

this seems a reasonable question, I just need to be sure that module doesn't export conflicting names for the module. If you fancy a PR, please go ahead! (source of truth being the ./esm/ folder or any esm export in the package.json)

@devingfx
Copy link
Author

After investing code, there is a conflict with CSSStyleDeclaration, you have one, and CSSOM have one other... without the same role nor apis ! :(

Your's is used to be synced with the ownerElement.style , and the one from CSSOM is the more generic one used to store also CSSRule.style and don't sync to anything.

Your's also is iterable and the other not...

Both are not fully compliant with W3C (like concerning constructed StyleSheet's .replaceSync ... )

So the thing is that you get 2 really different classes that should both be the same, depending on where you get it:

// el is a <style> with some css
assert( el.style.constructor == el.sheet.cssRules[0].style.contructor )
// false !

@devingfx
Copy link
Author

I tried that to extract the classes from instances :

import { DOMParser } from 'https://esm.sh/[email protected]'

const temp = ( new DOMParser ).parseFromString(`<style>
@import url("style.css") screen;
@media (min-width: 500px) {}
div {
	foo: bar;
}
</style>`, 'text/html' ).documentElement

const CSSStyleSheet = temp.sheet.constructor
const linkedomCSSStyleDeclaration = temp.style.constructor		// only Element.style , is part of linkedom
const CSSImportRule = temp.sheet.cssRules[0].constructor
const CSSMediaRule = temp.sheet.cssRules[1].constructor
const CSSStyleRule = temp.sheet.cssRules[2].constructor	// or CSSRule
const CSSStyleDeclaration = temp.sheet.cssRules[2].style.constructor
const CSSRule = CSSStyleRule.prototype.constructor

export {
	CSSStyleSheet,
	CSSStyleDeclaration, linkedomCSSStyleDeclaration,
	CSSImportRule,
	CSSMediaRule,
	CSSStyleRule,
	CSSRule,
}

@devingfx
Copy link
Author

devingfx commented Apr 25, 2023

My conclusion is that it's weird to have 2 different implementations of CSS parsing depending on using <style> or <elem style="">.
linkedom should use fully the cssom module to parse style attribute!! As this is the only aspect of CSS of this library and also not its role...

This means rewrite the way CSSStyleDeclaration is synced to the ownerElement and rename this class to be a child of CSSOM's CSSStyleDeclaration and maybe re-implement the Proxy things ...

( level of skill that I don't have! ^^; )

@WebReflection
Copy link
Owner

It’s not a goal of this project to be 100% standard compliant, there’s JSDOM for that goal … I think I remember why indeed I didn’t use cssom internally, neither I’ve exposed that class externally.

as performance here matters more than standard compliance, what is your use case for having cssom available? If the answer is test then I’m afraid I’m not keen to accommodate that use case with changes that might both break and will slow down this project.

@devingfx
Copy link
Author

devingfx commented Apr 25, 2023

I'm trying to re-implement a (compliant like) getComputedStyle for linkedom... but this means to implement a document.styleSheets that imply a way of loading/parsing <styles>s and <link rel=stylesheet>s and manipulate CSS classes.

  1. I agree with you that's not the scope of linkedom (It's why I try to do it from outside of the library) and that's a good thing to opt-in getComputedStyle if needed only

  2. I'm aware of JSDOM and other huge environments, but linkedom is fast! And so simple, no extra features, and that's good! Just need a basic CSS cascade resolver (that I did BTW, it's working bacause it use only W3C DOM specs and linkedom is fairly compliant on 99% ;) )

@WebReflection
Copy link
Owner

it's fast because I've pushed back on many similar requests (it misses just this, it misses just that, this should be more standard, ... and so on) but you gave me an idea around getComputedStyle as it passes through the windows proxy and it might be a decent guard to bootstrap more complex logic around CSS, hence use cssom when that's required.

I'll think about it, but not immediately.

@devingfx
Copy link
Author

devingfx commented Apr 27, 2023

Well, I'm not sure if you took it hard (I'm not english native)... If it's the case please apologize!

I did not asked you to add this or that nor to be more W3C compiant ! :)
I just asked "why not" exporting some already used bits... in fact, it is possible to just re-export cssom's exports, but as is, it could lead to unexpected weird bugs when using .style .
(BTW it's possible to access the classes using instances as I wrote before)

The idea behind my request was to be able to code a custom getComputedStyle for my project... not to be integrated in linkedom not make you work on it ! ;)

And finaly... I may still work on this on my side (the same it's not my priority), if you want I may share future stuff / investigation here...

PS: The issue was more a way to communicate than a real issue though !

@devingfx
Copy link
Author

it passes through the windows proxy and it might be a decent guard

I did'nt get why passing through the proxy is "a decent guard" ?

@WebReflection
Copy link
Owner

Apologies I’m not native English neither, let’s try to assume best intents from both side 😉

a guard meaning I can intercept the usage/need of getComputedStyle via the proxy and enable only when accessed a better cssom integration/story for style attributes and CSS parsing/behavior.

I’m short on time these days but I’ll have a look

@devingfx
Copy link
Author

devingfx commented Apr 28, 2023

AFAIK I'm afraid t say that implementing a getComputedStyle may be a real chalenge because of the CSS cascade. There are a lot of concern (not even covered by cssom) to take in account, especialy selector precedence and DOM layout, pseudos element etc...
You may just make possible to plug each project's own implementation instead of integrate it...
(In my case for ex. I don't need a full compliant browser selector precedence, just a "Object.assign()" kind of precedence in document inclusion order, to be honest, I could copy/paste a great part of JSDOM resolution algo )

The only real no go right now is the use of cssom "somewhere" but not "elsewhere" in linkedom, what's confusing when using a user-land own copy of cssom (or any other implementation).

  1. The fastest and easiest solution for linkedom is to deprecate .style parsing and let the user make his own implementation...
  2. The hardest way is to use cssom everywhere CSS parsing is needed, expose the classes, but as I said, this is more complicated that it sounds because of 2 version of CSSStyleDeclaration ...

Your choice BTW!

I'll be happy to help if you want me to start something (with your directives and good directions ;))

@WebReflection
Copy link
Owner

WebReflection commented Apr 29, 2023

OK, I took the time to give cssom a shot and here's what's problematic:

  • the style can be an observed attribute so I need to be able to pass through this project logic to trigger changes when any style.thinghy is mutated, in case it's observed
  • this means that I cannot use cssom CSSStyleDeclaration constructor out of the box, I still need to wrap it as Proxy so I gain nothing but a third party dependency to maintain (once wrapped) due this module own internal logic
  • the CSSStyleDeclaration in cssom is a bit outdated ... it doesn't react to properties such as style.backgroundColor = ... because it doesn't use a proxy and it doesn't normalize those properties
  • the only reason I use cssom is to parse CSS for <style> elements but here's the catch:
    • the sheet of a style element is a StyleSheet, not a CSSStyleDeclaration
    • because of previous point, there's literally no issue in using my own classes for different purposes/goals
    • I'd rather hide the detail behind <style>.sheet than harakiri myself in augmenting and wrapping a 3rd party module as nothing it offers is interesting for LinkeDOM logic

I am skeptic about exporting cssom because of the CSSStyleDeclaration class conflict but, most importantly, because cssom doesn't know anything about this module and doesn't offer me any hook to react to changes or mutations, it would be an hazard to expose logic that won't be reflected (Mutation Observer or anything else) within this module.

Last, but not least, bundled files like the worker.js can't benefit from tree-shaking if I export all cssom when I need, in fact, only its parser, so that's another no-go from my side.

Accordingly with these findings and blockers, the only escape hatch I can think about is to offer an export linkedom/cssom that just points at cssom and that would be it: you can use the same module exported in here + you are on your own with extra logic ... would that work?

@devingfx
Copy link
Author

devingfx commented Apr 29, 2023

I can to almost the same conclusions:

  • I undestood the role of linkedom's CSSStyleDeclaration being binded to the ownerNode...
  • CSSOM is outdated from W3C, and more complicated not maintained! The author claim some money to anything related to NV/CSSOM. There forks and notably 1 that is maintained and has integrated last PRs it's rrweb-io/CSSOM > npm i rrweb-cssom but I feel that dependency as potentialy problematic in future...
    • This made me search at other CSS parsers, some are promising (CSSTree, postcss) but exports custom AST and not an OM with names close to W3C CSSOM... It would require to rewrite a class sctructure and transform AST to this structure (too heavy dev, maybe a brand new linkeCSSOM ;) ).

So I came to the question:

Why parsing <style>.sheet tough? Is it so needed for SSR? (I'm not used at every frameworks like react, et cie) Can't it be removed, removing the same way 3rd party dependency. Just disclaim that CSS parsing is a user-land feature...
( I mean keeping .style parsing untouched because it is not realted to cssom at all)

@WebReflection
Copy link
Owner

Why parsing <style>.sheet tough? Is it so needed for SSR?

it's one nice-to-have thing I've previously used and if it's a no-brainer and it doesn't affect performance, as it's a lazy getter, the answer is more likely why not?

this is some pragmatism I've used when creating this project: if something is trivial, already solved elsewhere, and not perf critical, it's OK to have it in ... if any of these points doesn't match, it's either very needed, super useful, or both, otherwise it's a meh from my side.

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

2 participants