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 browser parser APIs for block-editor style transformations. #49483

Closed

Conversation

strarsis
Copy link
Contributor

What?

This PR uses the browser API for parsing and processing the frontend styles.
Where the browser API is yet insufficient, few, small, feature-focused and well-tested libraries are used.

Why?

Because a custom CSS parser is currently used (that is not developed as part of a dedicated parser project), many issues with style parsing and processing occur, often crashing the post-processing completely, resulting in no editor styles applied at all.

Note: A previous attempt that uses the css-tree parser has unacceptable size requirements (#25514).

How?

This PR implements the CSS post-processing by leveraging the currently supported browser APIs, supplemented by small, feature-focused libraries, where browser support is yet insufficient, allows to increase robustness, performance and size efficiency.

Testing Instructions

  1. Use the Gutenberg plugin with this PR (branch) applied.
  2. Add frontend styles or other styles as editor stylesheet that are known to cause issues with the custom CSS parser (e.g. url([...]url([...])[...]) causes styles wrapping to fail completely (although valid CSS) #21569; Error while traversing the CSS: property missing ':' #19930 and from other issues).
  3. Open the Gutenberg editor,
    note that the editor styles are correctly post-processed and applied, resulting in correctly wrapped selectors and rewritten URLs.

Testing Instructions for Keyboard

(UX is and should not be changed by this PR)

Screenshots or screencast

(Not applicable)
PoC of style post-processing using the browser API: https://codepen.io/strarsis/pen/MWmMQgy

Additional notes

I had issues with installing the npm dependencies (Ubuntu 22.04 LTS on WSL 2 on Windows 10), albeit the correct node (by .nvmrc) and npm (by package.json constraints) versions were used, npm install failed with error cb() never called!.
The solution was self-upgrading npm, but not to the latest available version of it (which would be incompatible with the required constraints by package.json), rather to its version that comes with that node version:

  • nvm install
  • nvm use
  • (npm install would result in error cb() never called! on a new node installation (although npm is of version 6.14.18).
  • npm install -g npm@6.14.18 # self-upgrade npm to its latest version in the constraints required by package.json
  • npm install # installation works now.

@strarsis strarsis requested a review from ellatrix as a code owner March 30, 2023 14:34
@strarsis strarsis changed the title Browser parser transform styles Use browser parser APIs for block-editor style transformations. Mar 30, 2023
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 30, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @strarsis! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@strarsis
Copy link
Contributor Author

The tests may have to be adjusted as now the browser AST is used.

Fix CSSwhat import.
Add root selector ignoring.
Fix selector skipping.
Update test snapshots (whitespace/newlines).
Fix CSSStyleRule check.
@zaguiini
Copy link
Contributor

zaguiini commented Mar 30, 2023

We're so close 🤏 to getting this right. Running this with .block { background: red; font-size: 1em; } I'm getting:

image

Which is not ideal because I'm getting the expanded version of the background rule. Is there any way to not expand the rule value with CSSStyleDeclaration?

Ideally, we should return the same set of declarations inputted by the user.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 30, 2023

@zaguiini: Is the stringified representation compact again? The browser represents the styles (CSSStyleDeclaration) as expanded as possible. But when it is transformed back into a string, it should look the same.

I prepended your input string to the PoC and it appears to be OK (.wrapper .block { background: red; font-size: 1em; }):
https://codepen.io/strarsis/pen/MWmMQgy (first line in input, check the output section at the bottom of page).

@zaguiini
Copy link
Contributor

@zaguiini: Is the stringified representation compact again? The browser represents the styles (CSSStyleDeclaration) as expanded as possible. But when it is transformed back into a string, it should look the same.

I prepended your input string to the PoC and it appears to be OK (.wrapper .block { background: red; font-size: 1em; }): https://codepen.io/strarsis/pen/MWmMQgy (first line in input, check the output at the bottom).

Yes, in this case, it's right. What I'm trying to do is to use traverseCSS to create an object from the CSS rules passed to the block. I'm defining background: red; but getting the longhand in cssRule.style. 😢

@strarsis
Copy link
Contributor Author

strarsis commented Mar 30, 2023

@zaguiini: So you want to parse the input styles, transform them and then get them back as modified string, right?
The transformCss (typo) traverseCSS function parses the input styles, then calls passed callback functions on each rule for transformation and then stringifies it back:
https://github.com/WordPress/gutenberg/pull/49483/files#diff-8b4a425589ca9e1508f73c26dda17e148bcc4faadd62c232906041fef55a6752R17

What are you using to modify the background property (the callback function)? The compact representation should be preserved.

For more granularity the postcss-value-parser is used, which allows fine-level value modifications, e.g. also the background value. It is well-tested and works in all browsers.

@zaguiini
Copy link
Contributor

@zaguiini: So you want to parse the input styles, transform them and then get them back as modified string, right? The transformCss (typo) traverseCSS function parses the input styles, then calls passed callback functions on each rule for transformation and then stringifies it back: https://github.com/WordPress/gutenberg/pull/49483/files#diff-8b4a425589ca9e1508f73c26dda17e148bcc4faadd62c232906041fef55a6752R17

What are you using to modify the background property (the callback function)? The compact representation should be preserved.

Well my use case is a little different:

  1. I need to parse a block;
  2. Walk through every rule;
  3. Add it to an object using the property name as key and its value as the value in the object.

Like that:

image

style.getPropertyValue( 'background' ) returns the shorthand version correctly, however I do not see background in the .style iterator, sadly. I only see the expanded property names. Is there any way to get the original property name?

@zaguiini
Copy link
Contributor

Did we ever consider using postcss? It doesn't look heavy: https://bundlephobia.com/package/postcss@8.4.21

@strarsis
Copy link
Contributor Author

strarsis commented Mar 30, 2023

@zaguiini: What browser are you using? For me in Chrome on Windows 10 the actual, unexpanded properties are listed with numerical index:
https://codepen.io/strarsis/pen/PodvjYp (check console output)
scr

@zaguiini
Copy link
Contributor

zaguiini commented Mar 30, 2023

@zaguiini: What browser are you using? For me in Chrome on Windows 10 the actual, unexpanded properties are listed with numerical index: https://codepen.io/strarsis/pen/PodvjYp (check console output) scr

I'm using Brave on MacOS. Can you try it with background?

Also, another important thing: does the CSSOM parser preserve prefixed, cross-browser properties? I don't think so but I'd like to confirm with you.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 30, 2023

@zaguiini: It does support (vendor) prefixed property names (in Chrome were I tested)! https://codepen.io/strarsis/pen/MWmMQgy

  .example {
  -webkit-mask: url('images/test/shape.svg');
          mask: url('images/test/shape.svg');
}
.example { -webkit-mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); }

@zaguiini
Copy link
Contributor

@zaguiini: It does support (vendor) prefixed property names (in Chrome were I tested)! https://codepen.io/strarsis/pen/MWmMQgy

  .example {
  -webkit-mask: url('images/test/shape.svg');
          mask: url('images/test/shape.svg');
}
.example { -webkit-mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); }

Can you confirm it works if we're adding URL transformation for example? Does it show up in the style object?

@strarsis
Copy link
Contributor Author

(https://codepen.io/strarsis/pen/MWmMQgy)
Input:

  .example { -webkit-mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); }

Output:

.wrapper .example { -webkit-mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); mask: url("https://example.com/test/wp/theme/test/images/test/shape.svg"); }

@strarsis
Copy link
Contributor Author

@zaguiini: The thing is that the browser will only process what it understands. So a Firefox browser handles the -moz vendor prefix, while Chrome (and other Webkit-based browsers) handle the -webkit vendor prefix.
And that is totally fine - if there are any styles that are invalid or not supported by the browser, it would not be used by it anyway.
As long as the browser also does the processing of the styles (which it does), all post-processed styles will have the proper, matching vendor-prefixes.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 31, 2023

@youknowriad, @zaguiini: PostCSS, that @zaguiini proposed, can be used in the browser, I should also give that a try.
Together with the existing PostCSS Editor Styles plugin the PostCSS URL rebase plugin should perform the required transformations. I prepare a 3rd PR that uses PostCSS. The bundle size also looks good so far.

PR that uses PostCSS: #49521

@strarsis
Copy link
Contributor Author

strarsis commented Oct 20, 2023

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

@strarsis
Copy link
Contributor Author

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

I am closing this browser-level approach PR now in favor of the merged PostCSS approach.
When the browser APIs became much more powerful in the future, one could revisit this approach, but using PostCSS is currently the best way for this kind of style manipulation.

@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
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants