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

Replacing cssom with mensch (because conversion to object model loses important stuff) #215

Closed
bago opened this issue Jun 21, 2016 · 4 comments

Comments

@bago
Copy link
Contributor

bago commented Jun 21, 2016

For mosaico I'm evaluating a "browser-side" css inliner. I did a lot of evaluation of existing tools (i'm writing a blog post too).

Now, given I need to do that in javascript and in the client my best option is Juice. The main issues with Juice are related to CSSOM: CSSOM loses many informations from the CSS so prevent Juice to do the "right thing".

I found specific use-case issues that have already been reported:
#205
#27

Now I've just forked juice and I'll try replacing cssom with mensch (a CSS parser we already happily use in mosaico because it preserve multiple properties, it preserve comments and let us manipulate the CSS preserving the formatting and most things we may not understand).

With regard to "multiple property declarations" (ofen used for flex or linear-gradient, but also used for rgba... like "color: #ccc; color: rgba(128,128,128,.8)" my plan was to do something similar to styliner and inline multiple values only when they are declared in the same "declaration group" (same rule). This keeps the inlined code "small" but most time it will do what the user expects.

Another thing I'd like to do is be able to use jquery instead of cheerio, so that I don't need to bring cheerio (and its dependencies) on the client (we already do this in mosaico, running cheerio in the backend but jquery in the browser). This needs some abstraction in the library but it worked in mosaico.

I know this is a major change, so I wanted to introduce this ASAP (I still have to put my hands in the code). What do you think? Is juice interested in similar changes? Should I try to work with PR in mind, or should I simply fork and forget?

@jrit
Copy link
Collaborator

jrit commented Jun 21, 2016

There are a few open issues that are related to CSSOM, such as you pointed out, that I'd like to see resolved. Replacement with Mensch or something else would be really interesting to me.

I'm not opposed to the jQuery/Cheerio change, but the current API exposes Cheerio directly, so that might require some thought about either changes to the API or making some reasonable assurance that jQuery and Cheerio behave the same - though it sounds like maybe you've already done that on your other project.

In any case, the current dependencies are clearly causing some issues and if you'd like to try and resolve them, please open a PR.

@bago
Copy link
Contributor Author

bago commented Jun 22, 2016

I just complete my first run replacing "cssom" with "mensch".

Tests are passing but the one that checks the cssom object structure result (of course) and those checking the "PreservedContents" because using mensch I keep the formatting of "preserved contents" AS IS, and I don't need to "rebuild" it, so the formatting is different but the content is the same (I also have been able to remove some code from juice because I don't need to understand @media e @font-face content in order to preserve them) . Is this OK or you would prefer to reformat that "preserved content" to mimic the CSSOM behaviour?

Also, with mensch it would be easy to keep @supports, @Keyframes, @import and other @at-rules not being currenlty preserved... shouldn't we preserve anything we don't understand instead of selectively keep @media and @font-face ?

While I wait for your opinions about this I'll start to dig in the "client.js" logic because I can't use key=>value when I want multiple occourences of key to be supported.

bago added a commit to bago/juice that referenced this issue Jun 22, 2016
First step for Automattic#215 , just changed 1 testcase and
altered test comparison to consider multiple newlines as one newline.
bago added a commit to bago/juice that referenced this issue Jun 23, 2016
Test case for Automattic#216
This is fixed by moving from cssom to mensch as per Automattic#215
@jrit
Copy link
Collaborator

jrit commented Jun 23, 2016

I don't feel a need to mimic the CSSOM behavior unless the breaking changes there would cause users really legitimate problems, mainly, loss of features they rely on currently.

With the @ rules, historically, there have been a lot of requests to preserve certain parts of CSS and get rid of others in order to have the most efficiently sized output. I think the most reasonable default is to preserve all of them. I'm not totally convinced what the best options are on top of that default, but if you can remove the current @ options and just include a preserveAtRules options that defaults to true I feel like that would cover any use cases I'd be interested in.

@bago bago mentioned this issue Jun 23, 2016
@bago
Copy link
Contributor Author

bago commented Jun 23, 2016

#220 merged, cool! So this one, #44, #74 and #216 can be closed too.
I'll open new issues for my next steps as soon as I'll have something to say ;-)

@bago bago closed this as completed Jun 23, 2016
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