WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.51k stars 4.2k forks source link

New CSS properties (WP 6.4) in editor.min.css are overwriting font sizes from theme.json #56293

Closed romanfinkwp closed 7 months ago

romanfinkwp commented 11 months ago

Description

I discovered a weird issue in WP 6.4.

There is CSS for editor styles (editor.min.css):

.editor-styles-wrapper {
    --wp--preset--font-size--normal: 16px;
    --wp--preset--font-size--huge: 42px
}

These properties are overwriting my font sizes from theme.json with slugs “normal” and “huge.”

These properties weren’t there in WordPress 6.3!

Because of that, in the Editor blocks with these properties are displaying with incorrect font sizes taken from these properties.

For example, my font size style with the slug “normal” is 24px. However, because of these properties in editor.min.css the block with “normal” font-size becomes 16px. The same issue occurs with the “huge” font-size… However, in the frontend, everything is okay.

Why are these properties there? Maybe it’s a mistake?

Step-by-step reproduction instructions

  1. Just open the file 'wp-includes\css\dist\block-library\editor.min.css' (WordPress 6.4)
  2. In this file there are CSS:
.editor-styles-wrapper {
    --wp--preset--font-size--normal: 16px;
    --wp--preset--font-size--huge: 42px
}
  1. These properties are overwriting properties from theme.json. If the theme.json uses Font Size values with slugs 'normal' and 'huge,' for example:
"fontSizes": [
        {
          "fluid": {
            "max": "24px",
            "min": "18px"
          },
          "name": "Normal",
          "size": "24px",
          "slug": "normal"
        },
]

Screenshots, screen recording, code snippet

No response

Environment info

WordPress 6.4.1

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

michalczaplinski commented 11 months ago

I can reproduce it. The problem seems to be that the following styles:

.editor-styles-wrapper {
    --wp--preset--font-size--normal: 16px;
    --wp--preset--font-size--huge: 42px;
}

have priority in the CSS cascade over these styles:

body {
    --wp--preset--font-size--normal: <generated-value>;
}

I can see that we're defining the normal & huge font sizes in 2 places:

https://github.com/WordPress/gutenberg/blob/e95bb8c9530bbdef1db623eca11b80bd73493197/packages/block-library/src/common.scss#L11-L15

https://github.com/WordPress/gutenberg/blob/e95bb8c9530bbdef1db623eca11b80bd73493197/packages/block-library/src/editor.scss#L66-L71

It seems redundant to me to define them in 2 places and removing the second one fixes the problem. I'm worried that it might have some backwards-compatibility problems though. Does this fix make sense to you @oandregal @andrewserong ?

andrewserong commented 11 months ago

It looks like the rules were added in two places back in https://github.com/WordPress/gutenberg/pull/37381, perhaps @oandregal might know more about why it was in both places?

If we need to keep the additional rule in editor.scss around, could one potential solution be to wrap it in a :where() to reduce its specificity?

michalczaplinski commented 11 months ago

Oh, I've been living under a rock because I didn't know about :where() 🤦 That might work! Thanks Andrew!

I'll wait for @oandregal's response because the huge and normal font `sizes have already had their specificity adjusted at least twice so there's possibly some nuance that I'm missing.

oandregal commented 11 months ago

The code at https://github.com/WordPress/gutenberg/pull/37381 was part of 5.9. Something else must have changed in 6.4 so that it worked properly in 6.3 (and before?). Before jumping into a fix, I'd like to understand what changed.

oandregal commented 11 months ago

This is my test:

[
  {
    "fluid": {
    "max": "42px",
    "min": "32px"
  },
  "name": "Normal",
  "size": "42px",
  "slug": "normal"
 }
]

In the devtools inspector, I look at the paragraph's HTML. It is the same for 6.3 and 6.4:

<p class="has-normal-font-size" style="font-size: 42px;">Some paragraph.</p>

Then I look at what are the styles that set font-size for this block:

CSS for 6.3 CSS for 6.4
Captura de ecrã 2023-11-27, às 12 02 23 Captura de ecrã 2023-11-27, às 12 02 30

Note how, in 6.3, the styles generated for the block (tag <style) use the .editor-style-wrapper .has-normal-font-size selector, while in 6.4 they use .has-normal-font-size selector.

This is why 6.4 has introduced this regression.

oandregal commented 11 months ago

So, in 6.4, the global styles are appended to the editor without being transformed: they use body and .has-normal-font-size instead of .editor-styles-wrapper and .editor-styles-wrapper .has-normal-font-size.

oandregal commented 11 months ago

Here's what I know:

We're now in a weird place, because there may be a few cases:

We need to provide the backward-compatible font size styles in both, though each needs its own CSS. I'm going to ping @ellatrix here, for thoughts on how to fix this issue.

ellatrix commented 11 months ago

Scoping should only be added in case the editor is not iframed, .editor-styles-wrapper should not be hardcoded in stylesheet, ideally.

This already happens for editor styles passed through the block editor setting or the canvas prop.

But I guess there's some other cases where either the CSS added in a different way or through a loaded stylesheet.

If you're adding styles dynamically, it should be easy to check whether the editor is iframed, and then don't add the .editor-styles-wrapper class.

Didn't :where(.editor-styles-wrapper) work to remove the specificity it adds?

oandregal commented 11 months ago

Based on the investigation above, using :where should be enough and adequate to fix this.

In light of #46752 it seems worth revisiting the usage of .editor-styles-wrapper widely:

ellatrix commented 11 months ago

Make a PR test here: #56564

tellthemachines commented 11 months ago
  • There are some block styles that use it (I've seen layout, for example)

It should be fine to remove the class from layout but we'll need to check if the specificity change doesn't affect anything.

hellofromtonya commented 11 months ago

Reopening for further consideration of issue reported in Core's Trac ticket 59835, which highlights the same GB PR #46752 but the changes in the packages/block-library/src/reset.scss which removed the html from the wrapper:

Actually it comes from a change in wp-includes/css/dist/block-library/reset.css where all rules were prefixed with html selector prior to 6.4.

The change was introduced by this commit: https://github.com/WordPress/gutenberg/commit/b18a270ae4b5d71cc0d880d40d21b5163686cd92?diff=unified#diff-bda8e417e9cfc8724167f4c82aab1d0d205245c51360a8afa4bba99ba078e6db

@ellatrix is this bug report related? Or should a new issue be opened to address it?

github-actions[bot] commented 10 months ago

Hi, This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps. Thanks for helping out.

aaronjorbin commented 9 months ago

@ellatrix any chance you can help answer @hellofromtonya's question? This is related to an issue that would be great to be fixed in the next 6.4 maintenance release which is scheduled for 30 January

ellatrix commented 9 months ago

By which stylesheet are these reset styles overriden? Worth noting that reset styles are supposed to be overridden by the theme, so it would be good to debug what is overriding them and why that is not expected. How do I reproduce the issue and with what theme?

ellatrix commented 9 months ago

A new issue also sounds good :)

github-actions[bot] commented 8 months ago

Hi, This issue has gone 30 days without any activity. This means it is time for a check-in to make sure it is still relevant. If you are still experiencing this issue with the latest versions, you can help the project by responding to confirm the problem and by providing any updated reproduction steps. Thanks for helping out.

ramonjd commented 7 months ago

It's not great, and I'm not proud of it, but here's a potential quick fix:

t-hamano commented 7 months ago

As far as I've researched, this issue was fixed in WP6.5 and then we had a similar issue with the latest Gutenberg plugin. See the survey results here.

And since this problem should have been fixed with #60400 I would like to close this issue.