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

Use css-tree for block-editor style transformations (3). #25514

Closed

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Sep 21, 2020

@youknowriad, @oxyc, @noahtallen: Note: This PR supersedes the older PRs (css-tree-transform-styles and css-tree-transform-styles-2) with changes on top of latest upstream master. Special thanks @oxyc for his fixes for the code in this PR.

Description

This PR adds the CSSTree parser for wrapping the theme styles into a styles wrapper and WHATWG URL for URL manipulation of URLs in styles.

How has this been tested?

I tested this on my local WordPress site.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Use WHATWG URL for block-editor style URL manipulations.
@strarsis strarsis changed the title Use css-tree for block-editor style transformations. Use css-tree for block-editor style transformations (3). Sep 21, 2020
@strarsis
Copy link
Contributor Author

The CI linter task complains: /home/runner/work/gutenberg/gutenberg/build/block-editor/index.js 1:5821 error Parsing error: The keyword 'const' is reserved
So const is not allowed? Is this an issue with upstream master?

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter is complaining because the library you're including (css-tree probably) is not transpiled so it leaves "const" in the built files which don't work on IE11. I'm not certain how to solve this though.

@@ -54,10 +55,11 @@
"@wordpress/wordcount": "file:../wordcount",
"classnames": "^2.2.5",
"css-mediaquery": "^0.1.2",
"css-tree": "^1.0.0-alpha.39",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this using an "alpha" version? can we use the latest stable instead?

Copy link
Contributor Author

@strarsis strarsis Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this doesn't make sense, css-tree is in use for many years and very stable, see this issue csstree/csstree#134 for having it finally published at least as a beta release now.

packages/block-editor/package.json Outdated Show resolved Hide resolved
@strarsis
Copy link
Contributor Author

strarsis commented Sep 22, 2020

@youknowriad: The css-tree package ships with a dist build that can also be required instead.
I require that instead, which should solve the linter/browser compat issue.

@youknowriad
Copy link
Contributor

I've been looking at the bundle-size impact of this PR. See the comment here #25533 (comment)

Unfortunately, it's way too big. +124 kB is 9% of all the JS of Gutenberg for a single dependency. Do you think there's any way to improve that, maybe by trying to load only aa subset of css-tree?

@strarsis
Copy link
Contributor Author

strarsis commented Sep 22, 2020

@youknowriad: The linter still complains about const, even with dist. I could switch back to non-dist and see whether the file size goes down. I can't find a const in the file at the position the linter complains about, so apparently the build results diverge for me and the CI/linter environment.

@strarsis
Copy link
Contributor Author

@youknowriad: Have the sizes changed now favourably?
Would it be possible to set up the bundler to transpile the css-tree dependency to ES5? For const, just var is used.

@youknowriad
Copy link
Contributor

Looks like the size diff is pretty similar regardless 🤔
I didn't try compiling the dependency yet, I guess we might need to unexclude it from webpack config and potentially babel.

Base automatically changed from master to trunk March 1, 2021 15:44
@strarsis
Copy link
Contributor Author

strarsis commented Aug 18, 2021

Of course, using a dedicated browser API for this would be great, but sadly it isn't supported at all yet (notably the Parser API): https://ishoudinireadyyet.com/

@strarsis
Copy link
Contributor Author

Hm, what about using the native Browser API and a CSS selector parser (as there is no Browser API for this yet) to achieve this?
PoC: https://codepen.io/strarsis/pen/MWmMQgy
This uses the native CSS parser/CSSOM of the browser.

@zaguiini
Copy link
Member

zaguiini commented Mar 13, 2023

This library is indeed a little bit heavy. Tree-shaking (picking some methods directly from the lib folder) reduces it to ~60KB but still, I think we can get something better than that.

https://github.com/adobe/css-tools is promising -- it has a parse function that returns an AST and stringify that returns the compiled CSS. Maybe we could use that instead? The parse function weighs about 10KB, and so does stringify. Maybe we can give it a go.

function isRemotePath( filePath ) {
return /^(?:https?:)?\/\//.test( filePath );
}
import { URL } from 'whatwg-url';
Copy link
Member

@zaguiini zaguiini Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this dependency is necessary anymore. URL has achieved a good level of browser standardization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. Browser support is basically 100% now. NodeJS supports it, too.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 13, 2023

Picking methods directly looks promising, hopefully this shaves off enough bytes to make it acceptable.

@strarsis
Copy link
Contributor Author

Alright, I rebase and resolve all conflicts to get this up to date. Then I clean things up and try to get the size as small as possible.

@zaguiini
Copy link
Member

zaguiini commented Mar 17, 2023

I managed to shave some bytes by cherry-picking the css-parse imports on a draft branch. However, it's still a huge price to pay:

trunk: 668kb
non-cherry-picked css-tree: 825kb
cherry-picked css-tree: 729kb

According to Bundlephobia, @adobe/css-tools would add 12kb to the trunk bundle size, and that's unminified -- it goes to 3.4kb once minzipped.

It's not a drop-in replacement, though, so a bigger rewrite would be necessary.

EDIT: Surprisingly, we're actually using @adobe/css-tools on trunk! It's a fork of reworkcss/css and that's the code in packages/block-editor/src/utils/transform-styles/ast, so it won't really fix our problem.

I tried using lightningcss but you need to do that top-level await madness in order to kick off the WASM download (which, btw, is 8 MB). Other options are regex-based and they might have the same problems (if not more) that the implementation we currently have.

I think we'll need to bite the bullet here... Considering the improvements and the reliability of css-tree, I'd say it's OK to have a 9.1% increase in bundle size if that means CSS transformations and inlining will work properly. That's my 2 cents, though. Interested in hearing other maintainers' thoughts @youknowriad @ellatrix

@youknowriad
Copy link
Contributor

Personally, I would have been ok with the bundle size change, if the fixes are for all users. The reality though is that block themes use iframes and there's a goal to actually move to iframes for everyone which means we'll be paying an extra price for code that is not used for most users. WDYT?

@kraftner
Copy link

I think realistically a lot of sites will not run on block themes for quite some time (for various, valid reasons) so this is still very much worth fixing. Especially since this is not some kind of new feature but a bug.
All even more so since this issue will probably only get worse as new CSS features are constantly being introduced. Here is one recent example with @layer

@strarsis
Copy link
Contributor Author

strarsis commented Mar 20, 2023

@kraftner: CSS Layers are supported by the css-tree parser though.

@youknowriad, @kraftner, @zaguiini; @dsturm:
There is an alternative though, that leverages the existing browser APIs to parse and process the styles, see this working example:
https://codepen.io/strarsis/pen/MWmMQgy
It has an extremely low footprint, should be very fast (as it uses the native browser parsing capabilities) and what the browser can parse, it can parse and process, too.
Its only dependency is CSSwhat as there is no browser API for selector-level CSS parsing yet.

@kraftner
Copy link

@kraftner: CSS Layers are supported by the css-tree parser though.

Okay, sorry, I didn't actually check this myself. But I think the general assumption that an outdated, broken parser will inevitably only continue to have more bugs when new CSS features get introduced over time still stands. And since WP is strong on back compat this will become a worsening problem for many years to come.

@youknowriad, @kraftner, @zaguiini: There is an alternative though, which uses the existing browser APIs to parse and process the styles, see this working example: https://codepen.io/strarsis/pen/MWmMQgy It has an extremely low footprint, should be very fast (as it uses the native browser parsing capabilities) and what the browser can parse, it can parse and process, too.

No personal opinion on the how, you're much more competent on that @strarsis 😄

@strarsis
Copy link
Contributor Author

strarsis commented Mar 20, 2023

@kraftner: I am also unhappy with the state of JS CSS parsers and the ongoing addition of new CSS stuff that has to be supported. This is not a perfect solution of course, this should rather fix the most blocking issues with CSS parsing. The only reason for why I picked css-tree in the first place was that it appears to be the most maintained parser right now. It is quite big, which is its greatest downside (beta versioning was another one). If a better CSS parser can be found, I would gladly use that one instead. With a halfway decent AST (and stringification of course) basically any parser could be used.

@youknowriad
Copy link
Contributor

@strarsis What's the downsides of your browser based parsing? Seems like the best option for me so far based on your description?

@strarsis
Copy link
Contributor Author

strarsis commented Mar 20, 2023

@strarsis What's the downsides of your browser based parsing?

The browser parsers could be subtly different, that's the only real downside I can think of right now. Browser CSS parsers are all much more advanced and tested than most libraries. And when something is not understood by the browser CSS parser, the original particular styles would not be usable anyway by that browser.

Gutenberg frontend testing is already performed in a headless but otherwise complete browser (JS API-wise), so this should not be a problem.

So yes, this solution appears to be indeed the best one. I can make a PR following this approach instead.

@zaguiini
Copy link
Member

zaguiini commented Mar 20, 2023

@strarsis I don't quite understand the solution using css-what as that library is a selector parser so how exactly are we able to transform and walk through the declaration lists (CSS rules blocks) using it?

I haven't had the time to fully play with it, though, but it looks like it won't let the user add additional transformers.

@strarsis
Copy link
Contributor Author

@zaguiini: The browser does the CSS parsing on rule level. There is no browser API yet for parsing the selectors themselves, hence CSSWhat is used. The CSS values can also be parsed using the browser API (CSSStyleValue), the browser support improved a lot since then (upcoming Safari should have it, Firefox does not, but this can change when all browsers finally support it). But CSS values can also be parsed using a JavaScript library. This is not 100% perfect, but lots of parsing is delegated to the browser. And the required additional bytes are much, much less than using a full CSS parser.

Concerning the extensibility: Well, for Gutenberg style isolation only two tasks are needed: 1. Wrap the selectors (prefix with a class selector), 2. Rewrite the url(...) references, so the style-relative URIs still work when those styles are inlined in editor.
Adding new tasks is also possible, as the browser handles the rule level CSS parsing, and with more time the browser APIs should become so good that it can basically replace a CSS parser library, but with the same compatibility and flexibility as the browser itself.

@strarsis
Copy link
Contributor Author

@youknowriad, @kraftner, @zaguiini; @dsturm:
After some more experimentation I found an extremely easy and nearly 100% supported approach for style isolation:
https://codepen.io/strarsis/pen/zYJmpoV

This uses Shadow DOM (Web Components API) (no <iframe>s!) and can be even be directly used with React (on which Gutenberg is currently based on): ReactShadow

No style post-processing necessary, just creating the shadow DOM for the editor - in open mode it can even be still easily accessed from the document. Styles are simply isolated inside.
This is in fact so easy and simple that I wonder whether someone else already tried this and found a show stopper? But I cannot find one, and together with React-compatibility this appears to be the ideal solution to the style isolation problem.

@youknowriad
Copy link
Contributor

@strarsis I know @ellatrix already tried shadow DOM at multiple occasions. Not sure how far we went there.

@strarsis
Copy link
Contributor Author

@zaguiini, @youknowriad: The Lightning CSS parser is written in Rust, but can be used in browser (WASM (Web Assembly)). It has a size of 20 kB (minified) / 5.4 kB (minified+gzipped):
https://bundlephobia.com/package/lightningcss-wasm@1.19.0
The Servo browser uses this, so it should be quite feature-complete.
Using this avoid the necessity of CSSWhat and other helper libraries that have to polyfill or complement the CSS Typed OM API

@zaguiini
Copy link
Member

@strarsis Lightning CSS is not this small 😅 Been there, done that. From above:

I tried using lightningcss but you need to do that top-level await madness in order to kick off the WASM download (which, btw, is 8 MB).

@strarsis
Copy link
Contributor Author

@youknowriad, @kraftner, @zaguiini; @dsturm:
PR for post-processing styles using the browser APIs (with supplemental libraries):
#49483

@strarsis
Copy link
Contributor Author

strarsis commented Oct 20, 2023

New editor styles transformation approach using PostCSS is now merged (many thanks @zaguiini)!

I am closing this PR in favor of the now merged in PostCSS method, which has a much smaller footprint and excellent style manipulation capabilities.

@strarsis strarsis closed this Oct 20, 2023
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

Successfully merging this pull request may close these issues.

4 participants