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

oxyc · 2019-08-22T13:35:18Z

Description

I'm not the actual author of this but it fixes the case when url embedded svgs containing parenthesis are not parsed properly as values. See reworkcss/css#67 (comment). Fixes #16408

How has this been tested?

I tested on a site I'm building with a full tailwindcss + swiper (which had the offending svg), minified. 310kb of minified css and it works fine now.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested. (Sort of...)
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.

oxyc · 2019-08-22T14:33:50Z

So this PR causes failure on the = after charset in twentynineteeen styles-editor.css.

~~To be honest someone else might need to take over from here~~. I guess this PR now requires either ' or ". I don't see how regex can parse these nested parenthesis properly.

@font-face {
  font-family: 'NonBreakingSpaceOverride';
  src: url(data:application/font-woff2;charset=utf-8;base64,d09GMgABAAAAAAMoAA0AAAAACDQAAALTAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP0ZGVE0cGh4GYACCahEICjx3CywAATYCJANUBCAFhiEHgWwbXQfILgpsY+rQRRARwyAs6uL7pxzYhxEE+32b3aeHmifR6tklkS9hiZA0ewkqGRJE+H7/+6378ASViK/PGeavqJyOzsceKi1s3BCiQsiOdn1r/RBgIJYEgCUhbm/8/8/h4saPssnTNkkiWUBrTRtjmQSajw3Ui3pZ3LYDPD+XG2C3JA/yKAS8/rU5eNfuGqRf4eNNgV4YAlIIgxglEkWe6FYpq10+wi3g+/nUgvgPFczNrz/RsTgVm/zfbPuHZlsuQECxuyqBcQwKFBjFgKO8AqP4bAN9tFJtnM9xPcbNjeXS/x1wY/xU52f5W/X1+9cnH4YwKIaoRRAkUkj/YlAAeF/624foiIDBgBmgQBeGAyhBljUPZUm/l2dTvmpqcBDUOHdbPZWd8JsBAsGr4w8/EDn82/bUPx4eh0YNrQTBuHO2FjQEAGBwK0DeI37DpQVqdERS4gZBhpeUhWCfLFz7J99aEBgsJCHvUGAdAPp4IADDCAPCEFMGpMZ9AQpTfQtQGhLbGVBZFV8BaqNyP68oTZgHNj3M8kBPfXTTC9t90UuzYhy9ciH0grVlOcqyCytisvbsERsEYztiznR0WCrmTksJwbSNK6fd1Rvr25I9oLvctUoEbNOmXJbqgYgPXEHJ82IUsrCnpkxh23F1rfZ2zcRnJYoXtauB3VTFkFXQg3uoZYD5qE0kdjDtoDoF1h2bulGmev5HbYhbrjtohQSRI4aNOkffIcT+d3v6atpaYh3JvPoQsztCcqvaBkppDSPcQ3bw3KaCBo1f5CJWTZEgW3LjLofYg51MaVezrx8xZitYbQ9KYeoRaqQdVLwSEfrKXLK1otCWOKNdR/YwYAfon5Yk8O2MJfSD10dPGA5PIJJQMkah0ugMJiv6x4Dm7LEa8xnrRGGGLAg4sAlbsA07sAt76DOsXKO3hIjtIlpnnFrt1qW4kh6NhS83P/6HB/fl1SMAAA==) format("woff2"), url(data:application/font-woff;charset=utf-8;base64,d09GRgABAAAAAAUQAA0AAAAACDQAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAABGRlRNAAAE9AAAABwAAAAchf5yU0dERUYAAATYAAAAHAAAAB4AJwAbT1MvMgAAAaAAAABJAAAAYJAcgU5jbWFwAAACIAAAAF4AAAFqUUxBZ2dhc3AAAATQAAAACAAAAAgAAAAQZ2x5ZgAAApAAAAAyAAAAPL0n8y9oZWFkAAABMAAAADAAAAA2Fi93Z2hoZWEAAAFgAAAAHQAAACQOSgWaaG10eAAAAewAAAAzAAAAVC7TAQBsb2NhAAACgAAAABAAAAAsAOQBAm1heHAAAAGAAAAAHQAAACAAWQALbmFtZQAAAsQAAAF6AAADIYvD/Adwb3N0AAAEQAAAAI4AAADsapk2o3jaY2BkYGAA4ov5mwzj+W2+MnCzXwCKMNzgCBSB0LfbQDQ7AxuI4mBgAlEAFKQIRHjaY2BkYGD3+NvCwMDBAALsDAyMDKhAFAA3+wH3AAAAeNpjYGRgYBBl4GBgYgABEMnIABJzAPMZAAVmAGUAAAB42mNgZlJhnMDAysDCKsKygYGBYRqEZtrDYMT4D8gHSmEHjgUFOQwODAqqf9g9/rYwMLB7MNUAhRlBcsxBrMlASoGBEQAj8QtyAAAAeNrjYGBkAAGmWQwMjO8gmBnIZ2NA0ExAzNjAAFYJVn0ASBsD6VAIDZb7AtELAgANIgb9AHjaY2BgYGaAYBkGRgYQSAHyGMF8FgYPIM3HwMHAxMDGoMCwQIFLQV8hXvXP//9AcRCfAcb///h/ygPW+w/vb7olBjUHCTCyMcAFGZmABBO6AogThgZgIUsXAEDcEzcAAHjaY2BgECMCyoEgACZaAed42mNgYmRgYGBnYGNgYAZSDJqMgorCgoqCjECRXwwNrCAKSP5mAAFGBiRgyAAAi/YFBQAAeNqtkc1OwkAUhU/5M25cEhcsZick0AwlBJq6MWwgJkAgYV/KAA2lJeUn+hY+gktXvpKv4dLTMqKycGHsTZNv7px7z50ZAFd4hYHjdw1Ls4EiHjVncIFnzVnc4F1zDkWjrzmPW+NNcwGlzIRKI3fJlUyrEjZQxb3mDH2fNGfRx4vmHKqG0JzHg6E0F9DOlFBGBxUI1GEzLNT4S0aLuTtsGAEUuYcQHkyg3KmIum1bNUvKlrjbbAIleqHHnS4iSudpQcySMYtdFiXlAxzSbAwfMxK6kZoHKhbjjespMTioOPZnzI+4ucCeTVyKMVKLfeAS6vSWaTinuZwzyy/Dc7vaed+6KaV0kukdPUk6yOcctZPvvxxqksq2lEW8RvHjMEO2FCl/zy6p3NEm0R9OFSafJdldc4QVeyaaObMBO0/5cCaa6d9Ggyubxire+lEojscdjoWUR1xGOy8KD8mG2ZLO2l2paDc3A39qmU2z2W5YNv5+u79e6QfGJY/hAAB42m3NywrCMBQE0DupWp/1AYI7/6DEaLQu66Mrd35BKUWKJSlFv1+rue4cGM7shgR981qSon+ZNwUJ8iDgoYU2OvDRRQ99DDDECAHGmGCKmf80hZSx/Kik/LliFbtmN6xmt+yOjdg9GztV4tROnRwX/Bsaaw51nt4Lc7tWaZYHp/MlzKx51LZs5htNri+2AAAAAQAB//8AD3jaY2BkYGDgAWIxIGZiYARCESBmAfMYAAR6AEMAAAABAAAAANXtRbgAAAAA2AhRFAAAAADYCNuG) format("woff");
}

oxyc · 2019-08-22T14:50:28Z

Found this NxtChg/pieces#3

Props to @lykahb. Is it okay if I credit you for these commits as i'm not really the author?

lykahb · 2019-08-22T15:02:29Z

@oxyc Thanks, I appreciate this. A while ago I moved from the JS CSS parsers to the native browser parser. But it it's nice to see the bug getting fixed.

noisysocks · 2019-08-29T05:55:54Z

I'm not very knowledgeable about the history of this function, but I wonder if it still makes sense for us to have https://github.com/reworkcss/css vendored into Gutenberg rather than included as a package dependency? If it were a dependency then we would get fixes like this simply by bumping the version we depend on.

All that aside, this change looks like a perfect candidate for a unit test. Could we add a test to packages/block-editor/src/utils/transform-styles/test/traverse.js?

oxyc · 2019-08-29T11:28:55Z

If it were a dependency then we would get fixes like this simply by bumping the version we depend on.

This fix has not been merged upstream either and in general the project is quite unmaintained with no releases since September 2018.

I can definitely add a unit test here though.

oxyc · 2019-10-22T13:02:19Z

This might be a PR where the performance impact should be measured but I cant for the life of me get test-performance e2e suits running...

UnhandledPromiseRejectionWarning: Error: Protocol error (Runtime.callFunctionOn): Target closed

…ress#16408) From reworkcss/css#110

oxyc · 2020-03-23T23:20:17Z

Doesnt affect performance.

Before:

Average time to load: 11464ms
Average time to DOM content load: 11422ms
Average time to type character: 33.84ms
Slowest time to type character: 65.16ms
Fastest time to type character: 29.61ms

Average time to load: 10118ms
Average time to DOM content load: 10070ms
Average time to type character: 35.15ms
Slowest time to type character: 77.79ms
Fastest time to type character: 31.23ms

Average time to load: 12113ms
Average time to DOM content load: 12061ms
Average time to type character: 37.24ms
Slowest time to type character: 78.57ms
Fastest time to type character: 32.45ms

Average time to load: 11232ms

After:

Average time to load: 10055ms
Average time to DOM content load: 10010ms
Average time to type character: 34.02ms
Slowest time to type character: 69.81ms
Fastest time to type character: 30.46ms

Average time to load: 12436ms
Average time to DOM content load: 12391ms
Average time to type character: 40.58ms
Slowest time to type character: 90.63ms
Fastest time to type character: 34.67ms

Average time to load: 10252ms
Average time to DOM content load: 10209ms
Average time to type character: 36.49ms
Slowest time to type character: 70.36ms
Fastest time to type character: 31ms

Average time to load: 10914ms

oxyc · 2020-04-02T17:10:56Z

Noteworthy that I've been running a patched version of Gutenberg with this fix since August 2019 with no issues. Only on one site, but the site pushes it's entire frontend stylesheet through the transform. 964kb in development mode (tailwind before purgecss), 142kb (minified, purged).

noisysocks

Thanks for updating the PR and the patience here 🙂

I still would love if we made this a package dependency instead of vendoring the library, but let's not let it stop us fixing this bug.

noisysocks · 2020-04-02T22:56:50Z

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

+ * Internal dependencies
+ */
+import traverse from '../traverse';
+import parse from '../ast/parse';


I think let's move the describe( 'CSS traverse' ) back into test/traverse.js and put the describe( 'CSS parse' ) into a new test/parse.js file.

noisysocks · 2020-04-02T22:57:27Z

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

+
+describe( 'CSS parse', () => {
+	it( 'Should handle closing parenthesis within url', () => {
+		const input = `.a { b: url(")"); d: url(";a"); }`;


String interpolation isn't necessary.

Suggested change

const input = `.a { b: url(")"); d: url(";a"); }`;

const input = '.a { b: url(")"); d: url(";a"); }';

oxyc · 2020-06-13T17:27:04Z

Thanks for the review @noisysocks! and sorry for not responding earlier, but shortly after your review, @strarsis came up with a better approach of replacing reworkcss with css-tree as a package dependency #21936, like you preferred :)

So I'll close this PR in favor of that one now that I've verified that it would fix my issue

strarsis · 2023-10-20T20:44:52Z

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

oxyc requested review from ellatrix, talldan and youknowriad as code owners August 22, 2019 13:35

oxyc force-pushed the transform-styles-svg-fix branch from 6bc994a to 18bc584 Compare August 22, 2019 15:07

gziolo added [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended labels Aug 27, 2019

gziolo requested a review from a team August 27, 2019 07:26

oxyc force-pushed the transform-styles-svg-fix branch from 18bc584 to d3d2022 Compare September 13, 2019 13:10

oxyc requested review from ajitbohra, jorgefilipecosta and Soean as code owners September 13, 2019 13:10

oxyc force-pushed the transform-styles-svg-fix branch 2 times, most recently from 0da0c72 to e9cae9c Compare September 13, 2019 14:36

Boris Lykah and others added 3 commits March 23, 2020 19:44

Handle closing parenthesis within data url in transform-styles (WordP…

3439bdf

…ress#16408) From reworkcss/css#110

handle urls without quotes and with semicolon

4711793

add unit tests for transform-styles parse url cases

49e7b8a

oxyc force-pushed the transform-styles-svg-fix branch from e9cae9c to 49e7b8a Compare March 23, 2020 23:20

oxyc mentioned this pull request Apr 2, 2020

transformStyles not working correctly #16408

Closed

noisysocks requested changes Apr 2, 2020

View reviewed changes

oxyc closed this Jun 13, 2020

oxyc mentioned this pull request Aug 18, 2021

Error while traversing the CSS: property missing ':' #19930

Closed

kraftner mentioned this pull request Apr 19, 2022

Fix or replace CSS Parser that transforms editor styles #40444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oxyc commented Aug 22, 2019 •

edited

Loading

oxyc commented Aug 22, 2019 •

edited

Loading

oxyc commented Aug 22, 2019

lykahb commented Aug 22, 2019

noisysocks commented Aug 29, 2019

oxyc commented Aug 29, 2019

oxyc commented Oct 22, 2019 •

edited

Loading

oxyc commented Mar 23, 2020

oxyc commented Apr 2, 2020

noisysocks left a comment

noisysocks Apr 2, 2020

noisysocks Apr 2, 2020

oxyc commented Jun 13, 2020 •

edited

Loading

strarsis commented Oct 20, 2023

	const input = `.a { b: url(")"); d: url(";a"); }`;
	const input = '.a { b: url(")"); d: url(";a"); }';

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

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

Conversation

oxyc commented Aug 22, 2019 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

oxyc commented Aug 22, 2019 • edited Loading

oxyc commented Aug 22, 2019

lykahb commented Aug 22, 2019

noisysocks commented Aug 29, 2019

oxyc commented Aug 29, 2019

oxyc commented Oct 22, 2019 • edited Loading

oxyc commented Mar 23, 2020

oxyc commented Apr 2, 2020

noisysocks left a comment

Choose a reason for hiding this comment

noisysocks Apr 2, 2020

Choose a reason for hiding this comment

noisysocks Apr 2, 2020

Choose a reason for hiding this comment

oxyc commented Jun 13, 2020 • edited Loading

strarsis commented Oct 20, 2023

oxyc commented Aug 22, 2019 •

edited

Loading

oxyc commented Aug 22, 2019 •

edited

Loading

oxyc commented Oct 22, 2019 •

edited

Loading

oxyc commented Jun 13, 2020 •

edited

Loading