Use CSSTree for style transformations (new) #21936

strarsis · 2020-04-27T22:15:45Z

Description

This PR adds the CSSTree parser for wrapping the theme styles into a styles wrapper.

How has this been tested?

I tested this on my local WordPress site.

Types of changes

Because a custom CSS parser is currently used, parse errors can occur, as in url([...]url([...])[...]) causes styles wrapping to fail completely (although valid CSS) #21569.
CSSTree is a native JavaScript CSS parser that is already used in other major projects like svgo and csso, hence it should handle all CSS fast and well.
CSSTree can also generate sourcemaps and even reuse existing sourcemaps. With some API changes it should be possible to add sourcemap support in Gutenberg, which means that developers can still see from where the injected styles came from
(PoCs here: https://codepen.io/strarsis/pen/RwWRqZv and also https://codesandbox.io/s/busy-williamson-r1frp). (Not implemented with this PR, but CSSTree adds the foundation for it).

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.

strarsis · 2020-04-28T18:27:26Z

@youknowriad, @noahtallen: OK, PR is ready. The original structure is preserved.

youknowriad · 2020-04-29T09:38:23Z

packages/block-editor/src/utils/transform-styles/transforms/url-rewrite.js

-
-	return raw;
-}
+import { URL } from 'whatwg-url';


I know we've gone back and forth about this kind of module and we have a @wordpress/url package. @aduth is this correct, should we replace with our own module instead?

packages/block-editor/src/utils/transform-styles/transforms/test/url-rewrite.js

youknowriad · 2020-04-29T09:41:02Z

Can we get a quick sanity checks from theme authors here @kjellr @jasmussen to ensure the editor styles are still working as intended in the editor since this is a big rewrite.

@strarsis did you check if there's any performance impact here (initial loading) good or bad? (nom run test-performance)?

…` when rewriting URLs.

strarsis · 2020-04-29T16:42:22Z

@youknowriad: Hm, the test-performance script doesn't want to run through on my system (same issue on the master branch):

$ npm run test-performance

> gutenberg@7.9.1 test-performance [...]/gutenberg
> wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js

Downloading Chromium r737027 - 123.7 Mb [====================] 100% 0.0s
Chromium (737027) downloaded to [...]/gutenberg/node_modules/puppeteer/.local-chromium/linux-737027
● Validation Error:

  Module @wordpress/jest-console in the setupFilesAfterEnv option was not found.
         <rootDir> is: [...]/gutenberg/packages/e2e-tests
[...]
npm ERR! gutenberg@7.9.1 test-performance: `wp-scripts test-e2e --config packages/e2e-tests/jest.performance.config.js`

The @wordpress/jest-console package seems to be there though.

jasmussen · 2020-04-30T08:54:19Z

Hello! Thanks for the PR and thanks for the ping.

I'm usually able to check out branches in forks using this process:

git remote add strarsis git@github.com:strarsis/gutenberg.git
git fetch strarsis
git checkout strarsis/css-tree-transform-styles-2 -B css-tree-transform-styles-2

However I'm doing something wrong, as I'm not able to check out and compile this branch, sorry about that. Let me know if you spot my error.

To respond at a more high level though, I'm afraid I wasn't familiar with CSStree — what feedback are you looking for? Should the block editor and editor styles ideally look visually unchanged before and after this PR? Or does it change anything with regards to how editor styles are authored? What would be the impact if/when the block editor lands in an iframe and the DOM continues to be lightened until at one point it may be mostly identical to the frontend?

strarsis · 2020-04-30T13:25:20Z

@jasmussen: An iframe-based approach to style namespacing hasn't been implemented yet.
For now, this PR should improve the style-wrapping using a CSS parser/AST library (CSSTree).
It also fixes CSS-specific issues like #21569 and #16408 where the currently used parser doesn't support some syntax. Until the new iframe paradigm has been worked out and implemented, fixing the parse issues would be a good step because it can be a blocker in theme development as the styles won't load at all in the editor.

Concerning your branch checkout: I am not sure what went wrong in your case. I use WSL, maybe the line endings accidentally were changed?

jasmussen · 2020-05-01T07:26:38Z

Thanks for taking the time, and thanks again for the PR. I'm not sure why the branch checkout didn't work, but it seems to me like the best way to test this PR is to verify current editor styles look mostly (exactly?) the same before and after this branch, correct?

As far as iframes go, while it still might be a ways out, it also might be closer than we think. While it's important that the idea of the iframe PR landing should not stop forward momentum — it might not land, after all — it also seems prudent to think of the iframe as a serious possibility, and avoid painting ourselves into a corner if we can. But it sounds like you're saying making this work with an iframed editor should not be a big refactor?

youknowriad · 2020-05-01T09:49:34Z

it also seems prudent to think of the iframe as a serious possibility, and avoid painting ourselves into a corner if we can. But it sounds like you're saying making this work with an iframed editor should not be a big refactor?

I think we should be prudent about the iframe, even if we merge it at first, the PR there showed that we can't get rid of the non-iframed version quickly since the iframe version breaks blocks that rely on the global "window/document" object.

That said, If ever we go full iframe, this PR and the whole editor styles normalization won't be needed anymore (so just remove the code).

I think this PR is a good PR code wise, avoid the bundled dependency and use a more maintained library. I'd like to check these:

Editor styles still look the same (like master)
Performance and bundle size. (for some reason the bundle size bot is not triggering here)

strarsis · 2020-05-01T12:31:28Z

@youknowriad: How can this be better tested? I also have issues getting the performance test running, even on master. But I use WSL 1 on Windows, there may be subtle differences.

youknowriad · 2020-05-01T12:33:44Z

Yeah, can't speak about Windows, but once the theme styles are validated properly, I'll help with the performance test.

strarsis · 2020-05-01T12:50:27Z

@youknowriad: Ideally there should be some theme styles that can be tested against. I could put in all the WordPress core theme styles (minified) like TwentyTwenty.

oxyc · 2020-05-01T19:14:03Z

I couldn't get the tests running on macos either. Same error as you but it used to work a few months back.

strarsis · 2020-05-01T21:39:29Z

@oxyc: I managed to get it running. You have to invoke some scripts first and then run puppeteer in headless mode with some extra options.
Prepare the workdir: npm run build:packages, npm run wp-env start

Add fields to puppeteer launch options:
packages/scripts/config/puppeteer.config.js:

module.exports = {
        launch: {
                devtools: process.env.PUPPETEER_DEVTOOLS === 'true',
                headless: process.env.PUPPETEER_HEADLESS !== 'false',
                slowMo: parseInt( process.env.PUPPETEER_SLOWMO, 10 ) || 0,
                args: [
"--disable-gpu",
"--renderer",
"--no-sandbox",
"--no-service-autorun",
"--no-experiments",
"--no-default-browser-check",
"--disable-dev-shm-usage",
"--disable-setuid-sandbox",
"--no-first-run",
"--no-zygote",
"--single-process",
"--disable-extensions",
                ],
        },
};

Then run the performance test:
PUPPETEER_HEADLESS=true npm run test-performance

However, while the performance test runs now on master, it fails with:

● Performance › Loading, typing and selecting blocks                                                                                                                                                                                                                                                                                                                                                                                  No node found for selector: .block-editor-inserter__search-input

(packages/e2e-tests/specs/performance/performance.test.js:75)
Related issue: #22038

Also, the e2e/performance tests are currently broken in master because of recent changes in inserter search input markup, see #22045.

oxyc · 2020-05-29T23:45:36Z

I just tried to test this again but npm run test-performance on master throws random error message that others dont seem to get, just like mentioned in #22045 before.

This time:

No node found for selector: tr[data-slug="gutenberg-test-plugin-disables-the-css-animations"] .activate a

and

Evaluation failed: ReferenceError: wp is not defined

Trying to run the tests by mimicking what travis does (on macos):

nvm install --latest-npm
npm ci
npm run build
npm run wp-env start
npm run test-performance # fail

# this fails with random errors as well
./node_modules/.bin/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --listTests > ~/.jest-e2e-tests
./node_modules/.bin/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 4 == 0' < ~/.jest-e2e-tests )

strarsis · 2020-05-30T10:19:49Z

@oxyc: Hm, this error stems from puppeteer/chromium automated testing, when a node can't be found for a selector. But this is on the DOM itself, no CSS involved. The error will also cause the rest of the code failing to load, hence the wp related error afterwards. I think these are issues with the test and changed DOM structure - or a build script that hadn't run and updated the (committed) build artifacts that are used in the automated testing.

youknowriad · 2020-06-01T14:50:54Z

@oxyc The error you're having is because you're not loading the e2e test plugin on your environment (they are in packages/e2e-tests/plugins

youknowriad · 2020-06-01T14:52:09Z

also right now on master you can do this to compare two branches ./bin/plugin/cli perf branch1 branch2

youknowriad · 2020-06-01T14:52:47Z

Sorry for letting this PR stale, I'm still interested in landing this, do you think you can rebase it again?

strarsis · 2020-06-02T09:39:34Z

@youknowriad: Sure! 🐱

…css-tree-transform-styles-2

youknowriad · 2020-06-05T08:49:13Z

packages/block-editor/src/utils/transform-styles/index.js

- * WordPress dependencies
- */
-import { compose } from '@wordpress/compose';
+import { map, compose } from 'lodash';


I had an error here when testing. "compose" is on @wordpress/element

youknowriad

After fixing the compose error, I tried this PR and it seems the styles are not applied properly. I can tell because the styles of the buttons in 2019, for instance, are not correct (they should be blue and not that rounded)

oxyc · 2020-06-13T00:21:38Z

packages/block-editor/src/utils/transform-styles/index.js

-		}
-
-		return css;
+		return traverse( css, compose( transforms ) );


This shouldn't use compose since the result of the second transform gets called with the result of the first (undefined), it should just be fed the original arguments, eg:

return traverse( css, over( transforms ) );

But that would change the old traverse api.

oxyc · 2020-06-13T02:37:02Z

With the above fix I'm currently getting:

  ● Performance › Loading, typing and selecting blocks

    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Warning: React does not recognize the `%s` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `%s` instead. If you accidentally passed it from a parent component, remove it from the DOM element.%s isSelected isselected·

But I guess shouldnt actually have affected the results since they already ran? Might be worth checking again once master is running the tests correctly.

If that's true, the initial performance test for Loading time:

3 runs	Average time to load	Average time to DOM content load
Before	10582ms	10479ms
After	10495ms	10392ms

strarsis · 2020-06-13T11:22:24Z

I am sorry that I hadn't really responded yet, I am very busy right now.
But from the performance data it looks that the PR would actually increase the performance (though even slightly)?

oxyc · 2020-06-13T16:16:18Z

After fixing the compose error, I tried this PR and it seems the styles are not applied properly. I can tell because the styles of the buttons in 2019, for instance, are not correct (they should be blue and not that rounded)

Found the bug.

Before:

.editor-styles-wrapper .wp-block-button:not(.is-style-outline) .wp-block-button__link

After

.editor-styles-wrapper .wp-block-button:not(.editor-styles-wrapper .is-style-outline) .wp-block-button__link

oxyc · 2020-06-13T16:53:42Z

Here's a fix for the above bug: oxyc@6b746cb

And here's a diff of twentyninenteens styles before and after (looks correct)

https://gist.github.com/oxyc/479a82cda75d92d0f30ba770c8fbc60f#file-after-patch

oxyc · 2020-06-13T17:19:05Z

Tested it with own theme where I first encountered #16408 and found another bug:

 @-webkit-keyframes background-zoom-sm {
-	0% {
+	.editor-styles-wrapper 0% {

Fix: oxyc@1c1590b

So with the following fixes and a fix for the merge artifacts I'd call the styles verified:

strarsis · 2020-06-13T17:44:15Z

@oxyc: Awesome, thanks! I got very little time currently, it is nice that you further looked into it.

strarsis · 2020-09-21T17:41:52Z

@youknowriad, @oxyc, @noahtallen: Superseded by new PR on top of latest upstream master: #25514

strarsis · 2023-10-20T20:43:17Z

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

Use CSSTree for style transformations.

8475db6

strarsis requested review from ellatrix and youknowriad as code owners April 27, 2020 22:15

strarsis mentioned this pull request Apr 27, 2020

Use css-tree parser for wrapping theme styles #21742

Closed

5 tasks

strarsis added 3 commits April 28, 2020 20:25

Add test for url() inside (valid) data URL.

cdb9043

Re-add DOM traversal test.

362098b

Remove old CSS parser.

dd91ca2

strarsis force-pushed the css-tree-transform-styles-2 branch from 903d14a to dd91ca2 Compare April 28, 2020 18:26

youknowriad reviewed Apr 29, 2020

View reviewed changes

packages/block-editor/src/utils/transform-styles/transforms/test/url-rewrite.js Outdated Show resolved Hide resolved

Add code and test to skip host-relative URLs (e.g. `/images/image.jpg…

9ca0894

…` when rewriting URLs.

strarsis mentioned this pull request Apr 30, 2020

transformStyles not working correctly #16408

Closed

Merge branch 'master' of https://github.com/WordPress/gutenberg into …

e25d57a

…css-tree-transform-styles-2

strarsis requested review from ajitbohra, dmsnell, epiqueras, gziolo, jorgefilipecosta, nerrad, noahtallen, oandregal, ntwb and swissspidy as code owners June 3, 2020 18:19

youknowriad reviewed Jun 5, 2020

View reviewed changes

oxyc reviewed Jun 13, 2020

View reviewed changes

oxyc mentioned this pull request Jun 13, 2020

Handle closing parenthesis within data url in transform-styles (#16408) #17146

Closed

5 tasks

strarsis mentioned this pull request Sep 21, 2020

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

Closed

5 tasks

strarsis closed this Sep 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CSSTree for style transformations (new) #21936

Use CSSTree for style transformations (new) #21936

strarsis commented Apr 27, 2020

strarsis commented Apr 28, 2020

youknowriad Apr 29, 2020

youknowriad commented Apr 29, 2020

strarsis commented Apr 29, 2020 •

edited

Loading

jasmussen commented Apr 30, 2020

strarsis commented Apr 30, 2020 •

edited

Loading

jasmussen commented May 1, 2020

youknowriad commented May 1, 2020

strarsis commented May 1, 2020

youknowriad commented May 1, 2020

strarsis commented May 1, 2020

oxyc commented May 1, 2020

strarsis commented May 1, 2020 •

edited

Loading

oxyc commented May 29, 2020 •

edited

Loading

strarsis commented May 30, 2020

youknowriad commented Jun 1, 2020

youknowriad commented Jun 1, 2020

youknowriad commented Jun 1, 2020

strarsis commented Jun 2, 2020

youknowriad Jun 5, 2020

youknowriad left a comment

oxyc Jun 13, 2020 •

edited

Loading

oxyc commented Jun 13, 2020 •

edited

Loading

strarsis commented Jun 13, 2020 •

edited

Loading

oxyc commented Jun 13, 2020

oxyc commented Jun 13, 2020 •

edited

Loading

oxyc commented Jun 13, 2020 •

edited

Loading

strarsis commented Jun 13, 2020

strarsis commented Sep 21, 2020 •

edited

Loading

strarsis commented Oct 20, 2023

Use CSSTree for style transformations (new) #21936

Use CSSTree for style transformations (new) #21936

Conversation

strarsis commented Apr 27, 2020

Description

How has this been tested?

Types of changes

Checklist:

strarsis commented Apr 28, 2020

youknowriad Apr 29, 2020

Choose a reason for hiding this comment

youknowriad commented Apr 29, 2020

strarsis commented Apr 29, 2020 • edited Loading

jasmussen commented Apr 30, 2020

strarsis commented Apr 30, 2020 • edited Loading

jasmussen commented May 1, 2020

youknowriad commented May 1, 2020

strarsis commented May 1, 2020

youknowriad commented May 1, 2020

strarsis commented May 1, 2020

oxyc commented May 1, 2020

strarsis commented May 1, 2020 • edited Loading

oxyc commented May 29, 2020 • edited Loading

strarsis commented May 30, 2020

youknowriad commented Jun 1, 2020

youknowriad commented Jun 1, 2020

youknowriad commented Jun 1, 2020

strarsis commented Jun 2, 2020

youknowriad Jun 5, 2020

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

oxyc Jun 13, 2020 • edited Loading

Choose a reason for hiding this comment

oxyc commented Jun 13, 2020 • edited Loading

strarsis commented Jun 13, 2020 • edited Loading

oxyc commented Jun 13, 2020

oxyc commented Jun 13, 2020 • edited Loading

oxyc commented Jun 13, 2020 • edited Loading

strarsis commented Jun 13, 2020

strarsis commented Sep 21, 2020 • edited Loading

strarsis commented Oct 20, 2023

strarsis commented Apr 29, 2020 •

edited

Loading

strarsis commented Apr 30, 2020 •

edited

Loading

strarsis commented May 1, 2020 •

edited

Loading

oxyc commented May 29, 2020 •

edited

Loading

oxyc Jun 13, 2020 •

edited

Loading

oxyc commented Jun 13, 2020 •

edited

Loading

strarsis commented Jun 13, 2020 •

edited

Loading

oxyc commented Jun 13, 2020 •

edited

Loading

oxyc commented Jun 13, 2020 •

edited

Loading

strarsis commented Sep 21, 2020 •

edited

Loading