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

Transform styles utility doesn't prefix CSS selectors correctly for some complex selectors #64395

Closed
2 tasks done
andreiglingeanu opened this issue Aug 9, 2024 · 3 comments · Fixed by #64458
Closed
2 tasks done
Assignees
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@andreiglingeanu
Copy link
Contributor

andreiglingeanu commented Aug 9, 2024

Description

transformStyles() styles utility works incorrectly for some special selectors. I assume this is because of an issue in postcss-prefixwrap which is used by transformStyles(). I did try to reproduce this issue in postcss-prefixwrap but their test setup is giving me some headaches.

Step-by-step reproduction instructions

Easiest way to reproduce it is to add the following CSS snippet in the block editor while the iframe isn't present, so that the prefixing with .editor-styles-wrapper takes place.

:is(.is-layout-flow, .is-layout-constrained) > *:where(:not(h1, h2, h3, h4, h5, h6)) {
  margin-block-start: 0;
}
:is(.is-layout-flow, .is-layout-constrained) :where(h1, h2, h3, h4, h5, h6) {
  margin-block-end: 0
}

The resulting CSS is this (WRONG):

.test :is(.is-layout-flow, .test .is-layout-constrained) > *:where(:not(h1, .test h2, .test h3, .test h4, .test h5, .test h6)) {
  margin-block-start: 0;
  margin-block-end: var(--theme-content-spacing);
}
.test :is(.is-layout-flow, .test .is-layout-constrained) :where(h1, .test h2, .test h3, .test h4, .test h5, .test h6) {
  margin-block-end: calc(var(--has-theme-content-spacing, 1) * (0.3em + 10px));
}

As you can see, the .test prefix is added after every comma, which is obviously incorrect. The correct output would be this (CORRECT):

.test :is(.is-layout-flow, .is-layout-constrained) > *:where(:not(h1, h2, h3, h4, h5, h6)) {
  margin-block-start: 0;
  margin-block-end: var(--theme-content-spacing);
}
.test :is(.is-layout-flow, .is-layout-constrained) :where(h1, h2, h3, h4, h5, h6) {
  margin-block-end: calc(var(--has-theme-content-spacing, 1) * (0.3em + 10px));
}

I know this potentially involves work on the parser but I'm happy to do it if you give me some pointers on where to start looking. If the way is debugging postcss-prefixwrap, I will do that.

Screenshots, screen recording, code snippet

Easiest way to reproduce this problem is to add the following test case in the packages/block-editor/src/utils/test/transform-styles.js test file:

it( 'should prefix complex selector correctly', () => {
	const input = `:is(.is-layout-flow, .is-layout-constrained) > *:where(:not(h1, h2, h3, h4, h5, h6)) {
margin-block-start: 0;
margin-block-end: var(--theme-content-spacing);
}
:is(.is-layout-flow, .is-layout-constrained) :where(h1, h2, h3, h4, h5, h6) {
margin-block-end: calc(var(--has-theme-content-spacing, 1) * (0.3em + 10px));
}`;

	const output = transformStyles(
		[
			{
				css: input,
			},
		],
		'.test'
	);

	console.log( 'here', { output } );

	expect( output ).toBeTruthy();
} );

You will see that the output is as above.

Environment info

  • Latest gutenberg repo, trunk branch

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@andreiglingeanu andreiglingeanu added the [Type] Bug An existing feature does not function as intended label Aug 9, 2024
@talldan
Copy link
Contributor

talldan commented Aug 9, 2024

@andreiglingeanu Thanks for reporting. I think it's the same issue as reported in #63829.

I worked on a fix in #63972, though I'd also reported the issue upstream (dbtedman/postcss-prefixwrap#515) and I see it's be fixed now in postcss-prefixwrap, so I can look into updating the dependency as a simpler fix. I'll get onto that when I'm back at work on Monday. 👍

@andreiglingeanu
Copy link
Contributor Author

@talldan this is amazing news! Thanks a lot 🙏

@andreiglingeanu
Copy link
Contributor Author

I can confirm that the output is correct with postcss-prefixwrap 1.51.0
CleanShot 2024-08-09 at 14 05 45@2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
2 participants