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

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

Closed
romanfinkwp opened this issue Nov 18, 2023 · 19 comments · Fixed by #56564
Closed
Assignees
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Testing Needs further testing to be confirmed. [Status] In Progress Tracking issues with work in progress [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended

Comments

@romanfinkwp
Copy link

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

@romanfinkwp romanfinkwp added the [Type] Bug An existing feature does not function as intended label Nov 18, 2023
@skorasaurus skorasaurus added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Nov 18, 2023
@jordesign jordesign added the Needs Testing Needs further testing to be confirmed. label Nov 19, 2023
@michalczaplinski
Copy link
Contributor

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:

// This CSS Custom Properties aren't used anymore as defaults,
// but we still need to keep them for backward compatibility.
--wp--preset--font-size--normal: 16px;
--wp--preset--font-size--huge: 42px;
}

// This CSS Custom Properties aren't used anymore as defaults,
// but we still need to keep them for backward compatibility.
.editor-styles-wrapper {
--wp--preset--font-size--normal: 16px;
--wp--preset--font-size--huge: 42px;
}

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
Copy link
Contributor

It looks like the rules were added in two places back in #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
Copy link
Contributor

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
Copy link
Member

oandregal commented Nov 27, 2023

The code at #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
Copy link
Member

This is my test:

  • Using TwentyTwentyThree theme, I set settings.typography.fontSizes to:
[
  {
    "fluid": {
    "max": "42px",
    "min": "32px"
  },
  "name": "Normal",
  "size": "42px",
  "slug": "normal"
 }
]
  • Add a paragraph and set it to font size normal in the editor.

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

  • the .has-normal-font-size is properly set
  • the style="font-size: 42px;" is also properly set
<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
Copy link
Member

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
Copy link
Member

oandregal commented Nov 27, 2023

Here's what I know:

  • The styles for normal and huge font sizes is a backwards compatibility mechanism. At the beginning, these were part of the default font sizes we define (see the current ones). At some point, they were removed. However, there could be old content that used those classes, so we still needed to enqueue them (but wanted not to list them in the design tools). That's why there were introduced in common.scss and editor.scss.
  • common.scss is for the front-end and editor.scss is for editor. They were different targets. For example, editor styles were scoped with .editor-style-wrapper and body was substituted by .editor-style-wrapper: this was to make sure the editor style rules didn't leak outside it. At least, it was this way until the introduction of the iframe in post editor in WordPress 6.3. Furthermore, in 6.4, #46752 removed that scoping, which is what caused this issue.

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

  • iframed editor: the .editor-styles-wrapper shouldn't be necessary
  • not iframed editor: still need .editor-styles-wrapper wrapper class

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
Copy link
Member

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
Copy link
Member

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:

  • There's some usage in the block-library styles (editor.scss, reset.scss, blocks, etc.).
  • There are some block styles that use it (I've seen layout, for example). Pinging @andrewserong @tellthemachines just for awareness, I don't know if there is actually any action to take.

@ellatrix
Copy link
Member

Make a PR test here: #56564

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 27, 2023
@tellthemachines
Copy link
Contributor

  • 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
Copy link
Contributor

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:
b18a270?diff=unified#diff-bda8e417e9cfc8724167f4c82aab1d0d205245c51360a8afa4bba99ba078e6db

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

Copy link

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.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Dec 29, 2023
@aaronjorbin
Copy link
Member

@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

@github-actions github-actions bot removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 18, 2024
@ellatrix
Copy link
Member

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
Copy link
Member

ellatrix commented Jan 18, 2024

A new issue also sounds good :)

Copy link

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.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 19, 2024
@ramonjd
Copy link
Member

ramonjd commented Apr 3, 2024

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

@t-hamano
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Testing Needs further testing to be confirmed. [Status] In Progress Tracking issues with work in progress [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.