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

Editor styles are broken when a Google Font @import is included #53509

Closed
andreiglingeanu opened this issue Aug 10, 2023 · 9 comments
Closed
Assignees
Labels
[Feature] Typography Font and typography-related issues and PRs [Package] Style Engine /packages/style-engine [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@andreiglingeanu
Copy link
Contributor

Description

When the editor style contains this line, it breaks the page entirely:

@import url('https://fonts.googleapis.com/css2?family=Lobster:wght@400;500;600;700&display=swap');

Step-by-step reproduction instructions

  1. Start with any classic theme
  2. Put the code from down below in the functions.php
  3. Observe the problem
add_editor_style('some-style.css');
@import url('https://fonts.googleapis.com/css2?family=Lobster:wght@400;500;600;700&display=swap');

:root {
	--test: red;
}

Result:

CleanShot 2023-08-10 at 10 57 06@2x

Expected result:

@import url('https://fonts.googleapis.com/css2?family=Lobster:wght@400;500;600;700&display=swap');

.editor-styles-wrapper {
	--test: red;
}

Screenshots, screen recording, code snippet

This seems like a problem within the block-editor/src/utils/transform-styles/transforms/wrap.js file.

Environment info

WordPress 6.3, a classic theme.

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

@cbirdsong
Copy link

This also occurs when using an @import for Adobe Fonts/Typekit.

@jordesign jordesign added [Type] Bug An existing feature does not function as intended Needs Testing Needs further testing to be confirmed. [Feature] Fonts API [Package] Style Engine /packages/style-engine labels Aug 10, 2023
@jorgefilipecosta jorgefilipecosta added [Priority] High Used to indicate top priority items that need quick attention and removed Needs Testing Needs further testing to be confirmed. labels Aug 14, 2023
@jorgefilipecosta
Copy link
Member

Howdy @andreiglingeanu and @cbirdsong, much appreciation for flagging the problem. I delved into it and successfully replicated it. This glitch might impact the editor styles on many sites.

@jorgefilipecosta
Copy link
Member

The issue stems from a bug in the CSS parsing library we used:
https://github.com/reworkcss/css

It's feasible to reproduce this using:

const css = require('css');
const obj = css.parse(`@import url('https://fonts.googleapis.com/css2?family=Lobster:wght@400;500;600;700&display=swap');

:root {
	--test: red;
}`);
console.log(obj);

For instance, on npm runkit: https://npm.runkit.com/css.

Our task involves rectifying our local fork packages/block-editor/src/utils/transform-styles/ast/parse.js and submitting an upstream patch to ensure universal benefit from the fix.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Aug 14, 2023
@annezazu annezazu added [Feature] Typography Font and typography-related issues and PRs and removed [Feature] Fonts API labels Sep 25, 2023
@annezazu
Copy link
Contributor

@jorgefilipecosta what can be done to move forward on this? This issue is labeled high priority but has sat for a number of months. The high priority label should be used for important items that need quick attention. To help ensure these issues are actionable, I'm doing a sweep and wanted to check in here to ensure we close this promptly whether that's with your PR or bringing in others to help resolve.

@skorasaurus
Copy link
Member

skorasaurus commented Dec 19, 2023

@annezazu @jorgefilipecosta
I don't think this is applicable as the reworkcss library has been replaced with PostCSS in #40444 ;

I'm unable to reproduce the error that @andreiglingeanu posted; i'm able to load the font in the editor as expected.

(if I add the following to the css file enqueued by add_editor_style)

h2 {
    font-family: 'Lobster';
}

the heading block with h2 will use that font in the editor

note, the new result is:


@import url('https://fonts.googleapis.com/css2?family=Lobster:wght@400;500;600;700&display=swap');

:root {
	--test: red;
}

(I believe the that root selector appearing instead of editor-styles-wrapper is now intentional; cc @strarsis

@annezazu
Copy link
Contributor

!! Amazing triaging and diving in @skorasaurus. I'm going to proactively remove the high priority label from now but keep in mind we can always add in back in as this updated convo unfolds.

@annezazu annezazu removed the [Priority] High Used to indicate top priority items that need quick attention label Dec 19, 2023
@andreiglingeanu
Copy link
Contributor Author

@skorasaurus I can re-validate the initial issue against the latest trunk, if it helps. But it all seems correct, judging from your comment.

@strarsis
Copy link
Contributor

strarsis commented Dec 26, 2023

(I believe the that root selector appearing instead of editor-styles-wrapper is now intentional; cc @strarsis

The CSS variables should also be isolated by adding .editor-styles-wrapper to the selector (also when defined in root scope).

I don't think this is applicable as the reworkcss library has been replaced with PostCSS in #40444 ;

🤔 Interesting, IMHO that CSS parser code was basically developed internally, and therefore replaced by a proper library.

@jorgefilipecosta
Copy link
Member

I verified this issue was fixed with the merge of #49521.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Typography Font and typography-related issues and PRs [Package] Style Engine /packages/style-engine [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants