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

Scripts: Control Directives for RTLCSS don't work in build command #68364

Open
2 of 6 tasks
t-hamano opened this issue Dec 28, 2024 · 13 comments · May be fixed by #68456
Open
2 of 6 tasks

Scripts: Control Directives for RTLCSS don't work in build command #68364

t-hamano opened this issue Dec 28, 2024 · 13 comments · May be fixed by #68456
Labels
[Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended

Comments

@t-hamano
Copy link
Contributor

Description

In #61540, wp-scripts supported CSS for RTL languages. Also, with RTLCSS, you can add some processing via the Control Directives.

However, this Control Directives does not seem to work with the build command.

I thought that #68201 might be affecting this, but it seems that's not the case. I don't know if the control directives were not working from the beginning.

cc @gziolo @ryelle

Step-by-step reproduction instructions

Use the following command to create a block without wp-script:

npx wp-create-block my-block --no-wp-scripts
cd my-block

Update the my-block/src/editor.scss file as follows:

.wp-block-create-block-my-block {
	/*rtl:ignore*/
	text-align: left;
	/*rtl:remove*/
	background: #ccc;
}

✅ Run ../node_modules/.bin/wp-scripts start and check the build file. Confirm that the controls directive is working.

index.css:

.wp-block-create-block-my-block {
  /*rtl:ignore*/
  text-align: left;
  /*rtl:remove*/
  background: #ccc;
}

index-rtl.css:

.wp-block-create-block-my-block {
  text-align: left;
}

❌ Run ../node_modules/.bin/wp-scripts build and check the build file. Confirm that the controls directive is NOT working.

index.css:

.wp-block-create-block-my-block{
	background:#ccc;
	text-align:left
}

index-rtl.css:

.wp-block-create-block-my-block{
	background:#ccc;
	text-align:right
}

Screenshots, screen recording, code snippet

b57b77409fdef37f9e0aea1bd2260107.mp4

Environment info

OS: WSL2 (Ubuntu 22.04.4 LTS)

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

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@t-hamano t-hamano added [Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended labels Dec 28, 2024
@t-hamano t-hamano changed the title Scripts: Contril Directives for RTLCSS don't work in build command Scripts: Control Directives for RTLCSS don't work in build command Dec 28, 2024
@Rishit30G
Copy link
Contributor

I can confirm that the issue is present, sharing the screencast for the same:
https://github.com/user-attachments/assets/e39e92da-825c-445e-a429-4bc5fbf70dbb

@SmushyTaco
Copy link

@t-hamano @Rishit30G when looking at start.js and build.js the problem seems to come from:

process.env.NODE_ENV = process.env.NODE_ENV || 'production';

which is in the build.js but not the start.js. When your SCSS code is processed in production mode, the comments are optimized out before RTLCSS gets the file for processing. So to fix this, comments need to be kept in for production mode and then removed only after RTLCSS runs.

@SmushyTaco
Copy link

SmushyTaco commented Jan 2, 2025

Changing this from:

options: {
	sourceMap: ! isProduction,
}

to:

options: {
	sourceMap: ! isProduction,
	sassOptions: {
		style: 'expanded'
	}
}

will fix it and make control directives work with production builds. The only issue is that the outputted CSS won't be minified anymore. So something needs to run after SCSS compilation and RTLCSS processing to remove comments and minify the CSS files once again.

@SmushyTaco
Copy link

SmushyTaco commented Jan 2, 2025

@t-hamano @Rishit30G I found a solution! Instead of doing:

/*rtl:ignore*/

Do:

/*!rtl:ignore*/

The ! right after /* keeps the comment in as seen here, even in SCSS compressed mode (which is what's used by default in production). No changes to the webpack.config.js is needed. Just use this comment style:

/*! */

Instead of this one:

/* */

This issue can be closed now.

@t-hamano
Copy link
Contributor Author

t-hamano commented Jan 2, 2025

@SmushyTaco Thanks for investigating.

The ! right after /* keeps the comment in as seen here, even in SCSS compressed mode (which is what's used by default in production). No changes to the webpack.config.js is needed. Just use this comment style:

It may indeed work, but it seems like a temporary workaround. Ideally, all control directives would work as expected and all comments should be removed from the built files.

So I'm leaving this issue open.

@SmushyTaco
Copy link

SmushyTaco commented Jan 2, 2025

@t-hamano it's not an issue though, it all works as intended. RTLCSS processes CSS files not any CSS preprocessors like SCSS, so the RLTCSS documentation is written accordingly. If the documentation was written for SCSS it would say what I'm saying. SCSS very clearly defines what you need to do to keep comments in for compilation in a production environment, we just weren't aware of the proper way of using it with SCSS.

@Rishit30G
Copy link
Contributor

t-hamano Rishit30G I found a solution! Instead of doing:

Thanks for sharing the solution @SmushyTaco, but unfortunately it did not work for me, maybe you can check once

Screen.Recording.2025-01-02.at.10.41.57.AM.mov

@SmushyTaco
Copy link

That's odd, I tested it on my PC and it worked for me. I'll investigate more later and possibly open a PR for RTLCSS if needed.

@SmushyTaco
Copy link

SmushyTaco commented Jan 2, 2025

@Rishit30G from this code, the match passes with the ! so it should be working.

This is the input I've used to test:

.wp-block-create-block-my-block {
  /*!rtl:ignore*/
  text-align: left;
  /*!rtl:remove*/
  background: #CCC;
}

When I build with:

npx webpack-cli --mode production --config node_modules/@wordpress/scripts/config/webpack.config.js

the index.css file is:

.wp-block-create-block-my-block{/*!rtl:ignore*/text-align:left;/*!rtl:remove*/background:#ccc}

and the index-rtl.css file is:

.wp-block-create-block-my-block{text-align:left}

But when I build with:

npx wp-scripts build

the index.css file is:

.wp-block-create-block-my-block{background:#ccc;text-align:left}

and the index-rtl.css file is:

.wp-block-create-block-my-block{background:#ccc;text-align:right}

The issue is the usage of cssnano here. When it's set to false it also misplaces the comments like so:

index.css

.wp-block-create-block-my-block{background:#ccc;text-align:left/*!rtl:remove*//*!rtl:ignore*/}

index-rtl.css:

.wp-block-create-block-my-block{background:#ccc;text-align:right/*!rtl:remove*//*!rtl:ignore*/}

And when I completely remove it, it works as intended. So to summarize, when using with RTLCSS control directives with SCSS you need to use this comment format:

/*! */

and the usage of cssnano needs to be fixed. I'll investigate to see what the solution could be.

@SmushyTaco
Copy link

SmushyTaco commented Jan 2, 2025

Okay I've found a solution. Changing this from:

preset: [
	'default',
	{
		discardComments: {
			removeAll: true,
		},
	},
],

to:

preset: [
	'default',
	{
		discardComments: {
			remove(comment) {
			    return !/^\s*!?\s*rtl:/.test(comment);
                        },
		},
		cssDeclarationSorter: false
	},
],

Fixes everything. I'll open a PR to fix and close this issue.

SmushyTaco added a commit to SmushyTaco/gutenberg that referenced this issue Jan 2, 2025
This solves WordPress#68364 to allow for the proper use of control directives.
@SmushyTaco SmushyTaco linked a pull request Jan 2, 2025 that will close this issue
@gziolo
Copy link
Member

gziolo commented Jan 2, 2025

@t-hamano, thank you for the report. It looks like it never fully worked with the /*rtl:ignore*/ on the production build when using WP Scripts. @SmushyTaco, good job finding the root cause. It makes me wonder whether we could use rtlcss directly as a PostCSS plugin instead. If it was executed before cssnano then the comments should be correctly processed.

More realistic alternative would be to move cssnano execution to the forked rtlcss-webpack-plugin as it looks like it triggers after all PostCSS processing here:

files.filter( cssOnly ).forEach( ( filename ) => {
// Get the asset source for each file generated by the chunk:
const src = compilation.assets[ filename ].source();
const dst = rtlcss.process( src );
const dstFileName = compilation.getPath( '[name]-rtl.css', {
chunk,
cssFileName: filename,
} );
compilation.assets[ dstFileName ] =
new webpack.sources.RawSource( dst );
chunk.files.add( dstFileName );
} );

@SmushyTaco
Copy link

@gziolo I actually think the fork should be deleted and that this fork should be put in it's place since it's rewritten in TypeScript and will continue to be maintained by me (#68303). If you want an alternative solution than the one I provided, I can look into it more tomorrow.

@gziolo
Copy link
Member

gziolo commented Jan 2, 2025

Great to hear you plan to maintain the rtlcss-webpack-plugin fork 👏🏻

I don't think it matters in this case whether the WordPress project uses its copy of the webpack plugin or not. In fact, if you plan to maintain a fork of rtlcss-webpack-plugin, then it could also fix that issue with the cssnano integration somehow without the need for additional intervention from devs if they want to remove all code comments from the production version of CSS. I would definitely explore alternatives before committing to the final solution here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] WP Scripts /packages/scripts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants